Skip to content

fix: wait for broadcast result - #113

Open
ovitrif wants to merge 28 commits into
chore/untrack-jni-drop-jvmfrom
codex/112-onchain-broadcast-result
Open

fix: wait for broadcast result#113
ovitrif wants to merge 28 commits into
chore/untrack-jni-drop-jvmfrom
codex/112-onchain-broadcast-result

Conversation

@ovitrif

@ovitrif ovitrif commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Closes #112

Summary

  • Wait for the configured backend result before explicit on-chain sends report acceptance.
  • Return transaction-keyed rejected, not-dispatched, acceptance-unknown failure, and acceptance-unknown timeout errors through Rust, Swift, Kotlin, and Python.
  • Persist every possibly dispatched transaction and its complete RBF lineage before broadcast, reserve its inputs across restart, expose pending transaction IDs, and support exact-byte rebroadcast or explicit abandonment after external reconciliation.
  • Derive abandonment timestamps inside the aggregate wallet from wall time and tracked observation/eviction state; overflow fails before mutation.
  • Keep LDK-managed broadcasts fire-and-forget on a dedicated priority queue.
  • Require backend evidence before reconciliation, preserve only the canonical payment record, and make confirmation cleanup crash-safe across BDK, payment-store, and intent-store persistence failures.
  • Classify Bitcoin Core, Electrum, and Esplora responses conservatively, including already-known acceptance and returned-txid mismatch rejection.
  • Publish the breaking binding contract as 0.7.0-rc.67; PR fix: sigabrt runtime teardown #111 reserves rc.66.

Preview

N/A — no user-visible changes.

QA Notes

  • A deterministic backend rejection does not reach the consumer success path and does not create sent activity.
  • A timeout or operational failure after dispatch carries the original transaction ID. Consumers must reconcile or call rebroadcast_transaction for that exact ID; they must not create a second payment.
  • list_pending_broadcasts enumerates unresolved IDs. abandon_pending_broadcast is valid only after the caller proves the entire payment lineage absent from mempool and chain.
  • Swift unit NodeError cases no longer carry the legacy generated message value. The four broadcast-result cases carry txid.
  • Kotlin unit NodeException cases now have empty generated messages. The four broadcast-result cases carry txid.

Validation

  • Exact PR head: 0bc67a976ad707f69a5d3f1602c3b8c0fdaee859; local HEAD and upstream match; worktree clean.
  • Local verification: cargo fmt --check, git diff --check.
  • cargo build --all-targets --locked: passed. Existing target-name, unused-import, and unused-variable warnings remain outside this change.
  • cargo test --lib: 131 passed.
  • cargo test --features uniffi --lib: 141 passed; only the repository's existing UniFFI warnings were emitted.
  • cargo test --doc: 13 passed.
  • cargo test --features uniffi --doc: 13 passed.
  • cargo test --test multi_address_types_tests rbf::test_rbf_cross_wallet_transaction -- --exact --nocapture: passed against real bitcoind and electrs.
  • onchain_transaction_evicted_event: accepted-send indexing and pending-intent assertions passed; the final eviction injection is guarded and skipped because the bundled Bitcoin Core lacks removetx.
  • The dedicated splice integration test reaches the same pre-existing On-chain transaction signing failed path and wait on both base 03ccac798aeb907c6dda367fa4c8af6e3eaa54fa and this branch.
  • Generated Swift, Kotlin Android, and Python sources remain committed and match bindings/ldk_node.udl (list_pending_broadcasts returns PendingBroadcastInfo).
  • Generated Swift archive SHA256: d6ccac3a6a013fbb5313d49faf4d3373b14053ede91923e9a1cd4f3230c6848c; Package.swift matches.

Consumer validation

  • iOS branch e72a87566f672c14f4b7b5a32f72cb6615262270 compiled against the rc.67 API-equivalent XCFramework before the internal timestamp-only follow-up; final bindgen regenerated only native runtime artifacts and left generated Swift unchanged. After the expected enum-payload compatibility adaptation, Accepted, rejected, failed, timed-out, not-dispatched, maximum-send, and wrapped-error tests passed: 7 passed, 0 failed, 0 skipped.
  • Android branch e733f9c4ed4dbed701a640fe58c3799717c77b0b compiled against the rc.67 API-equivalent Maven AAR before the internal timestamp-only follow-up; final bindgen regenerated only native runtime artifacts and left generated Kotlin unchanged. Repository and UI accepted/rejected send-path tests passed: 5 passed.
  • Earlier live consumer journeys proved the product behavior end to end: accepted iOS tx 24b2dd80ebdca0610fdc2e90b589b810bd9fd2781cc33de8a55b63e6020b44f9 and Android tx bd2fb7e01eb49fefc70fc46be9466d3f209999c1668cdd9ec2aeaac1dfab703a reached mempool/activity/success; rejected iOS tx feeb000e379dc52d1c140f3fd8a01edfa7dfb29f12a8e670ab1b99a7a33f7b7a and Android tx 481280340d1e5507079c771de07ab8d0c13ace14203f6e0d508c9250ab09726b propagated RPC -26, stayed absent from mempool/chain/activity, and did not show success.
  • Both consumer worktrees were restored to their committed production rc.63 wiring and are clean. No consumer branch or artifact was published.

Stack

Release

  • No tag or release was created. rc.67 may be released from the fully pushed PR head only after a non-author approval, all required checks, and PR fix: sigabrt runtime teardown #111's rc.66 ordering are resolved. Add the release link here before merge.

@chatgpt-codex-connector

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@ovitrif
ovitrif marked this pull request as draft September 2, 2026 08:47
@ovitrif
ovitrif marked this pull request as ready for review September 2, 2026 12:33
chatgpt-codex-connector[bot]

This comment was marked as resolved.

@ovitrif

This comment was marked as resolved.

@chatgpt-codex-connector

This comment was marked as resolved.

Comment thread src/chain/electrum.rs Outdated
Comment thread src/wallet/mod.rs
Comment thread src/wallet/mod.rs Outdated
Comment thread src/payment/onchain.rs Outdated
Comment thread src/wallet/mod.rs Outdated
@ovitrif
ovitrif requested a review from ben-kaufman September 2, 2026 21:50
Comment thread bindings/ldk_node.udl
Comment thread src/chain/mod.rs
Comment thread src/wallet/mod.rs Outdated
@ovitrif
ovitrif requested a review from ben-kaufman September 3, 2026 22:37
Comment thread src/wallet/mod.rs
@ovitrif
ovitrif requested a review from ben-kaufman September 3, 2026 23:01
Comment thread src/payment/onchain.rs
@ovitrif
ovitrif force-pushed the codex/112-onchain-broadcast-result branch from 3b1f49f to 2f6d7c8 Compare September 4, 2026 14:51
@ovitrif
ovitrif force-pushed the codex/112-onchain-broadcast-result branch from 2f6d7c8 to 0bc67a9 Compare September 4, 2026 15:20
@ovitrif
ovitrif changed the base branch from main to chore/untrack-jni-drop-jvm September 4, 2026 15:20
@ovitrif ovitrif changed the title fix: wait for on-chain broadcast results fix: wait for broadcast results Sep 4, 2026
@ovitrif ovitrif changed the title fix: wait for broadcast results fix: wait for broadcast result Sep 4, 2026
@ovitrif ovitrif self-assigned this Sep 5, 2026
@ovitrif
ovitrif force-pushed the codex/112-onchain-broadcast-result branch from 0bc67a9 to 8e707af Compare September 5, 2026 01:57
@coreyphillips

Copy link
Copy Markdown
Collaborator

Two independent reviews.

needs changing before merge

  • RBF abandonment leaves predecessor inputs reserved (crates/bdk-wallet-aggregate/src/lib.rs:644). Abandoning a multi-transaction RBF lineage leaves predecessor inputs reserved. BDK snapshots canonical transaction IDs before apply_evicted_txs, so one call evicts B but makes A canonical too late to evict it. I reproduced abandon_txs(&[A, B]) returning Ok while find_tx(A) still returned A, then line 1275 deletes the only intent. Evict until no lineage member remains canonical before deleting the intent.
  • Abandonment can race a claimed broadcast (src/payment/onchain.rs:790). An intent can be abandoned while its exact transaction is already being sent. rebroadcast_transaction releases the intent lock before awaiting line 724, and abandonment checks no queued or claimed request. I held a claimed request, abandoned it, then returned backend Ok; both calls returned Ok after the inputs were released. Hold an intent dispatch lease through backend completion, including shutdown, and reject abandonment while held.
  • Intent cleanup skips unloaded wallet accounts (src/wallet/mod.rs:1261). Cleanup can delete an intent without updating a runtime-derived account that owns its inputs. I confirmed that runtime registrations are not persisted, restore_pending_broadcasts runs at src/builder.rs:1973 before callers can restore them, and AggregateWallet::abandon_txs visits only loaded wallets. A crash in Abandoning or Rejecting can leave that account's transaction on disk, then remove it from recovery state. Persist participating account keys and retain the intent until every account is durably updated.
  • Dropping Node can panic an in-progress send (src/payment/onchain.rs:145). An exported OnchainPayment can panic if its owning Node is dropped during transaction preparation. I blocked the intent-store write, dropped Node, then resumed it; the send panicked at src/runtime.rs:309 with NotRunning after persisting the intent. The RBF and rebroadcast paths make the same infallible call. Use try_block_on, clean up undispatched new transactions, and preserve existing rebroadcast intents.
  • RBF re-entry overwrites a retained lineage (src/wallet/mod.rs:377). An accepted lineage can lose a replacement when an older member re-enters the mempool. At line 377, an update that observes A and evicts active B is ignored; bumping A then misses the intent at line 1898 and line 1523 overwrites [A, B] with [A, C]. The existing branch test bypasses this path by calling resolve_broadcast_intents directly. Reactivate observed members of Accepted intents, merge RBF by any lineage member, and reject key collisions.
  • A slow explicit send stalls safety-critical broadcasts (src/chain/mod.rs:1342). A slow explicit Electrum send blocks the sole worker carrying safety-critical LDK transactions. Priority selection occurs only before dequeue, then line 1342 awaits the full retrying RPC. I confirmed the base released this worker at its 15-second timeout, while a claimed request here waits for backend completion. Move explicit result-bearing dispatch off the LDK worker.

worth doing, does not block

  • Any unclassified backend error locks the spent UTXOs with no automatic release (src/chain/electrum.rs:990). classify_electrum_broadcast_result (src/chain/electrum.rs:990) maps every non-Protocol electrum error to TxBroadcastError::Failed, including AllAttemptsErrored from an unreachable server, and process_broadcast_package returns Failed at electrum.rs:538 even when the runtime client does not exist and nothing was sent at all. dispatch_prepared_transaction keeps the intent on Failed (src/payment/onchain.rs:161) and reapply_unresolved_broadcasts (src/wallet/mod.rs:1446) re-pins the transaction into the BDK graph with a fresh last_seen on every sync, so the inputs stay reserved and the balance stays reduced indefinitely. There is no TTL. The only exits are a later backend observation (impossible if the transaction was never sent) or an explicit abandon_pending_broadcast, which the docs say requires proving absence from mempool and chain. On mobile, sending while the server is unreachable is common: today it leaves the wallet untouched, after this change the balance drops for good until the app implements a reconciliation flow. I confirmed the retention path by reading and via the PR's own failed_send_survives_eviction_restart_and_retry_until_accepted, which asserts the intent survives both eviction and restart. Smallest improvement: return NotDispatched where dispatch provably did not happen (electrum.rs:538 is unambiguous, nothing was submitted), so those release immediately. Related: classify_rpc_broadcast_error's "code":N substring test cannot match an electrum Protocol payload that carries the daemon error nested inside message, because Value::to_string() escapes the inner quotes. Servers of that shape fall back entirely to the message-marker list, so deterministic rejections outside it (too-long-mempool-chain, tx-size, absurdly-high-fee) also become Failed and lock funds.
  • Received events are lost across restart (src/payment/onchain.rs:148). Accepted and reconciled sends can permanently lose OnchainTransactionReceived. The durable intent is removed before its in-memory marker is published, and take_locally_applied_unconfirmed_txids removes that marker before event persistence. I reproduced restart after an accepted send and after passive reconciliation; neither queued the event. Persist an idempotent marker until event insertion succeeds, and drain it for Bitcoind as well.
  • Electrum and Esplora client timeouts report Failed (src/chain/electrum.rs:990). Normal Electrum and Esplora transport timeouts are reported as Failed, not Timeout. Electrum maps all non-protocol errors to Failed, including nested timed-out I/O errors. Esplora's 10-second client timeout fires before its 15-second outer timeout and is likewise mapped to Failed. Classify these client timeout errors as Timeout.
  • Python's error message break is undocumented (CHANGELOG.md:14). The fielded NodeError form also removes generated messages from every Python exception, but the migration note names only Swift and Kotlin. I verified the new AlreadyRunning exception string is empty, while the base converter supplied its display message. Include Python in the compatibility note.
  • Validation records do not match the reviewed head (Package.swift:7). The PR body says it validates head 0bc67a9 and archive checksum d6ccac3, but the reviewed head is 505b1a6 and Package.swift contains bae015e. The required exact-head build and archive checksum are therefore not recorded. Refresh the validation after rebuilding the current head.

nits

  • Migration code for broadcast-intent formats 1, 2 and 3 that never shipped (src/wallet/mod.rs:76). ONCHAIN_BROADCAST_INTENT_PRIMARY_NAMESPACE does not exist on main (5c0c56b) or on the base branch, so this PR introduces the intent store for the first time and no released build ever wrote versions 1, 2 or 3. The three legacy constants, the two decode arms in read_broadcast_intent_from_store, BroadcastIntent::from_legacy, and the legacy_single_transaction_broadcast_intent_is_restored and legacy_resolved_lineage_is_migrated_without_becoming_pending tests are all dead weight from the branch's own iteration history. Shipping only version 4 would remove roughly a hundred lines and one decode surface.
  • PR description's XCFramework SHA256 does not match Package.swift at HEAD (Package.swift:7). The description states the archive SHA256 is d6ccac3a6a013fbb5313d49faf4d3373b14053ede91923e9a1cd4f3230c6848c and that Package.swift matches, but Package.swift at 505b1a6 has bae015e73bc12ee10fc7f817241e07e1a0eea9ab125c8b1403b7b2599ebce75e. The last commit refreshed the binding artifacts and the description was not updated. No release exists yet, so nothing is broken; just re-verify with shasum -a 256 bindings/swift/LDKNodeFFI.xcframework.zip against Package.swift before uploading, per the release checklist.

the reviewers disagree, your call

  • LDK broadcast backlog can grow without bound (src/tx_broadcaster.rs:225). Objection: The switch to an unbounded LDK channel is the deliberate point of the change: main's 50-slot channel silently dropped safety-critical packages on overflow, and ldk_broadcast_is_not_dropped_when_explicit_queue_is_saturated asserts the new behaviour. Growth is paced by LDK's rebroadcast cadence at roughly a kilobyte per package, so memory exhaustion is not established, and restoring a bound would reintroduce the drop. Answer: src/tx_broadcaster.rs:225 creates an unbounded lane, and lines 314-317 clone every package without coalescing. With a blackholed bitcoind backend and 100 fully signed pending claims, each 30-second rebroadcast adds 100 packages while the serial worker can complete at most two 15-second attempts, leaving 98 retained clones per cycle until memory is exhausted.

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.

chore: stop tracking generated binding artifacts On-chain send returns before backend broadcast result

3 participants