Skip to content

fix(utxo): verify spend ownership in Unspend#766

Merged
ordishs merged 7 commits into
bsv-blockchain:mainfrom
ordishs:security/4562-unspend-ownership-check
May 19, 2026
Merged

fix(utxo): verify spend ownership in Unspend#766
ordishs merged 7 commits into
bsv-blockchain:mainfrom
ordishs:security/4562-unspend-ownership-check

Conversation

@ordishs

@ordishs ordishs commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Closes #4562. Also closes #4589 (the audit explicitly cross-references — same SQL AND spending_data = $3 fix).

Summary

Both Aerospike and SQL Unspend clear spending_data based only on (TxID, Vout, UTXOHash) — without verifying the caller's expected spending data matches what's stored. If any caller (reorg, conflict resolution, batcher) passes a wrong/stale Spend record, the legitimate spend is silently wiped, opening a path to UTXO corruption / double-spend.

Fix

  • SQL (stores/utxo/sql/sql.go): extend the q1 UPDATE with AND spending_data = $3. When 0 rows are updated, run a diagnostic SELECT spending_data to distinguish three cases (matches the three branches in stores/utxo/aerospike/teranode.lua::unspend):
    • Row missing → NotFoundError("output %s:%d not found").
    • Row exists, spending_data IS NULL (UTXO already unspent) → ProcessingError("expected spending data but UTXO is unspent").
    • Row exists, spending_data differs → ProcessingError("ownership mismatch — caller's SpendingData does not match stored spend").
  • Aerospike (stores/utxo/aerospike/teranode.lua, un_spend.go): add expectedSpendingData parameter to the unspend Lua function and a byte-equality check against the stored value. The Go layer maps Lua error codes to typed Go errors:
    • ERROR_CODE_TX_NOT_FOUNDNotFoundError (parity with SQL).
    • ERROR_CODE_SPEND_OWNERSHIP_MISMATCH (with ERR_SPEND_OWNERSHIP_UNSPENT or ERR_SPEND_OWNERSHIP_MISMATCH message) → ProcessingError.
  • SpendingData is mandatory: both backends reject nil with ProcessingError before reaching the database. No escape hatch — every production caller derives spends from Spend() / SetConflicting() / GetSpends(), all of which populate it (audit below).

Caller audit

All production callers of Unspend already pass Spend records with SpendingData populated:

  • ProcessConflictingMarkConflictingRecursivelySetConflicting populates via spendpkg.NewSpendingData(tx.TxIDChainHash(), i) (aerospike conflicting.go:109, sql sql.go:4017).
  • Validator.reverseSpends, the SQL/Aerospike Spend rollback paths → all consume Spend() results, which are built from GetSpends(tx) (stores/utxo/utils.go:184-189).

A handful of test-only callers were silently relying on the buggy behaviour by passing mismatched SpendingData. They've been corrected to pass the SpendingData that GetSpends actually produces:

  • stores/utxo/tests/tests.go::Restore
  • stores/utxo/sql/sql_test.go::TestUnspend, TestTombstoneAfterSpendAndUnspend, TestSpendAndUnspendEdgeCases
  • stores/utxo/aerospike/aerospike_server_test.go::TestAerospike/aerospike_reset, TestAerospike/LUAScripts

Test plan

  • New SQL tests (sql_unspend_ownership_test.go): TestUnspendOwnership_HappyPath / _Mismatch / _NotFound / _MismatchOnUnspent / _NilSpendingDataRejected — exercise all three dispatch branches plus the nil guard, and assert that mismatched/rejected calls leave spending_data unchanged.
  • New Aerospike tests (unspend_ownership_test.go): TestUnspendOwnership_HappyPath / _Mismatch / _NotFound / _NilSpendingDataRejected — same branches as SQL, plus parity for the ERROR_CODE_TX_NOT_FOUNDNotFoundError mapping.
  • All ProcessConflicting tests still pass.
  • go test -race ./stores/utxo/sql/..., go test -race ./stores/utxo/, go test -race ./services/validator/ all green.

@github-actions

github-actions Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Status: Complete

Summary

This PR implements a critical security fix for UTXO spend ownership verification in the Unspend operation. The implementation prevents a scenario where stale or incorrect spend records could silently wipe legitimate spends, potentially leading to UTXO corruption or double-spend vulnerabilities.

Key Changes:

  • Added ownership verification via spending_data matching in both SQL and Aerospike backends
  • Made Unspend idempotent: ownership mismatches are silent no-ops (preserving existing data)
  • Mandatory SpendingData parameter with nil-guard
  • Comprehensive test coverage for happy path, mismatches, not-found, and edge cases

Security Impact: High-severity fix — closes #4562

Issues Found

[Minor] Unused Error Type
The ErrUtxoUnspent error type and NewUtxoUnspentError function are defined but never emitted after the latest commit (68b6cbc). The final commit message acknowledges this: "kept as unused-but-available API". Consider either using it or removing it to avoid dead code and potential confusion for future maintainers.

Notes

  • Test coverage is thorough with ownership verification tests for both backends
  • The idempotent semantics ("never wipe a spend we don't own") are well-documented
  • SQL implementation uses READ COMMITTED isolation which is appropriate for the concurrent access patterns
  • Lua package version correctly bumped (v57 → v59) to ensure UDF re-registration

@github-actions

github-actions Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Benchmark Comparison Report

Baseline: main (unknown)

Current: PR-766 (839e8cd)

Summary

  • Regressions: 0
  • Improvements: 0
  • Unchanged: 142
  • Significance level: p < 0.05
All benchmark results (sec/op)
Benchmark Baseline Current Change p-value
_NewBlockFromBytes-4 1.586µ 1.552µ ~ 0.100
SplitSyncedParentMap_SetIfNotExists/256_buckets-4 71.33n 72.12n ~ 0.700
SplitSyncedParentMap_SetIfNotExists/16_buckets-4 71.31n 71.03n ~ 0.400
SplitSyncedParentMap_SetIfNotExists/1_bucket-4 71.11n 71.41n ~ 0.100
SplitSyncedParentMap_ConcurrentSetIfNotExists/256_buckets... 31.82n 32.71n ~ 0.100
SplitSyncedParentMap_ConcurrentSetIfNotExists/16_buckets_... 53.61n 54.03n ~ 0.400
SplitSyncedParentMap_ConcurrentSetIfNotExists/1_bucket_pa... 128.8n 129.7n ~ 0.800
MiningCandidate_Stringify_Short-4 223.5n 215.9n ~ 0.100
MiningCandidate_Stringify_Long-4 1.613µ 1.617µ ~ 0.700
MiningSolution_Stringify-4 841.6n 850.3n ~ 0.100
BlockInfo_MarshalJSON-4 1.707µ 1.683µ ~ 0.100
NewFromBytes-4 125.3n 125.0n ~ 1.000
Mine_EasyDifficulty-4 60.90µ 61.50µ ~ 0.400
Mine_WithAddress-4 7.351µ 6.841µ ~ 0.700
BlockAssembler_AddTx-4 0.02860n 0.02911n ~ 1.000
AddNode-4 10.91 10.90 ~ 1.000
AddNodeWithMap-4 11.01 10.62 ~ 0.100
DirectSubtreeAdd/4_per_subtree-4 57.70n 62.61n ~ 0.100
DirectSubtreeAdd/64_per_subtree-4 28.94n 28.66n ~ 0.100
DirectSubtreeAdd/256_per_subtree-4 28.19n 27.34n ~ 0.200
DirectSubtreeAdd/1024_per_subtree-4 26.42n 26.24n ~ 0.100
DirectSubtreeAdd/2048_per_subtree-4 26.00n 25.94n ~ 0.600
SubtreeProcessorAdd/4_per_subtree-4 293.3n 280.4n ~ 0.100
SubtreeProcessorAdd/64_per_subtree-4 283.4n 277.2n ~ 0.200
SubtreeProcessorAdd/256_per_subtree-4 284.0n 277.4n ~ 0.100
SubtreeProcessorAdd/1024_per_subtree-4 276.5n 270.2n ~ 0.100
SubtreeProcessorAdd/2048_per_subtree-4 273.1n 270.8n ~ 0.100
SubtreeProcessorRotate/4_per_subtree-4 277.3n 272.3n ~ 0.100
SubtreeProcessorRotate/64_per_subtree-4 277.4n 272.6n ~ 0.200
SubtreeProcessorRotate/256_per_subtree-4 275.2n 272.0n ~ 0.100
SubtreeProcessorRotate/1024_per_subtree-4 275.8n 273.3n ~ 0.200
SubtreeNodeAddOnly/4_per_subtree-4 54.71n 54.80n ~ 1.000
SubtreeNodeAddOnly/64_per_subtree-4 34.56n 34.34n ~ 0.100
SubtreeNodeAddOnly/256_per_subtree-4 33.57n 33.62n ~ 0.100
SubtreeNodeAddOnly/1024_per_subtree-4 32.78n 32.90n ~ 0.700
SubtreeCreationOnly/4_per_subtree-4 114.4n 114.0n ~ 1.000
SubtreeCreationOnly/64_per_subtree-4 400.6n 400.3n ~ 1.000
SubtreeCreationOnly/256_per_subtree-4 1.337µ 1.337µ ~ 1.000
SubtreeCreationOnly/1024_per_subtree-4 4.498µ 4.428µ ~ 0.100
SubtreeCreationOnly/2048_per_subtree-4 8.277µ 8.016µ ~ 0.200
SubtreeProcessorOverheadBreakdown/64_per_subtree-4 276.8n 271.4n ~ 0.100
SubtreeProcessorOverheadBreakdown/1024_per_subtree-4 275.5n 276.6n ~ 1.000
ParallelGetAndSetIfNotExists/1k_nodes-4 587.8µ 814.8µ ~ 0.100
ParallelGetAndSetIfNotExists/10k_nodes-4 1.333m 1.580m ~ 0.100
ParallelGetAndSetIfNotExists/50k_nodes-4 6.694m 6.761m ~ 0.200
ParallelGetAndSetIfNotExists/100k_nodes-4 13.66m 13.51m ~ 0.100
SequentialGetAndSetIfNotExists/1k_nodes-4 676.6µ 667.6µ ~ 0.700
SequentialGetAndSetIfNotExists/10k_nodes-4 2.782m 2.782m ~ 1.000
SequentialGetAndSetIfNotExists/50k_nodes-4 10.44m 10.34m ~ 0.700
SequentialGetAndSetIfNotExists/100k_nodes-4 20.42m 20.12m ~ 0.100
ProcessOwnBlockSubtreeNodesParallel/1k_nodes-4 637.0µ 855.7µ ~ 0.100
ProcessOwnBlockSubtreeNodesParallel/10k_nodes-4 4.244m 4.196m ~ 1.000
ProcessOwnBlockSubtreeNodesParallel/100k_nodes-4 16.71m 16.77m ~ 1.000
ProcessOwnBlockSubtreeNodesSequential/1k_nodes-4 698.4µ 700.5µ ~ 0.700
ProcessOwnBlockSubtreeNodesSequential/10k_nodes-4 5.782m 5.762m ~ 0.400
ProcessOwnBlockSubtreeNodesSequential/100k_nodes-4 38.75m 38.39m ~ 0.700
DiskTxMap_SetIfNotExists-4 3.555µ 3.677µ ~ 0.200
DiskTxMap_SetIfNotExists_Parallel-4 3.493µ 3.420µ ~ 0.400
DiskTxMap_ExistenceOnly-4 310.3n 295.1n ~ 0.100
Queue-4 188.8n 188.6n ~ 1.000
AtomicPointer-4 4.625n 4.935n ~ 0.700
ReorgOptimizations/DedupFilterPipeline/Old/10K-4 897.6µ 836.5µ ~ 0.100
ReorgOptimizations/DedupFilterPipeline/New/10K-4 794.9µ 781.6µ ~ 0.100
ReorgOptimizations/AllMarkFalse/Old/10K-4 123.1µ 114.1µ ~ 0.400
ReorgOptimizations/AllMarkFalse/New/10K-4 62.06µ 62.53µ ~ 0.400
ReorgOptimizations/HashSlicePool/Old/10K-4 64.75µ 55.45µ ~ 0.200
ReorgOptimizations/HashSlicePool/New/10K-4 11.95µ 10.50µ ~ 0.100
ReorgOptimizations/NodeFlags/Old/10K-4 5.124µ 4.626µ ~ 0.700
ReorgOptimizations/NodeFlags/New/10K-4 2.012µ 1.644µ ~ 0.100
ReorgOptimizations/DedupFilterPipeline/Old/100K-4 9.852m 9.222m ~ 0.100
ReorgOptimizations/DedupFilterPipeline/New/100K-4 10.099m 9.535m ~ 0.100
ReorgOptimizations/AllMarkFalse/Old/100K-4 1.192m 1.132m ~ 0.100
ReorgOptimizations/AllMarkFalse/New/100K-4 689.9µ 684.3µ ~ 0.400
ReorgOptimizations/HashSlicePool/Old/100K-4 512.3µ 556.2µ ~ 0.100
ReorgOptimizations/HashSlicePool/New/100K-4 284.9µ 299.1µ ~ 0.400
ReorgOptimizations/NodeFlags/Old/100K-4 49.14µ 49.66µ ~ 0.700
ReorgOptimizations/NodeFlags/New/100K-4 17.25µ 17.27µ ~ 1.000
TxMapSetIfNotExists-4 54.33n 51.02n ~ 0.100
TxMapSetIfNotExistsDuplicate-4 38.07n 38.87n ~ 0.100
ChannelSendReceive-4 608.3n 587.5n ~ 0.100
CalcBlockWork-4 479.5n 476.3n ~ 0.400
CalculateWork-4 633.8n 635.7n ~ 0.100
BuildBlockLocatorString_Helpers/Size_10-4 1.319µ 1.322µ ~ 0.500
BuildBlockLocatorString_Helpers/Size_100-4 15.10µ 12.68µ ~ 0.200
BuildBlockLocatorString_Helpers/Size_1000-4 124.8µ 125.7µ ~ 0.200
CatchupWithHeaderCache-4 104.3m 104.4m ~ 0.700
_prepareTxsPerLevel-4 421.4m 415.3m ~ 0.400
_prepareTxsPerLevelOrdered-4 3.987m 3.670m ~ 0.100
_prepareTxsPerLevel_Comparison/Original-4 436.0m 420.8m ~ 0.100
_prepareTxsPerLevel_Comparison/Optimized-4 4.067m 3.607m ~ 0.100
SubtreeSizes/10k_tx_4_per_subtree-4 1.276m 1.270m ~ 1.000
SubtreeSizes/10k_tx_16_per_subtree-4 295.6µ 294.7µ ~ 1.000
SubtreeSizes/10k_tx_64_per_subtree-4 71.60µ 71.59µ ~ 1.000
SubtreeSizes/10k_tx_256_per_subtree-4 17.65µ 17.65µ ~ 1.000
SubtreeSizes/10k_tx_512_per_subtree-4 8.814µ 8.818µ ~ 1.000
SubtreeSizes/10k_tx_1024_per_subtree-4 4.332µ 4.355µ ~ 1.000
SubtreeSizes/10k_tx_2k_per_subtree-4 2.165µ 2.180µ ~ 0.100
BlockSizeScaling/10k_tx_64_per_subtree-4 69.42µ 69.45µ ~ 1.000
BlockSizeScaling/10k_tx_256_per_subtree-4 17.45µ 17.40µ ~ 0.400
BlockSizeScaling/10k_tx_1024_per_subtree-4 4.316µ 4.350µ ~ 0.400
BlockSizeScaling/50k_tx_64_per_subtree-4 375.4µ 378.4µ ~ 0.700
BlockSizeScaling/50k_tx_256_per_subtree-4 87.06µ 87.35µ ~ 0.700
BlockSizeScaling/50k_tx_1024_per_subtree-4 21.54µ 21.69µ ~ 0.700
SubtreeAllocations/small_subtrees_exists_check-4 148.2µ 149.3µ ~ 1.000
SubtreeAllocations/small_subtrees_data_fetch-4 161.6µ 157.8µ ~ 0.100
SubtreeAllocations/small_subtrees_full_validation-4 310.6µ 304.3µ ~ 0.200
SubtreeAllocations/medium_subtrees_exists_check-4 8.863µ 8.935µ ~ 0.700
SubtreeAllocations/medium_subtrees_data_fetch-4 9.369µ 9.320µ ~ 0.700
SubtreeAllocations/medium_subtrees_full_validation-4 17.48µ 17.55µ ~ 0.600
SubtreeAllocations/large_subtrees_exists_check-4 2.087µ 2.068µ ~ 0.800
SubtreeAllocations/large_subtrees_data_fetch-4 2.253µ 2.231µ ~ 0.100
SubtreeAllocations/large_subtrees_full_validation-4 4.313µ 4.363µ ~ 1.000
_BufferPoolAllocation/16KB-4 3.346µ 3.296µ ~ 0.100
_BufferPoolAllocation/32KB-4 7.248µ 7.217µ ~ 1.000
_BufferPoolAllocation/64KB-4 13.20µ 16.01µ ~ 0.700
_BufferPoolAllocation/128KB-4 34.11µ 30.42µ ~ 0.100
_BufferPoolAllocation/512KB-4 130.6µ 122.3µ ~ 0.700
_BufferPoolConcurrent/32KB-4 17.08µ 18.98µ ~ 0.100
_BufferPoolConcurrent/64KB-4 26.43µ 27.98µ ~ 0.100
_BufferPoolConcurrent/512KB-4 168.9µ 159.5µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/16KB-4 684.7µ 704.8µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/32KB-4 683.3µ 694.2µ ~ 0.400
_SubtreeDeserializationWithBufferSizes/64KB-4 665.7µ 692.5µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/128KB-4 667.2µ 714.2µ ~ 0.100
_SubtreeDeserializationWithBufferSizes/512KB-4 681.8µ 701.6µ ~ 0.700
_SubtreeDataDeserializationWithBufferSizes/16KB-4 37.84m 38.18m ~ 0.100
_SubtreeDataDeserializationWithBufferSizes/32KB-4 38.03m 37.98m ~ 0.700
_SubtreeDataDeserializationWithBufferSizes/64KB-4 37.99m 38.10m ~ 1.000
_SubtreeDataDeserializationWithBufferSizes/128KB-4 38.43m 37.93m ~ 0.700
_SubtreeDataDeserializationWithBufferSizes/512KB-4 38.17m 38.31m ~ 0.700
_PooledVsNonPooled/Pooled-4 817.9n 819.7n ~ 0.700
_PooledVsNonPooled/NonPooled-4 7.227µ 6.769µ ~ 0.400
_MemoryFootprint/Current_512KB_32concurrent-4 9.199µ 9.164µ ~ 0.700
_MemoryFootprint/Proposed_32KB_32concurrent-4 10.97µ 11.07µ ~ 0.700
_MemoryFootprint/Alternative_64KB_32concurrent-4 10.58µ 10.67µ ~ 0.700
StoreBlock_Sequential/BelowCSVHeight-4 317.2µ 313.0µ ~ 0.200
StoreBlock_Sequential/AboveCSVHeight-4 316.3µ 312.1µ ~ 0.400
GetUtxoHashes-4 257.2n 258.6n ~ 0.700
GetUtxoHashes_ManyOutputs-4 44.02µ 44.40µ ~ 0.700
_NewMetaDataFromBytes-4 239.1n 234.6n ~ 0.100
_Bytes-4 623.2n 628.0n ~ 1.000
_MetaBytes-4 570.1n 567.7n ~ 0.100

Threshold: >10% with p < 0.05 | Generated: 2026-05-15 10:42 UTC

ordishs added 5 commits April 28, 2026 18:21
SQL Unspend collapsed 'row missing', 'UTXO unspent', and 'ownership
mismatch' into a single ProcessingError, while Aerospike's Lua
already distinguished the three. Callers couldn't tell a genuinely-
unknown output from a mismatched spend.

- SQL: a diagnostic SELECT after a 0-row UPDATE dispatches the three
  cases into NotFoundError / 'UTXO is unspent' / 'ownership mismatch'.
- Aerospike: map LuaErrorCodeTxNotFound to NotFoundError so both
  backends return the same Go error type for missing records.

Adds TestUnspendOwnership_NotFound on both backends; tightens the
soft 'might not error' assertion in TestSpendAndUnspendEdgeCases and
the message check in TestUnspendOwnership_MismatchOnUnspent.
…562)

Unspend's "row exists, spending_data is nil" branch is functionally
idempotent — the caller wants the row cleared and it already is.
Returning a generic ProcessingError amplified a successful-but-response-
lost Unspend into a hard failure inside validator.reverseSpends' 3-retry
loop.

- New typed ErrUtxoUnspent (errors/error.proto code 76) so callers detect
  this branch via errors.Is without parsing messages.
- Aerospike: split Lua dispatch into ERROR_CODE_SPEND_OWNERSHIP_UNSPENT
  and _MISMATCH, mapped to ErrUtxoUnspent and ProcessingError in Go.
- SQL: diagnostic-SELECT case-2 returns ErrUtxoUnspent.
- validator.reverseSpends short-circuits on errors.Is(err, ErrUtxoUnspent)
  — exactly-once behaviour covered by new
  TestValidator_ReverseSpends_UtxoUnspentIsIdempotent.
- Aerospike TestUnspendOwnership_MismatchOnUnspent fills the parity gap
  with the SQL test for that branch.
- qDiag race window documented in sql.go — best-effort message; safety
  guarantee lives in q1's AND spending_data = $3 condition.
The Lua script (teranode.lua) changed in this PR — both the ownership
check in unspend and the split into distinct SPEND_OWNERSHIP_UNSPENT /
_MISMATCH error codes. Bump the package identifier so Aerospike re-
registers the UDF on next startup instead of running the previously
cached v57 bytecode.

@oskarszoon oskarszoon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approve. Caller audit is complete — all 9 production callers populate SpendingData via GetSpends / SetConflicting / spendsForTx, the SQL and Lua backends have matching three-branch diagnostic semantics (NotFound / unspent / ownership-mismatch), and the validator's reverseSpends correctly treats ErrUtxoUnspent as idempotent-success for retry safety. DoS surface is net-positive (visible failures replace silent corruption). No new state-changing path was added.

Sonar quality gate now passes (97.5% new-code coverage, 0 hotspots) and the previously-failing sequential-postgres / sequential-aerospike checks are re-running — assuming they go green, this is mergeable as-is.

One optional follow-up (non-gating): the SQL q1+qDiag race window is acknowledged inline and the safety guarantee correctly comes from q1's AND spending_data = $3 atomicity — worst case is a stale error message, not a wrong action. Could later be tightened by combining into a single SELECT spending_data FROM outputs ... FOR UPDATE + branch. Optimisation, not correctness.

@ordishs ordishs requested a review from liam May 14, 2026 13:01
ProcessConflicting builds affectedParentSpends with the loser's SpendingData,
but the parent's stored spending_data is often nil (loser never actually
spent — winner did) or belongs to the winner (post-reorg). The strict
ownership check from the previous commits erred on these cases, breaking
sequential-postgres and sequential-aerospike conflict-resolution tests
(double-spend forks, grandparent conflicts, deep chain conflicts).

The safety property is "never wipe a spend we don't own", not "error on
every no-op". This change keeps the match-on-spending_data guard but makes
the non-matching paths a silent success — the stored value is unchanged
either way.

- Aerospike Lua: gate the utxo clear on callerOwnsSpend; return STATUS_OK
  without bin mutation when stored is nil or differs. setDeleteAtHeight +
  aerospike:update still run for housekeeping.
- SQL: q1's AND spending_data = $3 still gates the clear. On 0 rows, look
  up transaction_id directly so the locked + DAH housekeeping still runs.
  Only a missing row is NotFoundError.
- Bump LuaPackage v58 → v59 (Lua content changed).
- Drop now-dead validator.reverseSpends ErrUtxoUnspent short-circuit —
  Unspend never returns that error anymore.
- Drop the now-unused LuaErrorCodeSpendOwnership* constants and the
  matching ERROR_CODE / ERR_ locals in the Lua module.
- Rewrite TestUnspendOwnership_Mismatch and _MismatchOnUnspent on both
  backends to assert no error + stored data unchanged. Remove the
  now-redundant TestValidator_ReverseSpends_UtxoUnspentIsIdempotent.

ErrUtxoUnspent / NewUtxoUnspentError / proto code UTXO_UNSPENT=76 are
kept as unused-but-available API; no caller currently emits them.
@sonarqubecloud

Copy link
Copy Markdown

@ordishs ordishs merged commit 3972117 into bsv-blockchain:main May 19, 2026
25 checks passed
icellan added a commit that referenced this pull request May 19, 2026
Brings in fff6d3e..origin/main (4 commits since previous merge):
  - #897 blockassembly columnar->row fallback on Unimplemented
  - #896 asset on-demand subtree-data admission control + 503/Retry-After
  - #766 utxo: verify spend ownership in Unspend
  - #700 health: disk space check

Conflict resolutions:
  - services/blockassembly/Client.go, util/http.go, util/http_test.go:
    take theirs. Main has the post-Copilot-fix versions of these files
    from #896 and #897 (parseRetryAfter strict-Atoi, debug log on
    columnar fallback, extended TestParseRetryAfter cases) — they
    supersede the pre-fix versions still living on pr-828.
  - stores/utxo/aerospike/un_spend.go: take ours (native-op call,
    no SpendingData arg). The ownership check #766 added to the UDF
    path requires a coordinated update to the BSV-forked aerospike-
    server's subOpUnspend dispatcher. Until that lands, the
    native-op path on this branch skips ownership verification.
    Tracking issue filed.
icellan added a commit that referenced this pull request May 19, 2026
Two related items from the external security review:

1. The aerospike_use_native_teranode_ops longdesc previously said "Safe
   to enable on mixed clusters: capability probe falls back to UDF when
   the fork is absent". That's misleading — the probe writes a single
   key whose digest pins it to one partition, so a mixed-version
   cluster can produce false-positive (probe lands on a BSV-fork
   master, runtime writes to partitions still on stock Aerospike fail
   with PARAMETER_ERROR) or false-negative (cached for Store lifetime
   even after the cluster finishes upgrading). Rewrite the rolling-
   deploy section to be explicit, drop the "Safe to enable" claim, and
   tell operators to keep the setting false until the rolling upgrade
   completes.

2. Probe key was a fixed string ("_teranode-native-op-probe") shared
   across every teranode pointing at the same cluster. Two instances
   booting concurrently could interleave PutBins / Operate / Get
   sequences and observe each other's writes, producing a false-
   negative for one of them. Add PID + time.Now().UnixNano() to the
   key so every process gets its own; the 60s TTL still bounds the
   record's residency if Delete fails.

Follow-ups still tracked in the PR body: per-call PARAMETER_ERROR ->
UDF fallback in teranodeBatchRecord (separate PR), and the un_spend.go
SpendingData ownership check landed for the UDF path in #766 but is
not yet wired into subOpUnspend on the native-op side — issue #899.
icellan added a commit that referenced this pull request May 19, 2026
…ranch

The merge from main (#766: verify spend ownership in Unspend) needed two
catch-up changes on this branch's native-op-migrated unspendLua:

1. executeTeranodeOp now receives spend.SpendingData.Bytes() as a third
   arg + a nil-guard. The UDF fallback path (useNativeTeranodeOps=false,
   default) forwards the arg verbatim into client.Execute, so the
   ownership check the Lua "unspend" added in #766 works end-to-end.
   The native operate-path forwards the arg over the wire too, but the
   BSV-forked server's subOpUnspend dispatcher must be updated before
   the check is actually applied server-side — tracked in #899.

2. unspendLua's response-handling branch for LuaStatusError now
   distinguishes res.ErrorCode == LuaErrorCodeTxNotFound and returns
   errors.NewNotFoundError. Without this branch, #766's
   TestUnspendOwnership_NotFound saw the Lua-side "TX not found"
   response wrapped as StorageError. The merge conflict landed at the
   call site only; this response-handling change from main slipped past.
icellan added a commit that referenced this pull request Jun 9, 2026
The native subOpUnspend=3 dispatcher in the BSV fork of aerospike-server now
enforces the #766 SpendingData ownership check. Update the stale un_spend.go
comment that said the dispatcher still needed the fix, document the minimum
server-build requirement in the UseNativeTeranodeOps longdesc, and drop the
private-repo reference from the native_op.go header comment.
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.

3 participants