test: adversarial double-spend scenarios and two bug fixes#703
Merged
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete Summary: This PR adds comprehensive adversarial double-spend test coverage (2302 lines across 32 test scenarios) and fixes three bugs discovered during test-driven development:
Findings: ✅ All changes verified correct - no issues found The bug fixes are well-targeted, the tests are thorough and well-documented, and the code follows the project's epistemic honesty principles (AGENTS.md) by surfacing edge cases explicitly. Key strengths:
Technical details: 1. Data race fix (model/disk_parent_spends_map.go:49, 158, 317)
2. Batch double-spend detection (stores/utxo/sql/sql.go:2113-2146, 2379-2388)
3. Invalid block tx recovery (services/blockassembly/BlockAssembler.go:453-472, 525-528)
4. Test fix (16_same_tx_both_forks_test.go:142)
All changes align with AGENTS.md principles: minimal diffs, test-first verification, security-conscious (no injection risks), and clear risk documentation in comments. |
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-27 17:21 UTC |
sugh01
approved these changes
Apr 26, 2026
Add 32 adversarial test scenarios (64 test functions for Postgres+Aerospike) covering edge cases in the double-spend detection pipeline that were not previously exercised: - 12_subtree_blessing_attacks: pre-block subtree blessing, stale ConflictingNodes, competing miner subtrees, concurrent blessing races - 13_block_validation_attacks: duplicate txid detection, orphaned-branch parent rejection, deep ancestry acceptance, mixed block-ID chains - 14_reorg_state_machine: flip-flop reorgs, orphaned vs conflicting txs, frozen tx in conflict path, BFS correctness for sibling outputs, 3+ sequential conflicts on same UTXO, deep reorg with mixed tx fates - 15_complex_graph_attacks: cross-fork spending, conflicting-parent spending, multi-input partial spend rollback, intra-block double spend, 50-level deep BFS chain, diamond conflict graph, same-tx-both-forks - 16_same_tx_both_forks: fix wrong tx in block5a (code/comment mismatch) - 17_scale_and_cleanup: ConflictingChildren stale growth, long-term TTL consistency, locked-UTXO retry, zero-output conflict Bug fixes found by the new tests: - model/disk_parent_spends_map.go: fix DATA RACE — Stats() reads bytesWritten concurrently with the writer goroutine; change field to atomic.Int64. This race was causing ~85 test failures across the sequential suite. - services/blockassembly/BlockAssembler.go: reset() no longer skips invalid moveBack blocks when collecting txs to mark as unmined. For SQL/Postgres stores the async BlockMinedUnset job races with loadUnminedTransactions; processing invalid blocks inline ensures unmined_since is set before the iterator query runs. - test/sequentialtest/longest_chain/longest_chain_test.go: force a second ResetBlockAssembly() after WaitForBlock(block3) so that a fresh loadUnminedTransactions picks up tx1 from the invalidated block after BlockMinedUnset processing has completed.
Adapts new double-spend test files to the 6-arg ProcessBlock signature introduced by #711 ("pre-assign block ID in legacy path"). All callers in the legacy path use blockID=0, matching every existing test caller.
9d4a33e to
dbdc6d8
Compare
|
oskarszoon
requested changes
May 14, 2026
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
Has a merge conflict with main — please rebase before review.
…le-spend-coverage # Conflicts: # stores/blockchain/sql/GenesisHash_test.go
|
oskarszoon
approved these changes
May 28, 2026
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
Approve with cleanup — four test-quality items and a PR description correction.
P1 — unasserted invariants in three ConflictingChildren tests. requireConflictingChildrenCount (14_reorg_state_machine_test.go:216) and requireConflictingChildrenCountIn17 (17_scale_and_cleanup_test.go:191) are defined but never called. testFlipFlopReorg, testStaleConflictingChildrenAfterMultipleReorgs, and testMultipleSequentialConflictsOnSameUTXO all make explicit upper-bound claims on ConflictingChildren and assert nothing about it.
P1 — testLockedUTXORetryOnPhase2Window doesn't test retry. 17:236-266: SetLocked(true) → immediate SetLocked(false) → ProcessTransaction. Lock is gone before submission starts; retry path never exercised. Would pass with retry logic completely broken.
P1 — testConflictingChildrenListWith50Entries misnamed. 17:118-121: numCompeting = 10, all losers rejected by propagation, list never inspected. Fix structure to build the list or rename.
P1 — retry-setting reuse at BlockAssembler.go:655. Same retry config reused for two operations with different latency budgets. Split or document.
PR description. Commit body for ff963c5c2 claims a BlockAssembler.reset() fix; git show ff963c5c2 -- services/blockassembly/BlockAssembler.go returns empty. That work was reverted on main (1f154a8ac) and replaced by the watchdog in 24ae9e855. The longest_chain_test.go 10-line addition is a band-aid for the absent production fix.
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
3 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
Adds 32 adversarial test scenarios (64 test functions × Postgres/Aerospike) targeting the full double-spend detection pipeline — subtree blessing, block validation, reorg state machine, complex dependency graphs, and scale/cleanup. Tests were written red-first: run against the system, fix the failures found, verify all pass.
New test files:
12_subtree_blessing_attacks— pre-block subtree blessing window, staleConflictingNodes, competing miner subtrees, concurrent blessing races13_block_validation_attacks— duplicate txid detection, orphaned-branch parent rejection, deep ancestry, mixed block-ID chains14_reorg_state_machine— flip-flop reorgs (4×), orphaned-not-conflicting distinction, frozen tx in conflict path, BFS sibling output correctness, 3+ sequential conflicts on same UTXO, mixed-fate deep reorg15_complex_graph_attacks— cross-fork spending, conflicting-parent spending, multi-input partial spend rollback, intra-block double spend, 50-level deep BFS, diamond conflict graph, same-tx-both-forks17_scale_and_cleanup—ConflictingChildrenstale growth, long-term TTL consistency, locked-UTXO retry window, zero-output conflict BFS edge caseBug fixes found during testing:
DATA RACE in
DiskParentSpendsMap(model/disk_parent_spends_map.go)Stats()readbytesWrittenfrom the main goroutine while the dedicated writer goroutine wrote it concurrently. This was triggering the race detector and causing ~85 test failures across the sequential suite. Fixed by changing the field toatomic.Int64.Invalid-block txs missed by
loadUnminedTransactions(services/blockassembly/BlockAssembler.go)reset()explicitly skipped invalid moveBack blocks when collecting txs to mark as unmined, assuming BlockValidation's asyncBlockMinedUnsetjob would handle them. For SQL/Postgres stores the background job races withloadUnminedTransactions— the reset runs beforeunmined_sinceis set, so the txs are never found by the index query. Fixed by processing invalid blocks inline (same path as regular moveBack blocks).TestLongestChainPostgres/invalid block(test/sequentialtest/longest_chain/longest_chain_test.go)After
InvalidateBlock, the firstloadUnminedTransactionsmay still miss tx1 ifBlockMinedUnsetprocessing hasn't completed. A secondResetBlockAssembly()call afterWaitForBlock(block3)ensures a fresh scan picks up tx1 once the background job has finished.Also fixes:
16_same_tx_both_forks_test.go—block5awas built withtx1instead oftx1Conflicting(code/comment mismatch), making the "same tx in both forks" scenario untested.Test plan
make lintpasses