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

[stateless_validation] Broadcast chunk endrosement to next 5 block producers #10449

Merged

Conversation

shreyan-gupta
Copy link
Contributor

In the current implementation, we only send the chunk endorsements to the next block producer. As discussed in this Zulip thread, it's possible for a chunk to be included in a block at a height different from that it was created for. This leads to the situation where this new block producer hasn't received the chunk endorsements for this chunk.

The solution here is to broadcast the chunk endorsement message to the next 5 block producers and hope that the chunk would get included in any one of them in cases of skipped blocks.

Note that this also means we are always going to use the chunk_validators as of the chunk.height_created and NOT chunk.height_included

Copy link
Contributor

@wacban wacban left a comment

Choose a reason for hiding this comment

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

LGTM

// h may be skipped and block producer at height h+1 picks up the chunk. We need to ensure
// that these later block producers also receive the chunk endorsement.
// Keeping a threshold of 5 block producers should be sufficient for most scenarios.
const NUM_NEXT_BLOCK_PRODUCERS_TO_SEND_CHUNK_ENDORSEMENT: u64 = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would consider making that configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe in a future PR. Do we have a stateless validation related config like we did for resharding?

self.epoch_manager.get_epoch_id_from_prev_block(chunk_header.prev_block_hash())?;
let block_producer =
self.epoch_manager.get_block_producer(&epoch_id, chunk_header.height_created())?;
// Verify that we are the block producer for this height.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could a malicious chunk producer try to OOM us by sending chunk endorsements for the next e100 heights?
No need to fix it here but if this is a risk please leave a TODO(stateless-validation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a very valid point! I've added a todo in code. We would definitely need to discuss this to see what are possible mitigations. I've tried listing out a few.

@shreyan-gupta shreyan-gupta added this pull request to the merge queue Jan 17, 2024
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (db20a66) 72.01% compared to head (30707f1) 71.98%.
Report is 2 commits behind head on master.

Files Patch % Lines
chain/client/src/chunk_validation.rs 81.25% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10449      +/-   ##
==========================================
- Coverage   72.01%   71.98%   -0.03%     
==========================================
  Files         718      718              
  Lines      144922   144919       -3     
  Branches   144922   144919       -3     
==========================================
- Hits       104360   104326      -34     
- Misses      35768    35804      +36     
+ Partials     4794     4789       -5     
Flag Coverage Δ
backward-compatibility 0.08% <0.00%> (ø)
db-migration 0.08% <0.00%> (ø)
genesis-check 1.26% <0.00%> (+<0.01%) ⬆️
integration-tests 36.77% <81.25%> (-0.10%) ⬇️
linux 71.45% <0.00%> (-0.07%) ⬇️
linux-nightly 71.55% <81.25%> (-0.02%) ⬇️
macos 55.43% <0.00%> (-0.05%) ⬇️
pytests 1.48% <0.00%> (+<0.01%) ⬆️
sanity-checks 1.27% <0.00%> (+<0.01%) ⬆️
unittests 68.09% <0.00%> (-0.03%) ⬇️
upgradability 0.13% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Merged via the queue into master with commit 0fcd21f Jan 17, 2024
25 of 26 checks passed
@shreyan-gupta shreyan-gupta deleted the shreyan/stateless_validation/broadcast_chunk_endorsement branch January 17, 2024 13:31
@shreyan-gupta shreyan-gupta added the A-stateless-validation Area: stateless validation label Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stateless-validation Area: stateless validation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants