feat: tipset prechecks#6886
Merged
LesnyRumcajs merged 2 commits intomainfrom Apr 10, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a pre-fetch gossip block validation path: a new SeenBlockCache, a GossipBlockValidator performing header-only checks (including bad/seen caches), metrics for rejected gossip blocks, and integration into the chain follower to skip fetching rejected blocks. Changes
Sequence DiagramsequenceDiagram
participant Gossipsub as Gossipsub Network
participant ChainFollower as Chain Follower
participant Validator as GossipBlockValidator
participant Caches as SeenBlockCache + BadBlockCache
participant Metrics as Metrics Counter
Gossipsub->>ChainFollower: PubsubMessage::Block(header)
ChainFollower->>Validator: validate_pre_fetch(header, ...)
Validator->>Caches: SeenBlockCache.test_and_insert(cid)
Caches-->>Validator: seen? (bool)
alt duplicate
Validator-->>ChainFollower: Err(DuplicateCID)
else not duplicate
Validator->>Caches: BadBlockCache.get(cid)
Caches-->>Validator: is_bad? (bool)
alt bad block
Validator-->>ChainFollower: Err(BadBlock)
else
Validator->>Validator: epoch/finality/timestamp/election/message checks
alt validation fails
Validator-->>ChainFollower: Err(reason)
else validation succeeds
Validator-->>ChainFollower: Ok
end
end
end
alt Err(reason)
ChainFollower->>Metrics: Increment GOSSIP_BLOCK_REJECTED_TOTAL{reason}
ChainFollower->>ChainFollower: Skip fetch/persist
else Ok
ChainFollower->>ChainFollower: Fetch full tipset and persist
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
LesnyRumcajs
requested review from
akaladarshi and
sudo-shashank
and removed request for
a team
LesnyRumcajs
force-pushed
the
tipset-prechecks
branch
from
58fc490 to
6e6800a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/chain_sync/validation.rs (1)
196-223: Document the new public helpers.
GossipBlockRejectReason::labelandGossipBlockValidator::neware public entry points now, so they should have rustdoc while this API surface is still small.As per coding guidelines, "Document public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain_sync/validation.rs` around lines 196 - 223, Add doc comments for the newly public helpers: document the purpose and behavior of GossipBlockRejectReason::label (what each returned &'static str represents and when to use it) and GossipBlockValidator::new (what a GossipBlockValidator validates and that it only uses header+CIDs for pre-validation). Place /// rustdoc comments immediately above the impl or method signatures so these public entry points are discoverable in generated docs; mention any important invariants or expected inputs for GossipBlockValidator.src/chain_sync/bad_block_cache.rs (1)
62-67: Add rustdoc toSeenBlockCache::new.The type itself is documented, but its public constructor is not.
As per coding guidelines, "Document public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain_sync/bad_block_cache.rs` around lines 62 - 67, Add a rustdoc comment to the public constructor SeenBlockCache::new describing what kind of cache it creates and its purpose (a size-tracking LRU used for seen gossip blocks), document the parameter `cap: NonZeroUsize` as the maximum number of entries, and document the return value as a new SeenBlockCache wrapping a SizeTrackingLruCache initialized with the metric name "seen_gossip_block"; keep the comment concise and follow existing doc style for public APIs.src/chain_sync/metrics.rs (1)
61-64: DocumentGossipRejectReasonLabel.This is a new public type, so it should carry a short rustdoc comment describing which metric it labels.
As per coding guidelines, "Document public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain_sync/metrics.rs` around lines 61 - 64, Add a short Rust doc comment above the public struct GossipRejectReasonLabel that explains this type is used to label the metric which tracks gossip message rejections (i.e., it carries the rejection reason string for the gossip rejection metric), so consumers know what metric it labels and what the &'static str field represents; update the doc to mention the field `reason` and its purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chain_sync/validation.rs`:
- Around line 236-255: The code currently inserts the block CID into
SeenBlockCache via check_duplicate before running transient validations
(validate_epoch_range, validate_timestamp, etc.), causing
early-rejected-but-later-valid blocks to be marked duplicate; fix by deferring
the check_duplicate call (and any test_and_insert) until after transient checks
succeed — i.e., move the call to check_duplicate(cid, seen_block_cache) to after
validate_epoch_range, validate_timestamp, validate_election_proof,
validate_signature_present, and validate_message_count so only fully-passed
blocks are cached; also add a regression test that submits a block once yielding
EpochTooFarAhead and then again later that is accepted, asserting it was not
suppressed as DuplicateBlock.
---
Nitpick comments:
In `@src/chain_sync/bad_block_cache.rs`:
- Around line 62-67: Add a rustdoc comment to the public constructor
SeenBlockCache::new describing what kind of cache it creates and its purpose (a
size-tracking LRU used for seen gossip blocks), document the parameter `cap:
NonZeroUsize` as the maximum number of entries, and document the return value as
a new SeenBlockCache wrapping a SizeTrackingLruCache initialized with the metric
name "seen_gossip_block"; keep the comment concise and follow existing doc style
for public APIs.
In `@src/chain_sync/metrics.rs`:
- Around line 61-64: Add a short Rust doc comment above the public struct
GossipRejectReasonLabel that explains this type is used to label the metric
which tracks gossip message rejections (i.e., it carries the rejection reason
string for the gossip rejection metric), so consumers know what metric it labels
and what the &'static str field represents; update the doc to mention the field
`reason` and its purpose.
In `@src/chain_sync/validation.rs`:
- Around line 196-223: Add doc comments for the newly public helpers: document
the purpose and behavior of GossipBlockRejectReason::label (what each returned
&'static str represents and when to use it) and GossipBlockValidator::new (what
a GossipBlockValidator validates and that it only uses header+CIDs for
pre-validation). Place /// rustdoc comments immediately above the impl or method
signatures so these public entry points are discoverable in generated docs;
mention any important invariants or expected inputs for GossipBlockValidator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f8fc99aa-e340-45dd-b594-e6769237cada
📒 Files selected for processing (4)
src/chain_sync/bad_block_cache.rssrc/chain_sync/chain_follower.rssrc/chain_sync/metrics.rssrc/chain_sync/validation.rs
LesnyRumcajs
force-pushed
the
tipset-prechecks
branch
from
6e6800a to
930c9bc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/chain_sync/validation.rs (1)
196-223: Document the new public helpers or narrow their visibility.
GossipBlockRejectReason::label()andGossipBlockValidator::new()are now public API surface but still undocumented. If they are internal-only,pub(crate)would also keep the surface smaller.As per coding guidelines "Document public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain_sync/validation.rs` around lines 196 - 223, GossipBlockRejectReason::label() and GossipBlockValidator::new() were made public without docs; either add concise doc-comments describing their purpose and usage (e.g., doc on GossipBlockRejectReason::label explaining it returns a stable metric label for rejection reasons, and doc on GossipBlockValidator::new describing it constructs a validator for a gossip block) or, if they are intended to be internal, reduce their visibility to pub(crate) to avoid expanding the public API; update the signatures accordingly and ensure the chosen approach is applied to both symbols consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chain_sync/validation.rs`:
- Around line 243-245: The code currently marks a CID seen inside
Self::check_duplicate (using seen_block_cache) before the subsequent
get_full_tipset() + persist path runs, causing transient fetch failures to cause
later rebroadcasts to be rejected as DuplicateBlock; change the behavior so
check_duplicate only checks membership (do not insert), and perform the insert
into seen_block_cache only after get_full_tipset() returns successfully (the
caller in chain_follower that invokes get_full_tipset should add the insert), or
alternatively add an explicit rollback removal of the CID from seen_block_cache
if get_full_tipset()/persist fails; update/check the functions check_duplicate
and the chain_follower code path that calls get_full_tipset to implement this
move-or-rollback change.
---
Nitpick comments:
In `@src/chain_sync/validation.rs`:
- Around line 196-223: GossipBlockRejectReason::label() and
GossipBlockValidator::new() were made public without docs; either add concise
doc-comments describing their purpose and usage (e.g., doc on
GossipBlockRejectReason::label explaining it returns a stable metric label for
rejection reasons, and doc on GossipBlockValidator::new describing it constructs
a validator for a gossip block) or, if they are intended to be internal, reduce
their visibility to pub(crate) to avoid expanding the public API; update the
signatures accordingly and ensure the chosen approach is applied to both symbols
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bd29c923-2196-4776-b8e1-93d0709ad7bd
📒 Files selected for processing (4)
src/chain_sync/bad_block_cache.rssrc/chain_sync/chain_follower.rssrc/chain_sync/metrics.rssrc/chain_sync/validation.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/chain_sync/metrics.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
hanabi1224
reviewed
Apr 10, 2026
hanabi1224
reviewed
Apr 10, 2026
hanabi1224
previously approved these changes
Apr 10, 2026
hanabi1224
approved these changes
Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #1914
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit