Skip to content

fix: signature order check#7016

Draft
brice-stacks wants to merge 3 commits intostacks-network:developfrom
brice-stacks:fix/out-of-order-signatures
Draft

fix: signature order check#7016
brice-stacks wants to merge 3 commits intostacks-network:developfrom
brice-stacks:fix/out-of-order-signatures

Conversation

@brice-stacks
Copy link
Copy Markdown
Contributor

The last_index variable in verify_signer_signatures was only updated on the first loop iteration (inside an else branch), so subsequent signatures were checked against the first signer's index rather than the previous one. This allowed out-of-order sequences like [0, 2, 1] to pass validation. Move the assignment outside the conditional so it updates every iteration, and add a regression test.

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

The last_index variable in verify_signer_signatures was only updated on
the first loop iteration (inside an else branch), so subsequent
signatures were checked against the first signer's index rather than
the previous one. This allowed out-of-order sequences like [0, 2, 1] to
pass validation. Move the assignment outside the conditional so it
updates every iteration, and add a regression test.
@BowTiedRadone
Copy link
Copy Markdown
Contributor

@brice-stacks This fixes #6907.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 24, 2026

Pull Request Test Coverage Report for Build 23526303949

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 158 unchanged lines in 34 files lost coverage.
  • Overall coverage decreased (-0.02%) to 85.692%

Files with Coverage Reduction New Missed Lines %
libsigner/src/session.rs 1 65.41%
stacks-common/src/deps_common/bitcoin/util/hash.rs 1 81.82%
stacks-common/src/util/secp256r1.rs 1 90.24%
stackslib/src/burnchains/mod.rs 1 83.9%
stackslib/src/chainstate/stacks/index/node.rs 1 86.61%
stackslib/src/chainstate/stacks/miner.rs 1 83.48%
stackslib/src/clarity_vm/clarity.rs 1 93.7%
stackslib/src/net/api/poststackerdbchunk.rs 1 82.49%
stackslib/src/net/download/nakamoto/download_state_machine.rs 1 90.57%
stacks-signer/src/monitoring/server.rs 1 57.78%
Totals Coverage Status
Change from base Build 23400582460: -0.02%
Covered Lines: 186535
Relevant Lines: 217681

💛 - Coveralls

Copy link
Copy Markdown
Contributor

@federico-stacks federico-stacks left a comment

Choose a reason for hiding this comment

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

lgtm!

just a small question about the test configuration

@brice-stacks
Copy link
Copy Markdown
Contributor Author

NOTE: Don't merge this until issue of old blocks is resolved.

@brice-stacks brice-stacks marked this pull request as draft March 25, 2026 14:44
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.

4 participants