-
Notifications
You must be signed in to change notification settings - Fork 907
Skip column gossip verification logic during block production #7973
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?
Skip column gossip verification logic during block production #7973
Conversation
…ip-column-verification-during-block-production
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
@eserilev the sync job is failing, doesn't feel like your change would trigger this, but interestingly both fullnode and supernode are failing to sync, and stuck at |
Err(warp_utils::reject::custom_bad_request(format!("{e:?}"))) | ||
} | ||
} | ||
GossipVerifiedDataColumn::new_for_block_publishing(data_column_sidecar) |
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 we still need to do the following to avoid trying to publish & process them again:
- check the availability cache and observed cache to make sure we exclude columns that we have seen
- mark the column as seen in the observe cache
lighthouse/beacon_node/beacon_chain/src/data_column_verification.rs
Lines 450 to 466 in 9d2f55a
verify_is_first_sidecar(chain, &data_column)?; | |
// Check if the data column is already in the DA checker cache. This happens when data columns | |
// are made available through the `engine_getBlobs` method. If it exists in the cache, we know | |
// it has already passed the gossip checks, even though this particular instance hasn't been | |
// seen / published on the gossip network yet (passed the `verify_is_first_sidecar` check above). | |
// In this case, we should accept it for gossip propagation. | |
if chain | |
.data_availability_checker | |
.is_data_column_cached(&data_column.block_root(), &data_column) | |
{ | |
// Observe this data column so we don't process it again. | |
if O::observe() { | |
observe_gossip_data_column(&data_column, chain)?; | |
} | |
return Err(GossipDataColumnError::PriorKnownUnpublished); | |
} |
We've seen a race condition where the proposer node gets the columns from peers before the proposer publishes - a scenario where other nodes was able to fetch blobs from the EL and compute them faster than the proposer node.
…ip-column-verification-during-block-production
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.
Just added a minor comment but i think we're close to merging this.
// are made available through the `engine_getBlobs` method. If it exists in the cache, we know | ||
// it has already passed the gossip checks, even though this particular instance hasn't been | ||
// seen / published on the gossip network yet (passed the `verify_is_first_sidecar` check above). | ||
// In this case, we should accept it for gossip propagation. |
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 we should also check for is_first_sidecar
here, and we can avoid acquiring the cache write lock below (in the observe
fn) if not needed - the cache read & write happens in burst around this time and it's time sensitive so it'd be great to avoid this.
Issue Addressed
#7950
Proposed Changes
Skip column gossip verification logic during block production as its redundant and potentially computationally expensive.