Parallel gossip verification and block unblinding #4647
Conversation
michaelsproul
added
ready-for-review
|
|
||
| // Complete gossip verification and publication to the relay in parallel. | ||
| // This avoids tripping the gossip duplicate filter. | ||
| let block_to_verify = Arc::new(block.clone()); |
There was a problem hiding this comment.
This clone of the blinded block is a bit unfortunate but I think unavoidable as we need an Arc block for most functions, but reconstruct_payload needs a full block. Because the block is blinded, cloning it should be quite fast!
|
I'm going to deploy this on Goerli and see if it solves the issue. If it doesn't, I'll consider added a 202 code for the duplicates as well. |
| task_spawner | ||
| .clone() | ||
| .spawn_async_with_rejection(Priority::P0, async move { | ||
| let block = SignedBlindedBeaconBlock::<T::EthSpec>::from_ssz_bytes( |
There was a problem hiding this comment.
Ah nice, the type alias is better - there's another reference in the v1 endpoint if you don't mind updating that one as well.
| let provenanced_gossip_verified_block = match full_block { | ||
| ProvenancedBlock::Local(block, phantom) => { | ||
| ProvenancedBlock::Local(gossip_verified_blinded_block.unblind(block), phantom) | ||
| } | ||
| ProvenancedBlock::Builder(block, phantom) => { | ||
| ProvenancedBlock::Builder(gossip_verified_blinded_block.unblind(block), phantom) | ||
| } | ||
| }; |
There was a problem hiding this comment.
I think it might be worth adding a comment on why we're passing the gossip verified block (so that it doesn't verify again after publishing block) because the code would still compile and run fine without this - just to prevent it from getting removed by accident. I wasn't familiar with gossip verification and had to navigate around to understand why we needed this.
|
Looks great! 👍 I only have some minor nitpicks / comments! |
|
I have second thoughts about this now, because by making gossip verification succeed it means we will try to process the block twice: the one from gossip and the one from the HTTP API. This is likely to mess with the snapshot cache (only one of the blocks will get the snapshot) and wastes processing time. I'm starting to think we should just hack the status code to a 202 and call it a day... |
|
Binning this in favour of #4655 |
Issue Addressed
Closes #4473
Replaces #4643
Proposed Changes
Payload: AbstractExecPayload<E>to enable the checking of blinded blocks.task_spawner(a small omission caused by conflicts between several recently merged PRs).