Skip to content

Conversation

@jacinta-stacks
Copy link
Contributor

@jacinta-stacks jacinta-stacks commented Jan 15, 2026

As part of GSM testing, I saw quite a few instances where the mock signer was behind in receiving block proposals resulting in them rejecting a block proposal that arrived late that was already globally accepted. This is legacy behaviour and its possible it could be improved, but maybe its better to leave it as is? I have added a test to demonstrate the case at the very least to prove GSM did not introduce this behaviour:

SIgner 1 is behind and is receiving a bunch of block pre-commits, signatures, and eventually BlockPushed and NewBlock events for a particular block N that the Signer has not yet seen a BlockProposal for. As a result, all of these events are ignored. It only THEN receives block N's BlockProposal. However, because it does not have have any block_info for the corresponding signer_signature_hash in its database, it doesn't hit the "Received a block proposal for a block that is already globally accepted. Ignoring..." path. It instead treats the BlockProposal as one for an entirely new block and checks this "new proposal" against the chain tip which happens to actually already be Block N. It therefore fails when it hits the chain length enforcement tip.height() < block.header.chain_length, resulting in InvalidParentBlock.

I think its a bit odd that a signer that in theory knows that this block proposal is simply late, issues a rejection with InvalidParentBlock. However, its not as simple as adding the signer_signature_hash to the database and marking it as globally accepted when NewBlock arrives as we need the entire NakamotoBlock at this time and its not available (plus the chain could have advanced before we received the NewBlock event and it would still be an issue). It would need to query for the tenure_tip earlier which may be worthwhile but at the same time, it could be so late that the chain has advanced TWICE. in which case this tip check would fail anyway and we would still fail incheck_latest_block_in_tenure. This makes me inclined to leave everything as is and just have this test demonstrating what happens when an out of order block is received.

… in out of order proposal case

Signed-off-by: Jacinta Ferrant <jacinta@stackslabs.com>
brice-stacks
brice-stacks previously approved these changes Jan 15, 2026
Copy link
Contributor

@brice-stacks brice-stacks left a comment

Choose a reason for hiding this comment

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

I agree with your assessment to just leave it as is. Since the block is already globally accepted, what this signer does is inconsequential for the network, so we shouldn't add more complexity to try to make it do something better.

@codecov
Copy link

codecov bot commented Jan 15, 2026

Codecov Report

❌ Patch coverage is 36.75214% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.40%. Comparing base (6eef6c5) to head (be12cac).

Files with missing lines Patch % Lines
stacks-node/src/tests/signer/v0.rs 37.06% 73 Missing ⚠️
...tacks-node/src/tests/signer/commands/block_wait.rs 0.00% 1 Missing ⚠️

❌ Your project check has failed because the head coverage (77.40%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6816      +/-   ##
===========================================
+ Coverage    72.55%   77.40%   +4.85%     
===========================================
  Files          586      586              
  Lines       362553   362599      +46     
===========================================
+ Hits        263057   280681   +17624     
+ Misses       99496    81918   -17578     
Files with missing lines Coverage Δ
...tacks-node/src/tests/signer/commands/block_wait.rs 23.76% <0.00%> (ø)
stacks-node/src/tests/signer/v0.rs 14.40% <37.06%> (+4.31%) ⬆️

... and 256 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6eef6c5...be12cac. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

… into chore/add-block-proposal-out-or-order-test
… into chore/add-block-proposal-out-or-order-test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants