fix: document reassignment lockout and repair re-extension CI tests - #1721
Conversation
|
🤖 Claude Code Review Status: Complete Current Review:
History:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Mandatory transaction re-extension therefore still retrieves the original script | ||
| from the UTXO store. A spend signed only for the replacement owner is rejected even | ||
| after maturity; supplying the replacement script in extended transaction bytes | ||
| cannot authorize it. Supporting a different owner requires an authoritative source |
There was a problem hiding this comment.
[Major] The documentation and tests here accurately capture a real behavioral regression, not just a pre-existing gap. Before mandatory re-extension, the old reassign_test.go asserted the opposite: after maturity, the new owner (Charles) could spend the reassigned UTXO. That was the point of the alert-service confiscation flow — reassign a frozen UTXO to a new authority who then spends it after the maturity delay.
With re-extension unconditionally reloading the stored (original) locking script, reassignment-to-a-different-owner is now effectively inert: the confiscated output becomes permanently unspendable by the intended new owner. Documenting it as a "current limitation" is the right call for a test-fix PR, and the same-owner path still exercises the maturity gate — good.
Non-blocking, but worth confirming: is there a tracking issue for restoring new-owner reassignment (the authoritative replacement-script source mentioned here)? Otherwise a security/confiscation feature is silently non-functional with only a docs note recording it.
There was a problem hiding this comment.
Resolved: the PR now links issue 1725 throughout, which explicitly tracks restoration, recovery, backend parity and operator guidance for ownership-changing reassignment. The previously-silent non-functional confiscation feature is now recorded with a tracking issue. Verified the docs claims against code — SQL honors the configurable maturity delay (sql/alert_system.go:172-176) while Aerospike hardcodes the constant (aerospike/alert_system.go:250), and neither store persists the replacement locking script.
There was a problem hiding this comment.
🟢 Approval recommended
The reviewed changes address the affected test assumptions with no unresolved blocking issues.
Pull request overview
Updates tests and documentation for mandatory transaction re-extension and reassignment behavior without changing production code.
Changes:
- Isolates transactions in concurrent propagation tests.
- Makes validator fee fixtures explicit.
- Expands reassignment coverage and documents script limitations.
File summaries
| File | Description |
|---|---|
test/e2e/daemon/ready/reassign_test.go |
Tests maturity and stored-owner validation paths. |
services/validator/utxo_commitment_ambiguity_test.go |
Makes the unspendable control fee-independent. |
services/propagation/Server_test.go |
Prevents shared transaction mutation during concurrency testing. |
docs/topics/services/alert.md |
Documents reassignment behavior and 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.
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-09-11 16:33 UTC |
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
The test changes are sound and I verified each rather than reading them. The docs paragraph is the problem.
Verified honest, not papering over:
utxo_commitment_ambiguity_test.gofixes a genuinely red test. Reverting just the two added lines underSETTINGS_CONTEXT=test:"... TX_POLICY (39): transaction fee is too low -> insufficient-fee" does not contain "Opcode missing or not understood".settings.conf:923-927setsminminingtxfee = 0.00000001with.dev/.docker/.teratestnetoverridden to 0 but no.test, andmake testrunsSETTINGS_CONTEXT=test. Only therealSats: 0fixture needs it. TheNewTxValidatorrebuild is necessary, not cosmetic —ScriptVerifierGoBDK.go:218-220snapshots policy into a native object, so mutating settings alone is a silent no-op.Server_test.gofixes a real race, and removes it rather than narrowing it. Base test against current production code under-raceon Postgres: 17WARNING: DATA RACE, writes atValidator.go:1657/:1658. PR head: clean. Production decodes its own tx per submission (Server.go:1058,:693), so this is a test-only defect.ReAssignUTXO(old == new)is supported by both stores, not undefined behaviour — SQL'sWHERE ... frozen = truestill matches andSET utxo_hashis a no-op write; the Aerospike UDF looks up by old hash and writes the new one as separate arguments.
P1 — docs/topics/services/alert.md:139-147 omits that the original owner is locked out too.
The paragraph says a spend signed for the replacement owner is rejected. It does not say the original stored owner is also locked out: re-extension supplies the original script so the script check passes, then the commitment comparison fails against the new hash. Probed: UTXO_MISMATCH (74): [Spend] utxo hash mismatch. The output is stranded for everyone, and there is no inverse operation in the store API.
An operator reading this as "the coin doesn't move yet" runs a court-ordered confiscation and permanently burns the output. That is the sentence to fix before this lands. Recovery — re-freeze and reassign back to the original hash — is undocumented and backend-divergent: SQL's freeze ignores the hash (sql/alert_system.go:96), Aerospike's matches it (aerospike/alert_system.go:74-76).
P1 — two more sentences in the same paragraph are wrong.
- "The reassignment smoke test covers both behaviors" is true for SQL only. The test sets no
UTXOStoreType, so it runs on sqlite, and its maturity assertion depends onReAssignedUtxoSpendableAfterBlocks = 5— which SQL honours and Aerospike ignores (aerospike/alert_system.go:250passes the hardcoded constant fromInterface.go:48). Production Aerospike has no coverage of either behaviour and could not run this test anyway, against a 1000-block gate. - "Reassignment to the stored owner still enforces the maturity delay" describes a case no caller produces.
services/alert/node.go:459-492always derives a new P2PKH from the confiscation input's pubkey, sonew != oldby construction. The surviving success path in the test is a self-reassignment reachable only via the operator RPC (handlers.go:2814). Every production path is broken; only a no-op still works.
P1 — no tracking issue, and the regression is release-visible.
gh issue list --search reassign, gh search issues ... ReAssignUTXO and ... confiscation all return nothing. This is the confiscation path and v0.15.9-beta-1 is tagged. Five other docs still advertise the feature working — rpc_reference.md:1536-1548, minersHowToInteractWithRPCServer.md:196-204, stores/utxo.md:394-399, alert_reference.md:327, glossary.md:3 — plus settings/utxostore_settings.go:54 and stores/utxo/Interface.go:527-528. And services/alert/node.go:455-492 still reports these to the alert network as Processed, with a comment at :478 that is now false.
The repo already has the pattern for this: v0.15.9-beta-1's release body carries a ### Notes section plus "See #1701 for the remaining non-atomic path". Release notes are generated from PR titles, and the release skill's triage puts test-only PRs under Hold — so a test: title makes this invisible. Suggest opening the issue, adding a ### Notes entry naming it, and retitling to mention the documented limitation.
P2, worth doing while the file is open:
TestShouldAllowReassignnow asserts rejection before maturity, after maturity, and that the output stays unspent. The name says the opposite of what it tests.TestReassignEnforcesMaturityAndRejectsUnstoredScriptor similar. (It is a real pin, to be fair — two assertions would fail if the feature started working again.)- All three negative assertions are bare
require.Errorwith no reason pinned.:161's message claims the replacement script was rejected, butnewSpendis still immature at that point — maturity is only confirmed at:170-172— so the rejection is attributable to the gate, not the script, and the assertion survives a script-checking regression. Therequire.Containsdiscipline used atutxo_commitment_ambiguity_test.go:167,193is what made the fee bug above visible; it is absent from the test that most needs it. :165,:172and:180sit exactly on the maturity boundary — the gate is strict<andMineAndWait(5)lands on preciselyspendableIn. One assertion depended on that before; three do now. A one-block shift flips:172toIMMATUREand silently changes:173's rejection reason while still passing. Mine+1.:170-172reads UTXO-store status synchronously with no retry, on that same boundary, whileMineAndWaitwaits on the blockchain store and the UTXO-store height is published asynchronously. New flake surface — and sincemake testexcludes./test/, this runs only in the retrying smoketest suite, so a flake gets retried green rather than reported.- Missing residue check after Bob's early rejection, where the Charles path has one at
:175-177.
Two things outside the diff, both confirmed on main: Aerospike ignores utxostore_reassignedUtxoSpendableAfterBlocks despite the setting being documented "DO NOT reduce in production", and daemon/test_daemon.go:114 still claims an empty UTXOStoreType defaults to aerospike when :454 only builds a container when non-empty — which is plausibly what made the "covers both behaviors" sentence seem true.
ac2833f to
117ce1f
Compare
icellan
changed the title
|
Addressed the requested changes in 117ce1f.
Validation: three race-enabled runs of the renamed smoke test on each branch, race-enabled settings tests, vet/staticcheck, Go/Markdown lint and whitespace/conflict checks. The release follow-up is #1726; its assertions and documentation match main, with one constructor argument adapted for the older validator API and one pre-existing trailing space removed by the required hook. Transaction-validation and store behavior are unchanged. |
There was a problem hiding this comment.
🟡 Changes recommended
A moderate test issue and several documentation corrections remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
docs/howto/miners/minersHowToInteractWithRPCServer.md:197
- The updated command entry still instructs callers to provide a destination address, but the registered
ReassignCmdaccepts four positional values: old txid, vout, old UTXO hash, and new UTXO hash (services/rpc/bsvjson/chainsvrcmds.go:715-720). The handler also returnsnil, not a boolean (services/rpc/handlers.go:2799-2820). Please correct this how-to and its parameter/result details so the new warning is actionable.
6. `reassign`: Updates a frozen UTXO's commitment for a destination address
- **Known regression:** do not reassign to a different owner. A successful result can leave the output unspendable by both owners, even after maturity. See the [reassignment limitation](../../topics/services/alert.md#24-utxo-reassignment) and [issue 1725](https://github.com/bsv-blockchain/teranode/issues/1725).
docs/references/services/rpc_reference.md:1543
- This new section repeats the wrong RPC contract:
ReassignCmdrequiresOldTxID,OldVout,OldUTXOHash, andNewUTXOHash, whilehandleReassignreturnsnilon success (services/rpc/bsvjson/chainsvrcmds.go:715-720,services/rpc/handlers.go:2799-2820). The documented destination address, three-argument example, and boolean result will cause callers to send invalid requests; please update them to the actual hash-based API.
**Known regression:** do not reassign to a different owner. The RPC can return
`true` while leaving the output unspendable by both owners, even after maturity.
See the [reassignment limitation](../../topics/services/alert.md#24-utxo-reassignment)
and [issue 1725](https://github.com/bsv-blockchain/teranode/issues/1725).
docs/topics/services/alert.md:147
- The Alert Service feature summary at lines 33-35 still says it can reassign UTXOs to another address, while this section now says ownership-changing reassignment must not be used. Update that summary too so the page's top-level feature list does not contradict the operator warning.
**Known regression — do not reassign to a different owner:** mandatory transaction
re-extension makes an output with a changed commitment unspendable by both owners,
even after maturity. `ReAssignUTXO` updates the commitment, freeze state and maturity
height but does not persist the replacement locking script. The new owner's spend
fails against the original stored script. The original owner's signature passes
that script, but its spend fails with `UTXO_MISMATCH` against the changed commitment.
Waiting for more blocks does not restore spendability.
docs/topics/stores/utxo.md:395
- The store overview above still advertises
ReAssignUTXOas reassigning a UTXO to a different owner (line 49). That contradicts this new warning and leaves the unsupported ownership-changing path presented as a normal feature; update the overview bullet to describe the commitment-only behavior and restriction.
3. **Reassigning UTXOs**: Updates the commitment of a frozen output.
- **Known regression:** changing the owner leaves the output unspendable by both owners after mandatory re-extension, even after maturity. Do not use ownership-changing reassignment. See the [reassignment limitation](../services/alert.md#24-utxo-reassignment) and [issue 1725](https://github.com/bsv-blockchain/teranode/issues/1725).
test/e2e/daemon/ready/reassign_test.go:60
- The bytes are captured after
ProcessTransactionhas already run. The validator unconditionally re-extends the same*bt.Txin place (services/validator/Validator.go:959-970, 1639-1643), so line 60 serializes the stored Bob script rather than the submitted Charles script; the later retry also reuses that mutated object. This lets the test pass without exercising rejection of a replacement script supplied in the original extended bytes. Savetx.ExtendedBytes()before ingress and run ingress, probe, and retry from fresh decodes.
require.Error(t, td.PropagationClient.ProcessTransaction(td.Ctx, tx))
// Validation re-extends in place. Preserve the submitted extended
// fields so a later attempt still supplies the same replacement script.
probe, err := bt.NewTxFromBytes(tx.ExtendedBytes())
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Lite
|
Addressed Copilot's three inline comments and the additional findings in its review summary in a21b8ae; the same updates are on #1726.
Validation: the reassignment smoke test passed three runs with the race detector on each branch; vet, staticcheck, Go/Markdown lint, whitespace/conflict checks and JSON example validation passed. Transaction-validation and store behavior remain unchanged. |
There was a problem hiding this comment.
🔵 Needs a closer look
Four documentation comments leave contradictory reassignment behavior or API guidance unresolved.
Review details
Suppressed comments (4)
docs/references/services/alert_reference.md:325
- This update still leaves the identifier list below saying reassignment takes a
New Address(line 333), but the corrected API accepts a replacement UTXO hash and no destination address. Please update that stale bullet as well; otherwise this reference contradicts the API contract documented here.
**Known regression:** an applied ownership-changing reassignment can leave the
output unspendable by both owners, even after maturity. Do not treat processing
status as proof of spendability. See the
[reassignment limitation](../../topics/services/alert.md#24-utxo-reassignment) and
[issue 1725](https://github.com/bsv-blockchain/teranode/issues/1725).
docs/topics/services/alert.md:153
- The new warning is followed by an unchanged flow that still describes ownership-changing reassignment as a normal successful procedure: it creates a new locking script, updates the database for the new assignment, and adds successful calls to
processedwithout explaining that the script is not stored and the output remains unspendable. Please update/remove the diagram and steps or label them explicitly as the current unsafe processing path; otherwise operators can follow a procedure this page says not to use.
This affects ownership-changing alert/confiscation and RPC reassignment. A successful
RPC result or alert processing response does not guarantee that the output can be
spent. There is no documented, validated recovery procedure across backends: SQL
freezing ignores the supplied commitment, whereas Aerospike checks it, so operators
must not assume that re-freezing and restoring the original hash is portable.
docs/topics/services/rpc.md:76
Restrictedis not an implemented access restriction:reassignremains inrpcLimited(services/rpc/Server.go:287-340), and the authentication reference still lists it as available to limited users (docs/references/services/rpc_reference.md:418-420). This status therefore conflicts with the actual RPC contract; keep it supported and describe the unsafe ownership-changing behavior in the description, or change the authorization and the auth documentation together.
| reassign | Restricted | Updates a frozen UTXO commitment; ownership changes currently strand the output |
stores/utxo/Interface.go:531
- The interface comment is now accurate, but the exported backend implementations still have contradictory godoc:
stores/utxo/sql/alert_system.go:147-150says reassignment assigns a new transaction output and becomes spendable after the delay, and the Aerospike method still describes a new transaction output. Update those implementation comments too, otherwise readers of the concrete store APIs can still believe ownership-changing reassignment is spendable after maturity.
// ReAssignUTXO updates a frozen UTXO's commitment and maturity gate.
// It does not persist a replacement locking script. Changing the owner
// currently strands the output for both owners even after maturity; see
// https://github.com/bsv-blockchain/teranode/issues/1725.
// SQL honors the configured delay; Aerospike currently uses the fixed constant.
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Re-reviewed at a21b8ae39 (base d5fc44a65 = current main, 15 files +223/−79). Every item from the last round is fixed, and two went past the ask — the title change and the ### Notes block. Nothing here blocks merge.
Verified rather than taken on trust:
- The original-owner lockout is now stated and pinned. Corrupting the expected cause under
-overlayfails with the real error —UTXO_ERROR (79): error in sql spend (batched mode) -> UTXO_MISMATCH (74): [Spend] utxo hash mismatch— sorequireRejected(originalOwnerSpendingTx, "UTXO_MISMATCH")is load-bearing, not decorative. - Every claim in the new
alert.mdparagraph was checked against the store code, the Lua UDF and the error allowlist. All of them hold, including the two that carry the most weight: SQL freezing ignoring the supplied commitment where Aerospike checks it (sql/alert_system.go:59-64,96vsteranode.lua:733), and Aerospike ignoringutxostore_reassignedUtxoSpendableAfterBlocks(aerospike/alert_system.go:250). Nothing is overstated in the other direction. - The four nils to
validator.Neware the three Kafka producers plus the block-assembly client. All nil-guarded, and the block-assembly one is unreachable becauseAddTXToBlockAssembly:falseshort-circuits it at:1233— which also suppressesWithLocked(true), so the probe can't leave a 2PC-locked record. No goroutine starts on this configuration, Prometheus is behind async.Once,-raceis clean (0 races,ok 8.902s). The struct-tag edits have exactly one consumer (settings/export.go:139-142, rawTag.Get) and no golden file. MineAndWait(+1)is margin in the right direction —+0also passes, so it isn't masking an off-by-one.
Three one-line test fixes I'd still take, in priority order:
reassign_test.go:65— the probe passestd.UtxoStore.GetBlockHeight()(tip) while ingress passes0, which resolves toblockState.Height + 1(propagation/Server.go:1429→Validator.go:888-889). That height feeds the maturity gate atsql/sql.go:2763, so the probe is systematically one block stricter than the path it exists to explain. SettestReassignedUtxoSpendableAfter = 1and ingress accepts and permanently spends the output at a height where the probe still says "not spendable until":require.Errorthen fails with a misdirecting message and every later assertion runs against a UTXO set the test already destroyed. Passing0gives an identical PASS.reassign_test.go:62— the helper proves the cause on the probe but leaves ingress as a barerequire.Error. Propagation has two non-verdict failure arms (Server.go:1158-1165/:1203-1210"server at capacity",Server.go:1399-1401blob-storeStorageError) that satisfy it while the probe independently returns the expected cause. Measured at the client, 3 of the 4 call sites already get the exact cause there — onlyUTXO_MISMATCHis redacted, which isERR_UTXO_MISMATCHbeing absent frompublicCauseCodes(errors/errors.go:773-812) whileERR_UTXO_FROZENandERR_TX_INVALIDare present. So the redaction comment is right, but the probe is needed for one assertion, not four. Capture the ingress error and assert on it for the other three.reassign_test.go:193—require.Eventuallydiscardserr,statusand which spend failed, andGetSpendreports a commitment mismatch as an error rather than a status (sql/sql.go:3545-3547). A futureReAssignUTXOregression would surface as a 30-second "both outputs must mature in the UTXO store" with the cause never printed.require.EventuallyWithT.
Doc surfaces still advertising the feature — my earlier list was short, so these are new asks, not repeats:
docs/topics/services/rpc.md:76— table still readsreassign | Supported | Reassigns ownership … to a new Bitcoin address.docs/topics/services/rpc.md:1053-1104— a full ownership-transfer how-to, unchanged, directly under the new warning.docs/topics/services/img/plantuml/rpc/rpc-reassign.puml:92-96, rendered atrpc.md:1068—Effects: - UTXO ownership changed to new address - Original script replaced with new script. The diagram asserts exactly what the warning retracts. (alert_reassign_utxo.svgis fine — it's a control-flow diagram and makes no spendability claim.)- Lower priority:
alert.md:33-35feature list;sql/alert_system.go:147-149andaerospike/alert_system.go:215-223, where only the interface godoc was updated; andstores/utxo/tests/tests.go:276,316-318, a conformance test still asserting different-owner reassignment "should succeed".
Separately, and more important than anything left in this PR: #1722 merged at 14:44:59Z with the old wording. release/v0.15 currently carries the paragraph that omits the original-owner lockout and still says "the reassignment smoke test covers both behaviors" and "reassignment to the stored owner still enforces the maturity delay". Verified against the branch. That's the version that ships, and it needs its own backport of the corrected paragraph, the Interface.go and settings comment fixes, and the strengthened test. The comment I left on #1722 landed after the merge, so nothing is gated on it.
One nit and one correction. Nit: alert.md:141-143 is slightly imprecise on cause — re-extension produces the new owner's lockout; the original owner's UTXO_MISMATCH predates it and is the intended effect of confiscation. Correction to my own last review: I flagged alert_reassign_utxo.svg as suspect and it isn't — rpc-reassign.puml is the wrong diagram.
Mandatory transaction re-extension exposed three test assumptions: concurrent propagation submissions shared mutable transactions, the zero-value invalid-script fixture inherited a minimum fee, and reassignment relied on a replacement locking script that was never stored.
The propagation fixture now decodes a transaction for each concurrent submission, and the invalid-script control explicitly disables the minimum fee and rebuilds the verifier. The renamed SQLite reassignment test pairs public-ingress rejection with exact error checks through a real validator over the same store. It pins script failure for the replacement owner and commitment mismatch for the original owner after maturity, checks that rejected spends leave outputs unspent, and retains self-reassignment only as a positive maturity control. It mines one extra block and waits for both UTXOs to mature before testing those rejection reasons.
The alert, RPC, store, miner and glossary docs now warn that a changed-owner reassignment strands the output for both owners. The documentation distinguishes SQLite coverage from Aerospike, which currently ignores the configurable maturity delay, and links the restoration/recovery issue. Misleading API, settings and daemon comments are corrected. Transaction-validation and store behavior are unchanged.
Validation:
testtxmetacache.go vetandstaticcheckfor the affected test/settings packages.Notes
Ownership-changing UTXO reassignment is currently unsafe after mandatory transaction re-extension: both the original and replacement owners are unable to spend the reassigned output, even after maturity. The alert/confiscation and RPC paths can report successful processing despite that lockout. Do not use ownership-changing reassignment; restoration and a validated recovery procedure are tracked in #1725.
This note is for releases containing the re-extension security merge. The existing
v0.15.9-beta-1tag points to9272b80c9259d00fd51eb8f66ee658a46adb940a, before that merge, so its release notes have not been marked as affected.