feat(wallet): broadcast/rollback semantics for create_action (F8.13)#375
feat(wallet): broadcast/rollback semantics for create_action (F8.13)#375
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…apter (#370) Implements both methods in MemoryStore, FileStore, and PostgresStore. Adds shared conformance examples covering status update, not-found errors, delete, and isolation. Extends RuboCop AllowedMethods for delete_action. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unify the auto_fund non-no_send path through pending state before any broadcast. When a broadcaster is configured, inputs and change are kept :pending until broadcast succeeds; on failure, rollback releases inputs back to :spendable, deletes phantom change outputs, and marks the action as 'failed'. Without a broadcaster, state is promoted immediately (backwards-compatible behaviour). Extracts shared rollback_pending_action helper used by both abort_action and the broadcast-failure path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Accepts the accept_delayed_broadcast option in create_action and sign_action. false (default): synchronous behaviour unchanged. true: falls through to synchronous processing, stores action as 'unproven', and emits a warning that background broadcasting is a planned future feature. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add arc_status attribute to BroadcastError so the wallet can distinguish
DOUBLE_SPEND_ATTEMPTED from REJECTED/INVALID/MALFORMED. Add broadcast_status_for
helper in WalletClient that maps BroadcastError to ReviewActionResultStatus strings
('doubleSpend', 'invalidTx', 'serviceError'). Include :broadcast_status in the
response hash on both success ('success') and failure, and propagate :competing_txs
from BroadcastResponse when present.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
) Adds send_with option to create_action, allowing callers to batch-broadcast previously no_send transactions. Each tx is broadcast independently with per-tx promote-on-success / rollback-on-failure semantics. Returns send_with_results array matching TS SDK SendWithResult[] shape. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR updates BSV::Wallet::WalletClient#create_action to avoid “phantom UTXO” state when broadcasting fails by introducing pending-first persistence and explicit rollback/promotion behavior, and it extends the storage adapter surface area to support action status updates/deletion.
Changes:
- Add optional
broadcaster:support toWalletClientand introduce broadcast-before-promote flow with rollback on failures. - Extend
StorageAdapterwithupdate_action_status/delete_actionand implement across Memory/File/Postgres stores. - Add/expand specs covering broadcaster integration, rollback behavior,
send_with, andaccept_delayed_broadcaststub behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| gem/bsv-wallet/lib/bsv/wallet_interface/wallet_client.rb | Implements pending-first storage, broadcast+rollback semantics, send_with, and delayed-broadcast stub behavior. |
| gem/bsv-wallet/lib/bsv/wallet_interface/storage_adapter.rb | Adds update_action_status and delete_action to the adapter contract. |
| gem/bsv-wallet/lib/bsv/wallet_interface/memory_store.rb | Implements new action mutation APIs in the in-memory adapter. |
| gem/bsv-wallet/lib/bsv/wallet_interface/file_store.rb | Persists new action mutation APIs to disk. |
| gem/bsv-wallet-postgres/lib/bsv/wallet_postgres/postgres_store.rb | Implements new action mutation APIs for Postgres-backed storage. |
| gem/bsv-sdk/lib/bsv/network/broadcast_error.rb | Adds arc_status to carry ARC txStatus through exceptions. |
| gem/bsv-sdk/lib/bsv/network/arc.rb | Propagates ARC txStatus into BroadcastError as arc_status for mapping. |
| gem/bsv-wallet/spec/support/shared_examples_for_storage_adapter.rb | Adds shared adapter specs for action status updates and deletion. |
| gem/bsv-wallet/spec/bsv/wallet_interface/wallet_client_spec.rb | Adds specs for broadcaster initialization and delayed-broadcast option handling. |
| gem/bsv-wallet/spec/bsv/wallet_interface/wallet_client_auto_fund_spec.rb | Adds end-to-end auto-fund tests for broadcaster success/failure rollback and send_with. |
| .rubocop.yml | Allows delete_action in predicate naming exceptions. |
| # Store everything in pending state first — inputs are already locked | ||
| # as :pending by lock_utxos above, so we match that discipline here. | ||
| store_action(tx, args, status: 'pending') | ||
| store_change_outputs(txid, tx, change_outputs, tx_hex) | ||
| store_tracked_outputs(txid, tx, caller_outputs) | ||
|
|
||
| # Promote from :pending to :spent now that all storage writes are done. | ||
| selected_outpoints.each { |op| @storage.update_output_state(op, :spent) } | ||
| # Record change outpoints and mark them :pending so a concurrent | ||
| # create_action cannot double-spend them before broadcast completes. | ||
| change_outpoints = [] | ||
| change_outputs.each do |spec| | ||
| idx = tx.outputs.index { |o| o.instance_variable_get(:@_spec).equal?(spec) } | ||
| next unless idx | ||
|
|
||
| op = "#{txid}.#{idx}" | ||
| change_outpoints << op | ||
| @storage.update_output_state(op, :pending, pending_reference: fund_ref) | ||
| end |
There was a problem hiding this comment.
In the non-no_send path, change outputs are initially stored with state: :spendable (store_change_outputs) and then flipped to :pending. This leaves a TOCTOU window where concurrent create_action calls can select the new change output before broadcast completes. Prefer storing change outputs directly as :pending (and recording pending_reference) so they are never briefly spendable.
|
|
||
| if broadcast_enabled? | ||
| broadcast_and_promote( | ||
| tx, txid, selected_outpoints, change_outpoints, fund_ref, beef_binary | ||
| ) | ||
| else | ||
| # No broadcaster configured — promote immediately and return BEEF | ||
| # for the caller to broadcast (backwards-compatible behaviour). | ||
| selected_outpoints.each { |op| @storage.update_output_state(op, :spent) } | ||
| change_outpoints.each { |op| @storage.update_output_state(op, :spendable) } | ||
|
|
||
| final_status = if args.dig(:options, :accept_delayed_broadcast) |
There was a problem hiding this comment.
accept_delayed_broadcast: true is handled in finalize_action and in the no-broadcaster path, but it is ignored when broadcast_enabled? is true: broadcast_and_promote always marks the action 'completed' on success (and 'failed' on error) with no warning. If accept_delayed_broadcast is meant to be a stub that still stores 'unproven', this branch should apply the same status/warning behavior (or skip synchronous broadcast) for consistency.
| if broadcast_enabled? | |
| broadcast_and_promote( | |
| tx, txid, selected_outpoints, change_outpoints, fund_ref, beef_binary | |
| ) | |
| else | |
| # No broadcaster configured — promote immediately and return BEEF | |
| # for the caller to broadcast (backwards-compatible behaviour). | |
| selected_outpoints.each { |op| @storage.update_output_state(op, :spent) } | |
| change_outpoints.each { |op| @storage.update_output_state(op, :spendable) } | |
| final_status = if args.dig(:options, :accept_delayed_broadcast) | |
| accept_delayed_broadcast = args.dig(:options, :accept_delayed_broadcast) | |
| if broadcast_enabled? && !accept_delayed_broadcast | |
| broadcast_and_promote( | |
| tx, txid, selected_outpoints, change_outpoints, fund_ref, beef_binary | |
| ) | |
| else | |
| # No broadcaster configured, or delayed broadcast was requested — | |
| # promote immediately and return BEEF for the caller to broadcast | |
| # (backwards-compatible behaviour for the current stub). | |
| selected_outpoints.each { |op| @storage.update_output_state(op, :spent) } | |
| change_outpoints.each { |op| @storage.update_output_state(op, :spendable) } | |
| final_status = if accept_delayed_broadcast |
| def broadcast_and_promote(tx, txid, input_outpoints, change_outpoints, fund_ref, beef_binary) | ||
| broadcast_result = @broadcaster.broadcast(tx) | ||
|
|
||
| # Broadcast succeeded — promote all pending state to final. | ||
| input_outpoints.each { |op| @storage.update_output_state(op, :spent) } | ||
| change_outpoints.each { |op| @storage.update_output_state(op, :spendable) } | ||
| @storage.update_action_status(txid, 'completed') | ||
|
|
||
| result = { txid: txid, tx: beef_binary.unpack('C*'), broadcast_result: broadcast_result, broadcast_status: 'success' } | ||
| result[:competing_txs] = broadcast_result.competing_txs if broadcast_result.competing_txs | ||
| result | ||
| rescue StandardError => e | ||
| rollback_pending_action(input_outpoints, change_outpoints, txid, fund_ref, action_status: 'failed') | ||
| { txid: txid, tx: beef_binary.unpack('C*'), broadcast_error: e.message, broadcast_status: broadcast_status_for(e) } | ||
| end |
There was a problem hiding this comment.
broadcast_and_promote rescues StandardError around both the broadcast call and the subsequent state promotion. If a non-broadcast exception occurs during promotion (e.g., update_output_state/action_status fails), it will be reported as a broadcast failure and rollback_pending_action will not revert any inputs already promoted to :spent (because release_pending_utxos only unlocks :pending). This can leave the wallet in a phantom-spent state even if the broadcast succeeded. Consider only rescuing broadcaster-related errors (e.g., BroadcastError/network exceptions) and allowing storage/promotion errors to raise (or handling them separately with a reliable compensation strategy).
| # Batch-broadcasts a list of previously no_send transactions. | ||
| # | ||
| # Each txid must correspond to a pending no_send entry registered in | ||
| # +@pending_by_txid+. Transactions are broadcast individually so that one | ||
| # failure does not block the others. On per-tx success the inputs are | ||
| # promoted to +:spent+ and change outputs to +:spendable+. On failure the | ||
| # pending state is rolled back via +rollback_pending_action+. | ||
| # | ||
| # @param txids [Array<String>] txids of no_send transactions to broadcast | ||
| # @return [Array<Hash>] per-tx results matching TS SDK +SendWithResult[]+ shape: | ||
| # +{ txid: String, status: 'unproven' | 'failed' }+ | ||
| def broadcast_send_with(txids) | ||
| txids.map { |txid| broadcast_single_no_send(txid) } | ||
| end |
There was a problem hiding this comment.
send_with is described as a batched broadcast (ARC#broadcast_many), but broadcast_send_with currently iterates txids and calls @broadcaster.broadcast per tx. If batching is required for cross-SDK compliance/performance, consider using broadcast_many when the broadcaster supports it (and still apply per-tx promotion/rollback based on each returned result), or update the contract/docs to reflect per-tx broadcasting.
| ds = @db[:wallet_actions].where(txid: txid) | ||
| raise WalletError, "Action not found: #{txid}" if ds.empty? | ||
|
|
||
| ds.update( | ||
| data: Sequel.lit( | ||
| "data || jsonb_build_object('status', ?)", | ||
| new_status | ||
| ) | ||
| ) | ||
| symbolise_keys(ds.first[:data]) |
There was a problem hiding this comment.
PostgresStore#update_action_status filters by txid, but wallet_actions.txid is not unique in the initial schema migration, so this UPDATE will modify all matching rows. That can make action status updates ambiguous if duplicates exist (e.g., retries) and ds.first may not correspond to the intended action. Consider enforcing uniqueness on wallet_actions.txid (migration + conflict handling) or updating only the latest row (by id) for the given txid.
| ds = @db[:wallet_actions].where(txid: txid) | |
| raise WalletError, "Action not found: #{txid}" if ds.empty? | |
| ds.update( | |
| data: Sequel.lit( | |
| "data || jsonb_build_object('status', ?)", | |
| new_status | |
| ) | |
| ) | |
| symbolise_keys(ds.first[:data]) | |
| action = @db[:wallet_actions] | |
| .where(txid: txid) | |
| .order(Sequel.desc(:id)) | |
| .first | |
| raise WalletError, "Action not found: #{txid}" unless action | |
| @db[:wallet_actions] | |
| .where(id: action[:id]) | |
| .update( | |
| data: Sequel.lit( | |
| "data || jsonb_build_object('status', ?)", | |
| new_status | |
| ) | |
| ) | |
| updated_data = @db[:wallet_actions] | |
| .where(id: action[:id]) | |
| .get(:data) | |
| symbolise_keys(updated_data) |
…ency, finalize_action 1. send_with uses the original signed tx from pending entry (preserves source_satoshis/source_locking_script for EF submission) instead of reconstructing from stored hex which loses that metadata 2. promote_no_send sets action status to 'unproven' (matching the returned SendWithResult status) instead of 'completed' 3. finalize_action now uses the same pending→broadcast→promote/rollback discipline as auto_fund_and_create when a broadcaster is configured Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Fixes the broadcast/rollback semantics in
create_actionto prevent phantom UTXOs when broadcasts fail (F8.13 cross-SDK compliance).broadcaster:parameter to WalletClient (duck-typed, any object with#broadcast) (feat(wallet): add broadcaster: parameter to WalletClient #369)delete_actionandupdate_action_statusto StorageAdapter — implemented in MemoryStore, FileStore; PostgresStore flagged for downstream update (feat(wallet): add delete_action and update_action_status to StorageAdapter #370)no_sendpath now stores state as:pendingfirst, broadcasts via configured broadcaster, promotes on success, rolls back on failure — no more phantom UTXOs (feat(wallet): unify auto_fund non-no_send path through pending state #371)ReviewActionResultStatus(success/doubleSpend/invalidTx/serviceError) and returned in the result hash, not raised (feat(wallet): return broadcast result in create_action response #372)send_withbatches previouslyno_sendtransactions for broadcast, with per-tx rollback on failure (feat(wallet): implement send_with for batched no_send transactions #373)accept_delayed_broadcastoption accepted (stub — defaults tofalse, background processing deferred to follow-up) (feat(wallet): accept accept_delayed_broadcast option (stub) #374)Acceptance Criteria Verification
create_action(nonoSend) broadcasts before promoting state:spendable, phantom change deletedacceptDelayedBroadcastoption accepted (defaults tofalse)noSend: truepath unchangedsendWithbatching works with previouslynoSendtransactionsReviewActionResultStatusTest Results
Test Plan
bundle exec rake— full suite passes (4,200 examples, 0 failures)Closes #368
Generated with Claude Code