Gloas payload envelope processing [WIP]#8806
Gloas payload envelope processing [WIP]#8806eserilev wants to merge 16 commits intosigp:unstablefrom
Conversation
…oas-payload-processing
…oas-payload-processing
…oas-payload-processing
…oas-payload-processing
jimmygchen
changed the title
…oas-payload-processing
…oas-payload-processing
| // TODO(gloas) are these other logs important? | ||
| root = ?beacon_block_root, | ||
| // graffiti = block.body().graffiti().as_utf8_lossy(), | ||
| // proposer_index = block.proposer_index(), | ||
| // slot = %block.slot(), |
There was a problem hiding this comment.
we wont have this info post gloas, I'd like to delete these fields if thats okay
There was a problem hiding this comment.
slot might be useful too, otherwise i agree
| // TODO(gloas) are these other logs important? | ||
| root = ?beacon_block_root, | ||
| // graffiti = block.body().graffiti().as_utf8_lossy(), | ||
| // proposer_index = block.proposer_index(), | ||
| // slot = %block.slot(), |
There was a problem hiding this comment.
would like to delete these as well
|
This one should be ready for a first pass |
|
I tried looking at removing the beacon chain dependency from the gossip path. It didn't seem to make unit testing that much easier, the dependency on |
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
mergify
bot
added
waiting-on-author
|
I'll take a look today |
pawanjay176
left a comment
pawanjay176
left a comment
There was a problem hiding this comment.
Some early comments, still going through post gossip verification
| block_root: &Hash256, | ||
| ) -> Result<Option<SignedBeaconBlock<T::EthSpec>>, Error> { | ||
| match self.store.try_get_full_block(block_root)? { | ||
| Some(DatabaseBlock::Full(block)) => Ok(Some(block)), |
There was a problem hiding this comment.
We could potentially look up a block status table here to figure out if we have processed the block and it turned out to be invalid
| let beacon_block_root = envelope.beacon_block_root; | ||
|
|
||
| // Check that we've seen the beacon block for this envelope and that it passes validation. | ||
| // TODO(EIP-7732): We might need some type of status table in order to differentiate between: |
There was a problem hiding this comment.
This is a great point in favour of having some sort of a block status table because invalid blocks aren't kept in fork choice.
So for a payload referencing an invalid block, we have no way of knowing if we have already processed and rejected the block without keeping that info around.
| }); | ||
| }; | ||
|
|
||
| let latest_finalized_slot = fork_choice_read_lock |
There was a problem hiding this comment.
Should we be using the canonical_head for this like we do for blocks? Also, we should probably get this post making sure the block is available in fork choice
|
|
||
| // Verify the envelope signature. | ||
| // | ||
| // For self-build envelopes, we can use the proposer cache for the fork and the |
There was a problem hiding this comment.
| // For self-build envelopes, we can use the proposer cache for the fork and the | |
| // For self-built envelopes, we can use the proposer cache for the fork and the |
| // 2. Blocks we've seen that are invalid (REJECT). | ||
| // | ||
| // Presently these two cases are conflated. | ||
| let fork_choice_read_lock = chain.canonical_head.fork_choice_read_lock(); |
There was a problem hiding this comment.
If we have a block_processing_table, we could have a Processed(Bid, bool) state that is only entered post adding to fork choice. That way, we could potentially need only a single call to make sure the block is valid and to do all consequent checks with the bid
| %beacon_block_root, | ||
| "Proposer shuffling cache miss for envelope verification" | ||
| ); | ||
| let snapshot = load_snapshot(envelope_ref, chain)?; |
There was a problem hiding this comment.
just noting that load_snapshot again acquires the fork_choice lock which it uses to basically get what we have as proto_block in the current scope. Maybe we can pass an optional proto_block to the function as well?
There was a problem hiding this comment.
actually this is just a read_lock so maybe not that big a deal
| Ok((snapshot.state_root, snapshot.pre_state)) | ||
| }, | ||
| )?; | ||
| let fork = proposer.fork; |
There was a problem hiding this comment.
Just wondering if the fork used here v/s in the non self build case below would be identical. We had issues in the past wrt the state not being advanced to the new fork and that causing signature verification errors.
I think this is safe, but would be good if @michaelsproul can confirm
There was a problem hiding this comment.
maybe we can name this file envelope_verification given that we do more than gossip verification here?
| match notify_execution_layer { | ||
| NotifyExecutionLayer::No if chain.config.optimistic_finalized_sync => { | ||
| let new_payload_request = Self::build_new_payload_request(&envelope, &block)?; | ||
| if let Err(e) = new_payload_request.perform_optimistic_sync_verifications() { |
There was a problem hiding this comment.
I think we should add a todo here to reassess if optimistic syncing works similarly to pre-gloas.
| // TODO(gloas) are these other logs important? | ||
| root = ?beacon_block_root, | ||
| // graffiti = block.body().graffiti().as_utf8_lossy(), | ||
| // proposer_index = block.proposer_index(), | ||
| // slot = %block.slot(), |
There was a problem hiding this comment.
slot might be useful too, otherwise i agree
3 participants
Implement payload envelope gossip verification and processing