-
Notifications
You must be signed in to change notification settings - Fork 877
Optimize GetBlobsV2 Verification #7553
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
base: unstable
Are you sure you want to change the base?
Conversation
…verification is skipped
verify_proposer_and_signature(&data_column, &parent_block, chain)?; | ||
|
||
match check_block_header { | ||
CheckBlockHeader::Yes => verify_proposer_and_signature(&data_column, &parent_block, chain)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth considering skipping some more checks:
verify_parent_block_and_finalized_descendant
: this acquires a fork choice read lock - so it could delay verification when the node is overwhelmedverify_column_inclusion_proof
isn't necessary for sidecars constructed internally, because we're verifying proofs we just constructed (on the critical path as well!) - although this isn't part of the header, so skipping it whenCheckBlockHeader::No
may be confusing.chain.observed_slashable.write()
: it's likely a quick operation but i think we'd want to avoid acquiring locks unnecessarily - in this case it's observing the data from the header
I realise I may have not explained the rationale in the issues clearly - the main thing is we can avoid verifying some properties of the DataColumnSidecar
if the sidecar is constructed internally by the node, e.g. block proposal, fetch blobs etc - and in these case we can skip:
- the block header that we've already verified, and have just been cloned over to the sidecar
- the kzg inclusion proof that we've just constructed
But for any DataColumnSidecar
s received over gossip, we'd want to fully verify them.
For reference, the function to construct DataColumnSidecar
is below, and and the only untrusted input is Blobs
and Proofs
:
lighthouse/beacon_node/beacon_chain/src/kzg_utils.rs
Lines 166 to 172 in 9779b4b
pub fn blobs_to_data_column_sidecars<E: EthSpec>( | |
blobs: &[&Blob<E>], | |
cell_proofs: Vec<KzgProof>, | |
block: &SignedBeaconBlock<E>, | |
kzg: &Kzg, | |
spec: &ChainSpec, | |
) -> Result<DataColumnSidecarList<E>, DataColumnSidecarError> { |
let gossip_verified_column = GossipVerifiedDataColumn::new( | ||
data_column_sidecar, | ||
subnet.into(), | ||
CheckBlockHeader::Yes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DataColumnSidecar
is constructed in the same function internally by the node, so we just do light verification here i believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this looks good, thanks @eserilev !
I've added some questions regarding if we could skip more checks, let me know what you think!
@michaelsproul we briefly talked about this before, so would be great to get your input too 🙏
} | ||
ConstructedInternally::Yes => (), | ||
}; | ||
|
||
let kzg = &chain.kzg; | ||
let kzg_verified_data_column = verify_kzg_for_data_column(data_column.clone(), kzg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be overthinking, but I'm also tempted to include observe_slashable
(without changing the ordering), because it acquires a write lock, and usually data columns arrives in burst (128 simultaneously), and the write lock can only be acquired by one thread at a time, so this write could potentially slow things down even though it's a very fast operation.
chain
.observed_slashable
.write()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth writing some bench tests for gossip verification (in a later PR) - given that 1/ gossip verification time is critical to both the network and the performance of the node; 2/ the volume of verification is significantly higher now with data columns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue for benchmark #7576
Issue Addressed
#7545
Proposed Changes
Add a verification mode that skips block proposer and block signature checks.