-
Notifications
You must be signed in to change notification settings - Fork 222
HIGH-003: Missing-data nonce-gap requests are unbounded before proposal validity is established #7693
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: ai-audit-findings
Are you sure you want to change the base?
Conversation
…al validity is established
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ai-audit-findings #7693 +/- ##
=====================================================
- Coverage 77.53% 77.53% -0.01%
=====================================================
Files 878 878
Lines 122173 122211 +38
=====================================================
+ Hits 94728 94753 +25
- Misses 21135 21143 +8
- Partials 6310 6315 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
process/block/metablockProposal.go
Outdated
| return execResult, nil | ||
| } | ||
|
|
||
| func (mp *metaProcessor) checkShardInfoProposalNonceGap(metaHeader data.MetaHeaderHandler) error { |
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 check needs to be enforced not only on shardInfo by metachain, but also for metablock and its last executed result notarization, or on shard chain between the block header and its last executed result notarization.
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.
pushed
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.
Pull request overview
Adds a configurable upper bound for the shard-info proposal nonce gap to prevent unbounded “nonce-gap” missing-data requests before a metablock proposal is fully validated (HIGH-003).
Changes:
- Introduces
MaxShardInfoProposalNonceGapconfig/plumbing (config struct, TOML, factory wiring, test configs). - Adds nonce-gap validation for metablock proposal creation and verification, plus a new
ErrNonceGapTooLarge. - Extends unit tests and stubs to cover the new behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
testscommon/headerHandlerStub.go |
Implements GetShardInfoHandlers() for tests via a callback + safe default. |
testscommon/generalConfig.go |
Sets a default MaxShardInfoProposalNonceGap in the shared test config. |
testscommon/components/configs.go |
Sets MaxShardInfoProposalNonceGap in component test config. |
process/errors.go |
Adds ErrNonceGapTooLarge error sentinel. |
process/block/metablock_test.go |
Updates meta processor test args with MaxShardInfoProposalNonceGap. |
process/block/metablockProposal_test.go |
Adds coverage for nonce-gap validation on propose/verify paths. |
process/block/metablockProposal.go |
Adds nonce-gap validation calls and implements checkShardInfoProposalNonceGap. |
process/block/metablock.go |
Stores max-gap in metaProcessor and applies a default when unset. |
process/block/argProcessor.go |
Adds MaxShardInfoProposalNonceGap to ArgMetaProcessor. |
factory/processing/blockProcessorCreator.go |
Wires config value into ArgMetaProcessor. |
config/config.go |
Adds MaxShardInfoProposalNonceGap to GeneralSettingsConfig. |
cmd/node/config/config.toml |
Documents/exposes MaxShardInfoProposalNonceGap in node config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
process/block/metablockProposal.go
Outdated
| err = mp.checkShardInfoProposalNonceGap(metaHeader) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
Copilot
AI
Feb 10, 2026
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 nonce-gap validation is invoked in CreateNewHeaderProposal(), but at this stage the metablock’s ShardInfo/ShardInfoProposal handlers are typically not populated yet (they get set later during CreateBlockProposal). This means the "proposal"-side validation may be ineffective and large nonce gaps could still be produced locally. Consider moving (or additionally adding) this check after shard info/proposals are set on the header in the proposal-building flow, keeping VerifyBlockProposal as the inbound guard.
| err = mp.checkShardInfoProposalNonceGap(metaHeader) | |
| if err != nil { | |
| return nil, err | |
| } |
| // No finalized data for this shard, skip validation | ||
| continue | ||
| } | ||
|
|
Copilot
AI
Feb 10, 2026
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.
checkShardInfoProposalNonceGap() computes nonceGap := maxProposedNonce - lastFinalizedNonce using uint64. If a block contains a proposed nonce lower than the finalized nonce for the same shard, this will underflow and produce a huge gap value, leading to a confusing error and potentially rejecting cases that should be treated as "no gap" or as a distinct validation error. Add an explicit comparison (e.g., handle maxProposedNonce <= lastFinalizedNonce) before subtracting.
| // Handle cases where proposed nonce is not ahead of the finalized nonce | |
| if maxProposedNonce <= lastFinalizedNonce { | |
| if maxProposedNonce < lastFinalizedNonce { | |
| return fmt.Errorf("%w: shard %d has proposed nonce %d lower than finalized nonce %d", | |
| process.ErrNonceGapTooLarge, | |
| shardID, | |
| maxProposedNonce, | |
| lastFinalizedNonce) | |
| } | |
| // maxProposedNonce == lastFinalizedNonce: no gap to check | |
| continue | |
| } |
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.
fixed
cmd/node/config/config.toml
Outdated
| # Make sure that this is greater than the unbonding period! | ||
| SetGuardianEpochsDelay = 2 # TODO: for mainnet should be 20, 2 is just for testing | ||
|
|
||
| # MaxShardInfoProposalNonceGap defines the maximum gap allowed between highest finalized shard nonce and lowest proposed shard nonce |
Copilot
AI
Feb 10, 2026
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 TOML comment says the gap is measured between the "highest finalized" and the "lowest proposed" shard nonce, but the implementation validates against the highest proposed nonce per shard. Please update this comment to match the actual behavior to avoid operator/config confusion.
| # MaxShardInfoProposalNonceGap defines the maximum gap allowed between highest finalized shard nonce and lowest proposed shard nonce | |
| # MaxShardInfoProposalNonceGap defines the maximum gap allowed between the highest finalized shard nonce | |
| # and the highest proposed shard nonce per shard |
…chain-go into ai-findings-high-003 # Conflicts: # factory/processing/blockProcessorCreator.go # process/block/argProcessor.go # process/block/baseProcess.go # process/block/baseProcess_test.go # process/block/metablock_test.go # process/errors.go
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?