🤖 Claude Code Review

Status: Complete

Current Review:
No issues found. All previously identified concerns have been addressed:

• ✅ Nil UTXOHash handling: Fixed in repository.go:920 - now safely logs TxID:Vout instead of dereferencing UTXOHash
• ✅ Documentation accuracy: swagger_routes.go:384 correctly states "default 100MB" matching asset_reference.md:788
• ✅ Test coverage: Added TestGetUtxoNilUTXOHash to verify nil-hash path works with real Repository

Summary:
This PR successfully optimizes UTXO lookups by:

1. Removing redundant transaction fetches from GetUTXO (eliminates full tx parse per lookup)
2. Adding bulk endpoint POST /api/v1/utxos with bounded concurrency (1024 goroutines)
3. Making store implementations tolerant of nil UTXOHash when caller provides (txid, vout) primary key

The implementation is solid, well-tested, and follows the project verification workflow from AGENTS.md. Documentation accurately describes the new bulk endpoint behavior including the known lockTime semantic divergence between SQL and Aerospike backends.

[Minor] Error format mismatch in GetUTXOs.go line 142

The format string "error getting utxo %s:%d" has 2 placeholders but 3 arguments are provided (txHash.String(), vout, err). The error will be wrapped but not shown in the message.

Suggested fix:
errors.NewProcessingError("error getting utxo %s:%d: %v", txHash.String(), vout, err)

This matches the pattern used elsewhere in the codebase (see stores/utxo/aerospike/spend.go:194 for similar usage).

Benchmark Comparison Report

Baseline: main (unknown)

Current: PR-950 (192db50)

Summary

• Regressions: 0
• Improvements: 0
• Unchanged: 144
• Significance level: p < 0.05

Threshold: >10% with p < 0.05 | Generated: 2026-05-27 16:55 UTC

Review

Clean design overall — the binary layout is well-documented, the store nil-UTXOHash tolerance and the repo logging fix are correct and necessary, and test coverage is broad (order preservation, mixed statuses, nil-hash assertions, HEX/JSON modes, the real-repo nil-log test). One critical, remotely-triggerable crash blocks merge.

🔴 CRITICAL — Unauthenticated DoS: out-of-range vout panics the aerospike store and crashes the asset server

Both new paths now pass a client-controlled vout straight to GetSpend with no bounds check. The aerospike store indexes the per-batch UTXO list without validating the offset:

stores/utxo/aerospike/get.go:186

b, ok := utxos[spend.Vout%uint32(s.utxoBatchSize)].([]byte)

• CalculateKeySource(hash, vout, 128) returns num = vout/128, so every vout in 0..127 maps to the same batch-0 record, which exists for every tx (util/uaerospike/client.go:505).
• The Utxos bin is utxos[start:end], sized to the actual output count, not padded (stores/utxo/aerospike/create.go:574-577). A 2-output tx stores a 2-element list.
• So utxos[vout%128] with e.g. vout=127 on a 2-element list → index out of range panic.

This indexing is pre-existing but was previously unreachable with bad vouts: the only HTTP caller (GetUTXO) validated voutUint32 >= uint32(len(tx.Outputs)) → 404 before hitting the store. This PR removes that guard and adds a second unvalidated path.

Impact differs by endpoint — and the bulk one is fatal:

Full chain for the bulk path has no recover anywhere: GetUTXOs goroutine → repository.GetUtxo (repository.go:911) → aerospike GetSpend (panics). Echo's recover middleware only protects the request goroutine, not the ones errgroup spawns.

Reproduction (unauthenticated, one request):

POST /api/v1/utxos
body = <32-byte txid of ANY tx with <128 outputs> || <uint32 LE vout in (numOutputs, 127]>

The coinbase of any block (1 output, well-known txid) with vout=1 suffices.

Required fix (root cause, protects all callers, minimal): bounds-check the offset in aerospike/get.go and treat overflow as not-found, mirroring the existing ErrKeyNotFound branch:

idx := spend.Vout % uint32(s.utxoBatchSize)
if int(idx) >= len(utxos) {
    return &utxo.SpendResponse{Status: int(utxo.Status_NOT_FOUND)}, nil
}
b, ok := utxos[idx].([]byte)

SQL is already safe (WHERE ... AND o.idx = $2 → ErrNoRows → Status_NOT_FOUND, sql.go:3303-3310), so the gap is aerospike-only (production).

Strongly recommend additionally isolating panics in the errgroup body so any future store-level panic degrades to 500 instead of taking down the process — a public fan-out endpoint should never let a per-record panic escape the goroutine.

Test gap that hid this: every GetUTXOs/GetUTXO test mocks the repository, so "Vout out of range" returns a mocked Status_NOT_FOUND and never exercises the real store indexing. Per the repo's testing rule (use sqlitememory, don't mock the store), an out-of-range-vout test against a real store would have caught the aerospike-vs-SQL divergence. Please add one.

🟡 Minor

• 413 can be masked as 500. GetUTXOs reads the body via io.ReadAll (GetUTXOs.go:493). Echo's BodyLimit returns 413 up-front only via the Content-Length check; a client streaming without an accurate Content-Length trips the limit inside Read, so io.ReadAll returns ErrStatusRequestEntityTooLarge and the handler wraps it as 500. Common clients send Content-Length so the documented 413 holds for them — consider detecting echo.ErrStatusRequestEntityTooLarge and surfacing 413.
• Behavior change (already flagged by author): "tx not found" vs "output index out of range" now collapse to NOT_FOUND (3): UTXO not found. Documented, status code unchanged — just confirm no internal client greps the old message strings.
• Error counter only on success. prometheusAssetHTTPGetUTXOs is incremented only on the 200 path; 400/413/500 record nothing. Mirrors existing GetUTXO, so the bulk endpoint's failure rate will be invisible.

🟢 Nits

• i := i (GetUTXOs.go:516) is dead since Go 1.22; go.mod is go 1.26.0.
• writeUTXOsRecord doc says [...4 vin][32 txid]; PR body / asset_reference.md say "spendingVin"/"spendingTxID" — same thing, align wording.

Verdict

Request changes. Endpoint design, docs, and test breadth are strong. But removing the vout < len(outputs) guard exposes a pre-existing unchecked slice index in the aerospike store, and routing it through errgroup goroutines turns it into a one-request remote crash of the asset server.

Before merge:

1. Bounds-check the offset in aerospike/get.go (return Status_NOT_FOUND).
2. Add panic isolation around the errgroup per-record body.
3. Add a real-store (sqlitememory + aerospike) out-of-range-vout test.
4. (Optional) map ErrStatusRequestEntityTooLarge from io.ReadAll to 413.

Thanks for the careful review @ordishs — the panic chain is exactly as you described and the test gap is a fair critique. Pushed 287eb7085 addressing every item.

Critical fixes

1. Aerospike bounds-check. stores/utxo/aerospike/get.go:186 now guards idx >= len(utxos) and returns Status_NOT_FOUND, mirroring the existing ErrKeyNotFound branch. SQL was already safe.
2. Errgroup panic isolation. Each GetUTXOs goroutine body now runs under a deferred recover() that converts any panic into a 500 errgroup error and logs the stack trace. A per-record panic can no longer escape the request goroutine, regardless of which store backend is in use.
3. Real-store regression tests. Added:
   • stores/utxo/sql/sql_test.go::TestGetSpendNilUTXOHash/out-of-range_vout_returns_NOT_FOUND (sqlitememory, runs locally).
   • stores/utxo/aerospike/aerospike_server_test.go::TestAerospike/aerospike_get_spend_out_of_range_vout (testcontainers, runs in CI).
   • services/asset/httpimpl/GetUTXOs_test.go::TestGetUTXOs/panic_in_goroutine_becomes_500_... — mocks the repo to panic and asserts 500, locking the panic-isolation contract.

Minor

• 413 mapping. io.ReadAll now detects echo.ErrStatusRequestEntityTooLarge and surfaces 413 explicitly, so streaming clients without Content-Length get the documented status instead of a generic 500.
• Counter only on success. Acknowledged — mirrors GetUTXO and other handlers. Happy to add failure label-vector counters as a follow-up if you'd like; left out of this PR to keep parity with the rest of the asset surface.
• Message-string behaviour change. Confirmed no internal client greps the old strings.

Nits

• i := i dropped.
• writeUTXOsRecord doc comment now uses spendingVin/spendingTxID, matching the PR description and asset_reference.md.

Re @oskarszoon's items

• LockTime divergence. Pre-existing and non-trivial: SQL returns coinbaseSpendingHeight; Aerospike's stored LockTime bin is the tx's nLockTime (stores/utxo/aerospike/create.go:714) — semantically different. Documented as a known gap in asset_reference.md; reconciling the two stores is out of scope for this PR.
• Anonymous + unrated endpoint. Agreed this needs the auth/rate-limit work from feat: add asset service rate limiting, peer authentication and request visibility #643. Will open a tracking issue referencing the asset_httpHeavy style gating once feat: add asset service rate limiting, peer authentication and request visibility #643 lands.

Re the github-actions bot's "error format mismatch"

The format string "error getting utxo %s:%d" with three args (txHash, vout, err) is the project's standard pattern, not a bug: errors.New strips a trailing error argument and attaches it as the wrapped error (errors/errors.go:328-337), then formats the remaining params. The wrapped error shows in the chain via %w-style unwrap, not via %v interpolation. Same pattern in stores/utxo/aerospike/spend.go etc.

Re-review — all blocking feedback addressed ✅

Re-reviewed at 830258f26 (fix commit 287eb7085, "prevent remote panic via out-of-range vout"). Every point from the previous review is resolved, and the fix matches the recommended approach.

🔴 Critical DoS — fixed at the root

stores/utxo/aerospike/get.go now bounds-checks the batch offset before indexing and returns NOT_FOUND on overflow, mirroring the ErrKeyNotFound branch:

idx := spend.Vout % uint32(s.utxoBatchSize)
if int(idx) >= len(utxos) {
    return &utxo.SpendResponse{Status: int(utxo.Status_NOT_FOUND)}, nil
}
b, ok := utxos[idx].([]byte)

Verified correct across single-batch, cross-batch, and exact-boundary vouts (e.g. 200-output tx: vout=199→valid, vout=200→NOT_FOUND via offset guard, vout=300→NOT_FOUND via missing record). SQL was already safe; both stores now have explicit coverage.

🟢 Defense-in-depth — added

GetUTXOs.go wraps each errgroup goroutine body in a recover() that logs debug.Stack() and converts any panic into a 500, so a future store-level panic degrades the request instead of taking down the process — exactly the isolation a public fan-out endpoint needs.

🟢 413 mapping — added and confirmed live

io.ReadAll now maps echo.ErrStatusRequestEntityTooLarge → 413. I checked that teranode/errors.Is delegates to stdlib errors.Is (errors/errors.go:903), so the foreign echo sentinel actually matches — not dead code. The documented 413 contract now holds for both Content-Length and chunked clients.

🟢 Test gap — closed at all three levels

• stores/utxo/aerospike/aerospike_server_test.go → aerospike_get_spend_out_of_range_vout (the real production store; would have panicked pre-fix).
• stores/utxo/sql/sql_test.go → out-of-range vout returns NOT_FOUND, not error or panic.
• GetUTXOs_test.go → panic in goroutine becomes 500 instead of crashing the process (forces a mock panic, asserts 500).

The i := i nit was also dropped (Go 1.26 loop semantics).

Verification I ran

• go test ./stores/utxo/sql/ → PASS, including all three TestGetSpendNilUTXOHash subcases (nil-skips-check, wrong-hash-fails, out-of-range→NOT_FOUND).
• The httpimpl and repository/aerospike tests I could not run in my environment — they transitively import the gobdk cgo module and my local Xcode license isn't accepted (cgo-on blocked, cgo-off excludes gobdk). Reviewed those by inspection; logic is sound. Worth a glance at CI to confirm the -race run is green on services/asset/httpimpl/... and the aerospike integration suite.

Non-blocking

• The 413 read-path branch has no test (hard to exercise without a reader that lies about Content-Length) — optional.
• prometheusAssetHTTPGetUTXOs is still only incremented on success, so the bulk endpoint's failure rate is invisible — pre-existing pattern, fine to defer.

LGTM once CI is green. Nice, thorough turnaround.

Addressed the latest bot/Sonar findings in 344d159.

Fixed

• github-actions[bot] inline (swagger_routes.go:384) — `default 4MB` → `default 100MB`. The merge from main brought in PR feat: add asset service rate limiting, peer authentication and request visibility #643's global `asset_httpBodyLimit` (100MB); I'd missed updating the swagger annotation to match `asset_reference.md`.
• SonarCloud CRITICAL — `GetUTXOs` cognitive complexity 31 (limit 15). Extracted two helpers, no behaviour change:
  • `readUTXOsBody(c)` — body read + 413 mapping + length-multiple validation.
  • `fetchOneUTXO(ctx, txHash, vout)` — per-record store lookup with the ErrNotFound / nil-resp / wrapped-error semantics.
    The panic-recover defer stays inline in the goroutine wrapper because it needs the closure variables (`txHash`, `vout`, `retErr`).

Flagging as false positive (please confirm)

• SonarCloud 2× MAJOR — "Filename does not follow snake_case.go convention" on `GetUTXOs.go` and `GetUTXOs_test.go`. Every `Get*.go` file under `services/asset/httpimpl/` (`GetUTXO.go`, `GetTransaction.go`, `GetBlock.go`, `GetBlockHeader.go`, ...) is CamelCase; my files match the established directory convention. Happy to rename if you'd prefer to align with Sonar (would need to rename the existing dozen+ files too for consistency).

All 10 `TestGetUTXOs` subtests pass under `-race`; `make lint-new` clean.

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE