Repository navigation
Conversation
|
🤖 Claude Code Review Status: Complete Current Review: The expression path now calls History:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-10-28 19:14 UTC |
fc8414f to
2d8d13b
Compare
2d8d13b to
8ff6d94
Compare
freemans13
left a comment
freemans13
left a comment
There was a problem hiding this comment.
Approving. The code fixes bitcoin-sv/teranode#4934. One thing to do before merge: rewrite the PR description, which still describes the first design.
The description is stale. Commit 8ff6d94 replaced the persisted-spender lookup with ordered backend writes, but the description still documents the lookup. It describes the PersistedSpender callback, the memoised pool read, Aerospike calling GetSpend, and a residual risk about a read outside the batch transaction. None of those exist any more. It also says the CHANGELOG entry sits under "Not breaking, but worth knowing", but the entry is under "Breaking — behaviour changes on upgrade". That placement is the right one, since FilterConflictingDuplicateSpendClaims is removed. The test table is missing TestSpendBatchReportsPersistedOwnerOutsideBatch, TestSpendBatchStorageFailureDoesNotInventConflict and TestSpendSplitCompletesAllClaimsOnBackendFailure. If this is squash-merged, that text becomes the commit message.
What I checked on e71688a:
- Every
processcallback finishes before returning. SQL goes throughretrySpendBatch. Aerospike runs eitherexecuteLuaSpendBatchorSpendMultiWithExpressions, and the expressions path retries through Lua inline (spend_expressions.go:534, no goroutine). So each part really is decided against what the earlier parts wrote. - The panic sweep in
sendSpendBatchLuacannot double-complete an item from an earlier part, becausebatchSpend.completeis guarded by a compare-and-swap. - Losing claims now reach the backend instead of being refused in memory. I checked that this cannot hurt a sibling: the Lua
spendMultireports errors per item and applies the other claims in the call, so a loser sharing a parent record with an honest spend does not drag it down. Refused spends have no side effects beyond the error. - Locally, these all pass:
TestProcessSpendBatch, the four SQLite tests,TestRespendBehindLoserInSameBatchPostgresBulkand #825's Postgres bulk test (Postgres testcontainer), and on an Aerospike testcontainerTestRespendBehindLoserInSameBatch,TestSpendSplitCompletesAllClaimsOnBackendFailure,TestStore_SpendBatchRejectsDuplicateDifferentSpendersandTestStore_SpendExpressions_CrossBatchDoubleSpend, all on bothluaandexpressions.
What I did not check: CI on this head (a merge of main) was still queued when I looked. The SonarQube comment predates this head, though the PR's own code has not changed since 6f1ccb3. I ran only the tests above, not the full suite.
On the bot's open thread about cost: I agree it is minor. The worst case is one backend call per item, and a batch holds at most utxostore_spendBatcherSize items (1024 by default), so the cost is bounded. One inline note below, non-blocking.
| // share a part. Every item reaches process exactly once, including invalid items | ||
| // whose validation belongs to the backend. | ||
| // | ||
| // process must finish the backend operation before returning. The backend then |
There was a problem hiding this comment.
Non-blocking. I disabled the split (made the conflict check always false) in a scratch copy, and every backend test still passed. That covers the four SQLite tests, both Postgres bulk tests including #825's TestSpendBatchRejectsDuplicateDifferentSpendersPostgresBulk, and the Aerospike tests on lua and expressions. Only TestProcessSpendBatch notices.
The split is still worth keeping. There are two reasons, and neither engine guarantees an order here:
- Postgres says that when
UPDATE ... FROMjoins two source rows to one target row, which row it uses "is not readily predictable". It happens to follow the order of the VALUES list today. - On Aerospike, two batch sub-operations on one key also have no promised order. Two Lua calls end up in the same batch when claims differ only in block height or ignore flags, and expressions always put each claim in its own sub-operation.
The split therefore protects against unspecified behaviour that no test can provoke. Could the comment say so, naming those two cases? Otherwise someone measuring the extra calls the bot flagged will find nothing fails without the split and remove it.
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
Splitting at the first conflicting claim fixes the same-batch respend, but every conflicting claim now costs its own serial backend round trip instead of a free local reject. Four inline points below.
|
Fixed in 147424c. Thanks, all four points were correct. 147424c fixes them:
One limit remains. If the earlier claim fails without a known owner (storage error, missing parent, frozen), the next competing claim still costs its own call. I kept that on purpose: without a known owner, a local decision could invent a conflict, which is the bug this PR fixes. |
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
Previous four points are addressed and resolved. The new breaker accounting has its own problem, plus two questions on the local reject.
| // batch without a split. The deferred record keeps the expression-path | ||
| // result of earlier parts when a later part panics. | ||
| var outcome spendBreakerOutcome | ||
| defer outcome.record(s.spendCircuitBreaker) |
There was a problem hiding this comment.
[Minor] On the expression path, this still changes the order of breaker events for a batch that is never split. Before this PR, processSpendBatchResultsExpressions called RecordSuccess/RecordFailure before it ran the FILTERED_OUT retry through executeLuaSpendBatch. Now the retry records inline through countLuaRecord, which returns true whenever split is false, and the expression flags are applied later in this defer. So the Lua retry events now come first.
Example: the breaker is half-open with halfOpenMax=1. One expression record times out, and one FILTERED_OUT record is retried through Lua and returns OK. Before: RecordFailure trips the breaker back to open, and the Lua success is then ignored. Now: the Lua RecordSuccess resets the breaker to closed, and the deferred RecordFailure only sets consecutiveFailures=1. The breaker closes while the backend is still failing. In the closed state the leftover count also differs (1 instead of 0).
This conflicts with the CHANGELOG line "counts a batch that is not split exactly as before". One option is to apply the expression flags right away when !outcome.split, before the retry, and use the defer only for split batches. A test like TestSpendLuaBreakerRecordsInOrder, but on the expression path with a timeout plus a FILTERED_OUT retry, would cover it.
There was a problem hiding this comment.
Confirmed fixed in 5fc0482. processSpendBatchResultsExpressions now calls outcome.recordExpressions before it dispatches the FILTERED_OUT retry through executeLuaSpendBatch, and the deferred record is gone. An unsplit batch therefore records expression success, then expression failure, then the Lua retry events, which is the order before this PR. TestSpendExpressionBreakerBeforeLuaRetry covers this. This thread can be resolved.
|
|
||
| rejectedByOwner := func(item T, spend *Spend, key spendOutpointKey) bool { | ||
| owner, known := owners[key] | ||
| if !known || sameSpendingData(owner.data, spend.SpendingData) || !h.SameChecks(owner.decider, item) { |
There was a problem hiding this comment.
[Minor] ProcessSpendBatch is exported, and the CHANGELOG tells Go integrations to migrate to it. A handler that leaves SameChecks nil works for batches without competing claims. It then panics with a nil func call the first time a competing claim meets a known owner, so the failure only shows up when a double-spend arrives. Both in-tree handlers set it. Consider treating nil as "never reject locally" (h.SameChecks == nil || !h.SameChecks(...)), or say in the field doc that it is required.
There was a problem hiding this comment.
Confirmed fixed in 5fc0482. A nil SameChecks now means "never reject locally", the field doc says so, and TestProcessSpendBatchWithoutSameChecks covers it. This thread can be resolved.
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Request changes: one Aerospike spend-breaker regression. An unsplit expression batch can count a successful Lua retry before the expression failure that caused it, leaving the breaker closed during a partial backend failure where the previous code opened it.
- P1 —
stores/utxo/aerospike/spend.go:1019andstores/utxo/aerospike/spend_expressions.go:481-526: When one expression record times out and another returnsFILTERED_OUTthen succeeds on Lua retry, the Lua success resets the breaker before the deferred expression failure is recorded. With one prior failure and threshold two, the breaker ends closed instead of opening as it did before this change. Please preserve expression-before-retry breaker order, including when a batch is not split, and add a regression test for this mixed result.
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
Spend fix looks right now; resolved the threads it covers. What's left is breaker accounting: split batches still drop record outcomes, the expression-path ordering (bot thread on spend.go:1019) is open, and the CHANGELOG overstates both.
…lter rejects an idempotent re-spend Spend a double-spend attempt ahead of an idempotent re-spend of the legitimate spender in one batch. FilterConflictingDuplicateSpendClaims awards the outpoint to the first claim in the batch without consulting the persisted spender, so the re-spend is rejected with ErrSpent naming a tx that never spent anything. Fails on 5021b54 at duplicate_spend.go:48.
FilterConflictingDuplicateSpendClaims awarded a colliding outpoint to the first claim in the batch. Batch arrival order has no consensus meaning, so an idempotent re-spend by the tx that already owns the outpoint lost to a double-spend attempt that arrived a moment earlier. The rejected re-spend then made Store.Spend un-spend the inputs it had just re-applied. Take the winner from the persisted spender instead. Both backends answer a new PersistedSpender callback, consulted only for outpoints that actually collide, so an ordinary batch pays nothing extra. With nothing persisted the first claim still wins, which is the guarantee bsv-blockchain#825 added the filter for: two genuinely conflicting claims never both reach the backend. Refs bitcoin-sv/teranode#4934.
…ier claim in its batch Also restores the sendSpendBatchLua doc comment, which the new Aerospike helper had been inserted into.
… Aerospike paths Drive the SQL case through sendSpendBatch with an explicit batch so it no longer depends on batcher timing, and pin the persisted spender out of the spent-error rather than matching the message. The end-to-end SQLite test stays: the rollback that loses the spend lives in Store.Spend, not in the batch callback. Verified each test fails with the resolver disabled and passes with it on: SQLite, Postgres 13 bulk, Aerospike Lua and Aerospike expressions.
…r is known A batch that alternated two valid spends of one outpoint cost one serial backend call per claim. ProcessSpendBatch now reads each part's answers, learns the stored owner from a success or from UTXO_SPENT data, and refuses later competing claims with UTXO_SPENT naming that owner. Claims that match the owner still reach the backend as idempotent re-spends. The SQL spend batcher answers each caller through a once-only deliver and recovers a panic, so claims in parts that never ran get an error instead of waiting for the spend timeout. The Aerospike expression-path circuit breaker and the spend batch histograms now count one batcher batch, however many parts it needs.
The Lua path recorded the spend circuit breaker once per batch record in each part, so a split batch counted the same record key more than once. The Lua handlers now collect results in spendBreakerOutcome, and sendSpendBatchLua records each record key once. A split batch counts like the same batch without a split; distinct keys still count separately.
…a local reject The Lua spend breaker merged records with equal values, so an unsplit batch with two inputs from one parent counted once instead of twice, and it applied all successes before all failures. The Lua path now records each batch record inline, in order, as before; a split batch only skips a record key that an earlier part already counted. The expression-path result is recorded in a defer, so a panic in a later part keeps the failures of earlier parts. ProcessSpendBatch now refuses a claim locally only when SameChecks holds against the claim that showed the owner: the same UTXO hash, block height and ignore flags. Other claims go to the store, which can answer CONFLICTING, LOCKED or a hash mismatch.
3380ac4 to
5fc0482
Compare
|
Problem
An idempotent re-spend could be rejected because a competing claim arrived earlier in the same UTXO batch. The error named a transaction that never owned the output and could trigger rollback of legitimate spends. This fixes bitcoin-sv/teranode#4934.
Changes
ProcessSpendBatchsubmits competing claims in ordered backend parts. Success or authoritativeUTXO_SPENTdata identifies the stored owner; later competing claims are rejected locally only when their UTXO hash, height and validation flags match. Owner re-spends still reach the backend. Errors naming the claim itself do not establish ownership.SameCheckscallback safely disables local rejection.FilterConflictingDuplicateSpendClaims.Rebased onto
upstream/main, preserving both the upstream and PR changelog entries.Validation
Passed the focused race-enabled regression run across
stores/utxo,stores/utxo/sqlandstores/utxo/aerospike, including real PostgreSQL and Aerospike containers (no skipped tests). Coverage includes persisted-owner re-spends, absent owners, storage failures, alternating claims, SQL panic completion, different validation flags, nilSameChecks, repeated Lua keys, mixed expression/Lua outcomes in closed and half-open states, split batches, retry panics, and failure after expression or Lua recovery. The new failure cases were reproduced before their fixes.Passed UTXO
go vet, Staticcheck and changed-code golangci-lint checks.Broader
go test -short -tags testtxmetacache ./stores/utxo/...runs, both with and without-race, failed only atTestNativeTeranodeOpProbe_Container/executeTeranodeOp_native_path: Aerospike returnedFAIL_FORBIDDENduring startup. The same failure was reproduced onupstream/main. Other UTXO packages passed.Security scans match the base branch: Gosec reports the same 105 findings, and govulncheck reports the same GO-2026-6809 and GO-2026-6253 vulnerabilities. This PR does not change dependencies or the Go toolchain.
Limits
A claim whose earlier competitor failed without a known owner still costs its own backend call. Local rejection reflects a stored state observed while the claim was waiting; a concurrent rollback can subsequently unspend that owner. General cross-batch rollback and timeout behavior is unchanged.