Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EIP-7805 (FOCIL) specs #4003

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from

Conversation

terencechain
Copy link
Contributor

@terencechain terencechain commented Nov 4, 2024

This PR introduces a preliminary version of the EIP-7805 consensus specs, covering the beacon chain, fork choice, and validator. While there are still open questions regarding the consensus design, we believe it's early enough to open this up for broader community feedback. Major credit to @soispoke, @fradamt, @Ma-Julian, @jihoonsong, and everyone else who provided feedback along the way

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments from an initial skim.

specs/_features/eip7805/beacon-chain.md Outdated Show resolved Hide resolved
specs/_features/eip7805/fork-choice.md Outdated Show resolved Hide resolved
```python
def validate_inclusion_lists(store: Store, inclusion_list_transactions: List[Transaction, MAX_TRANSACTIONS_PER_INCLUSION_LIST * IL_COMMITTEE_SIZE], execution_payload: ExecutionPayload) -> bool:
"""
Return ``True`` if and only if the input ``inclusion_list_transactions`` satifies validation, that to verify if the `execution_payload` satisfies `inclusion_list_transactions` validity conditions either when all transactions are present in payload or when any missing transactions are found to be invalid when appended to the end of the payload unless the block is full.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should line wrap this. And execution_payload and inclusion_list_transactions should be wrapped in double back-ticks; they currently use single back-ticks.

Comment on lines 38 to 39
### Modified `Store`
**Note:** `Store` is modified to track the seen inclusion lists.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a blank line between these lines.

specs/_features/eip7805/beacon-chain.md Outdated Show resolved Hide resolved
Comment on lines 55 to 58
- _[REJECT]_ The signature of `inclusion_list.signature` is valid with respect to the validator index.


### The Req/Resp domain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the extra blank line.

epoch: Epoch,
validator_index: ValidatorIndex) -> Optional[Slot]:
"""
Returns the slot during the requested epoch in which the validator with index `validator_index`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs double back-ticks too.

Comment on lines 89 to 92
The proposer should call `engine_updateInclusionListV1` at `PROPOSER_INCLUSION_LIST_CUT_OFF` into the slot with the list of the inclusion lists that gathered since `inclusion_list_CUT_OFF`


## New inclusion list committee duty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete extra blank line here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the sentence on line 89 needs punctuation.

specs/_features/eip7805/validator.md Outdated Show resolved Hide resolved

| `fork_version` | Chunk SSZ type |
|------------------------|------------------------------------------|
| `EIP-7805_FORK_VERSION` | `EIP-7805.SignedInclusionList` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this dash will work here. In most places, we need to replace EIP-7805 with EIP7805.

specs/_features/eip7805/beacon-chain.md Outdated Show resolved Hide resolved
specs/_features/eip7805/beacon-chain.md Outdated Show resolved Hide resolved
specs/_features/eip7805/validator.md Outdated Show resolved Hide resolved
specs/_features/eip7805/validator.md Outdated Show resolved Hide resolved
Comment on lines 64 to 65
`on_inclusion_list` is called to import `signed_inclusion_list` to the fork choice store.
```python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a blank line between the paragraph & code block.

specs/_features/eip7805/fork-choice.md Outdated Show resolved Hide resolved
inclusion_list_committee: Vector[ValidatorIndex, IL_COMMITTEE_SIZE]]) -> None:
"""
``on_inclusion_list`` verify the inclusion list before importing it to fork choice store.
If there exists more than 1 inclusion list in store with the same slot and validator index, remove the original one.
Copy link
Member

@jtraglia jtraglia Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is an out-of-date comment. Where is "remove the original one" happening exactly? It appears that it adds the validator to inclusion_list_equivocators instead now.

Copy link
Contributor

@ensi321 ensi321 Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is an out-of-date comment. Where is "remove the original one" happening exactly? It appears that it adds the validator to inclusion_list_equivocators instead now.

I thought it's the other way around?

p2p only accepts an IL if it's the first or second IL broadcasted by the peer.

The responsibility of picking out the equivocators should be on the p2p side. Beacon node just replace the first IL with the second IL if that happens

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments are outdated, and different client implementations may handle this differently. Essentially, we want to:

  • Cache at least one inclusion list
  • Re-broadcast up to two inclusion lists over P2P
  • Track which inclusion lists have been equivocated (e.g., identify the specific validator)
  • When calling the EL to validate inclusion lists, ignore those that have been equivocated (discard them from being sent to the EL)
  • Regarding whether to prioritize the first or second inclusion list, I'm leaning towards the first as it seems simpler

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is outdated, yeah. It doesn't really matter which one we keep, we could also not keep either since anyway we ignore them once we know that it is an equivocation. For spec simplicity it seemed easier to just not do anything instead of removing, and also perhaps it makes sense to keep the first, just because if you're the builder you would probably rather use it even if there's an equivocation, just in case, and you'd rather use the first since you saw it earlier and from your perspective it is more likely to be enforced by someone

specs/_features/eip7805/fork-choice.md Outdated Show resolved Hide resolved

This topic is used to propagate signed inclusion list as `SignedInclusionList`.
The following validations MUST pass before forwarding the `inclusion_list` on the network, assuming the alias `message = signed_inclusion_list.message`:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we also limiting IL size by MAX_BYTES_PER_INCLUSION_LIST = 8192 bytes as indicated in the EIP?


#### Messages

##### InclusionListByCommitteeIndices v1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need another endpoint like InclusionListByRange since recently joined nodes need to request past ILs to sync blocks since the last checkpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not. Inclusion lists only matter for the current slot. When a node is syncing blocks from past slots, the inclusion list check can be skipped

Co-authored-by: Justin Traglia <[email protected]>
#### `SignedInclusionList`

```python
class SignedInclusionList(Container):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we introduce a SignedInclusionList container instead of just adding the signature field directly onto InclusionList?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants