test: fix CI fixtures after transaction re-extension (release/v0.15) - #1722
Conversation
Backport of ac2833f with identical test and documentation changes.
|
🤖 Claude Code Review Status: Complete Current Review:
Minor note (non-blocking): |
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved approval-blocking issues were identified.
Pull request overview
Backports test and documentation fixes for mandatory transaction re-extension without changing production code.
Changes:
- Isolates concurrent transaction submissions.
- Makes validator fee fixtures deterministic.
- Strengthens reassignment maturity tests.
- Documents replacement-script limitations.
File summaries
| File | Description |
|---|---|
test/e2e/daemon/ready/reassign_test.go |
Tests maturity and stored-owner behavior. |
services/validator/utxo_commitment_ambiguity_test.go |
Disables fees for the zero-value control. |
services/propagation/Server_test.go |
Uses independent decoded transactions concurrently. |
docs/topics/services/alert.md |
Documents reassignment limitations. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
oskarszoon
left a comment
There was a problem hiding this comment.
Tests and docs only, mechanics verified: the extended-bytes round-trip is sound (go-bt
ReadFrom detects the 0xEF marker, so PreviousTxScript/PreviousTxSatoshis
survive NewTxFromBytes), settings are fresh per subtest so the fee override can't
leak, and the per-goroutine decode fixes a real shared-pointer race. Two things before
this lands.
Test name now says the opposite of what it asserts — test/e2e/daemon/ready/reassign_test.go:24
TestShouldAllowReassign asserts reassignment to a different owner is rejected before
maturity, after maturity, and leaves the output unspent. The only surviving success
path reassigns to the stored owner, which changes no ownership. Rename it to what it
pins — TestReassignEnforcesMaturityAndRejectsUnstoredScript or similar — otherwise
the next person greps for reassignment coverage and concludes the feature works.
The regression needs more than a docs paragraph
Confirmed in both stores: stores/utxo/sql/alert_system.go:179-186 updates
utxo_hash/frozen/spendableIn only, and
stores/utxo/aerospike/alert_system.go:245-251 passes just the two hashes and heights
to the Lua UDF. Neither persists the replacement script, so alert-service reassignment
to a new owner is non-functional on release/v0.15 after the security fix.
No tracking issue exists. This is the confiscation path — it needs an issue and a
v0.15.x release-note entry, not only docs/topics/services/alert.md:141.
Smaller
services/validator/utxo_commitment_ambiguity_test.go:184-187— reaching in to set
MinMiningTxFeeand rebuildtxValidatorafter construction would be cleaner as an
option onnewAmbiguityValidator. Safe as written.reassign_test.go:136vs:175— you assertSpendingData == nilafter Charles's
rejection but not after Bob's early rejection, then re-submit that same transaction
at :178. One more assertion makes the laterrequire.NoErrorunambiguous.
Unrelated, found while checking the above: sql/alert_system.go:174-178 honours
tSettings.UtxoStore.ReAssignedUtxoSpendableAfterBlocks but
aerospike/alert_system.go:249 hardcodes the package constant (1000). Production runs
Aerospike, so that setting is silently a no-op there.
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-09-11 14:38 UTC |
|
Follow-up on the reviewed-and-approved state here, from reviewing the identical change on
The risk is an operator reading that paragraph as "the coin doesn't move yet", running a court-ordered confiscation, and permanently burning the output. Recovery — re-freeze and reassign back to the original hash — is undocumented and diverges by backend: SQL's freeze ignores the hash ( Two other sentences in the same paragraph:
There is no tracking issue for the regression — Full detail on #1721. |
Backport of #1721 to
release/v0.15. The test and documentation changes are identical to main, with no branch-specific adjustments.Mandatory transaction re-extension exposed three test assumptions: concurrent propagation submissions shared a mutable transaction, the zero-value script fixture depended on the configured minimum fee, and the reassignment smoke test trusted a replacement script that was never stored.
This change gives each concurrent submission its own decoded transaction while retaining the duplicate-submission assertion, explicitly disables the minimum fee only for the unspendable-script control, and checks reassignment maturity with a spend signed for the stored owner. The smoke test also verifies that an unstored replacement script remains rejected after maturity and leaves the output unspent.
Reassignment limitation:
ReAssignUTXOchanges the commitment and maturity state but does not persist the replacement locking script. Re-extension therefore retrieves the original script. Supporting a different owner requires an authoritative replacement-script source and corresponding validation support. This limitation is documented in the alert-service guide; production code is unchanged.Validation on this branch:
SETTINGS_CONTEXT=test, the race detector, andtesttxmetacache, including real Aerospike/PostgreSQL containers.go vetandstaticcheckfor the three affected packages.The zero-value fixture's
insufficient-feefailure was reproduced with a nonzero minimum fee before verifying the fix.A broader local run also hit an HTTP startup failure on main and a batch-test failure on release. Both full propagation packages and three isolated runs of the release batch test passed on rerun. These tests are unchanged by this patch.