Skip to content

fix: avoid livelock after mining delay - #7489

Open
brice-stacks wants to merge 2 commits into
stacks-network:mainfrom
brice-stacks:feat/replication-void-fix
Open

fix: avoid livelock after mining delay#7489
brice-stacks wants to merge 2 commits into
stacks-network:mainfrom
brice-stacks:feat/replication-void-fix

Conversation

@brice-stacks

Copy link
Copy Markdown
Contributor

If, somehow, we arrive in a scenario where the miner's proposals are not reaching signers (or at least not 70% of them) for block_proposal_max_age_secs, then finally, the proposals arrive at the signers, they would previously silently drop this proposal, neither approving or rejecting it. The miner, continuing to wait for approval or rejection would be permanently stuck in the propose_block loop, only ever reproposing the same block. This commit changes the signer behavior so that instead of silently ignoring the block, the reject it with a new reason, ProposalTooOld, which, when it receives >= 30% of these rejections, will trigger the miner to exit that loop and mine a new block.

Checklist

  • Test coverage for new or modified code paths
  • For new Clarity features or consensus changes, add property tests (see docs/property-testing.md)
  • Changelog fragment(s) or "no changelog" label added (see changelog.d/README.md)
  • Required documentation changes (e.g., rpc/openapi.yaml for RPC endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo

If, somehow, we arrive in a scenario where the miner's proposals are not
reaching signers (or at least not 70% of them) for
`block_proposal_max_age_secs`, then finally, the proposals arrive at the
signers, they would previously silently drop this proposal, neither
approving or rejecting it. The miner, continuing to wait for approval or
rejection would be permanently stuck in the `propose_block` loop, only
ever reproposing the same block. This commit changes the signer behavior
so that instead of silently ignoring the block, the reject it with a new
reason, `ProposalTooOld`, which, when it receives >= 30% of these
rejections, will trigger the miner to exit that loop and mine a new
block.
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 31036439263

Coverage increased (+0.06%) to 86.642%

Details

  • Coverage increased (+0.06%) from the base build.
  • Patch coverage: 1 uncovered change across 1 file (9 of 10 lines covered, 90.0%).
  • 387 coverage regressions across 36 files.

Uncovered Changes

File Changed Covered %
libsigner/src/v0/messages.rs 6 5 83.33%
Total (2 files) 10 9 90.0%

Coverage Regressions

387 previously-covered lines in 36 files lost coverage.

Top 10 Files by Coverage Loss Lines Losing Coverage Coverage
stackslib/src/chainstate/burn/db/sortdb.rs 229 90.46%
stackslib/src/burnchains/db.rs 33 78.24%
stacks-signer/src/client/stacks_client.rs 27 86.76%
stackslib/src/chainstate/stacks/db/snapshot/blocks.rs 9 96.04%
stacks-signer/src/monitoring/mod.rs 9 77.08%
stackslib/src/chainstate/stacks/db/snapshot/burnchain.rs 7 94.77%
stackslib/src/burnchains/bitcoin/network.rs 5 79.8%
stackslib/src/chainstate/nakamoto/mod.rs 5 84.77%
stackslib/src/chainstate/stacks/db/snapshot/common.rs 5 96.75%
stackslib/src/chainstate/stacks/db/snapshot/index.rs 5 97.62%

Coverage Stats

Coverage Status
Relevant Lines: 232774
Covered Lines: 201681
Line Coverage: 86.64%
Coverage Strength: 19357119.01 hits per line

💛 - Coveralls

Comment thread stacks-signer/src/v0/signer.rs
Comment thread stacks-signer/src/v0/signer.rs

@federico-stacks federico-stacks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Implementation looks fine. There is a conflict that need to be addressed.

I also noticed 3 things that are pre-existing behaviour, so flagging for possible follow-ups/investigation:

  • BlockInfo::valid is set at pre-commit, one phase before a signature is owed. mark_pre_committed() sets valid = Some(true), and determine_response keys on valid alone. It therefore can't distinguish "I intend to sign" (PreCommitted) from "I signed" (LocallyAccepted), and returns a full acceptance for both. Net effect: a re-proposal makes a pre-committed signer release its signature even though the 70% pre-commit threshold was never reached. Maybe not a big issue considering it is only reachable when pre-commits aren't circulating to 70% while the miner is already re-proposing. A check on state alongside valid should close it.

  • should_reevaluate_block has a predicate name but broadcasts.. It sounds a bit measleading. A better naming (respond_to_known_proposal?!), or even producing a return type (Responded / Reevaluate) or even producing return type + splitting the re-evaluation behavior and the broadcasting could improve readability and maintainability.

  • determine_response drops failed_txid when re-sending a rejection. It substitutes RejectReason::RejectedInPriorRound, and BlockRejection::new passes None for failed_txid. The miner's exclusion logic requires both a failed_txid and RejectCode::ValidationFailed(BadTransaction|ProblematicTransaction), so a re-sent rejection contributes reject weight but no txid (probably relevant in case the miner didn't receive the previous rejection).

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.

3 participants