Skip to content

fix: wait for broadcast result - #113

Open
ovitrif wants to merge 43 commits into
mainfrom
codex/112-onchain-broadcast-result
Open

ovitrif wants to merge 43 commits into
mainfrom
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, and support exact-byte rebroadcast or explicit abandonment after independent reconciliation.
  • Expose durable Pending, Accepted, and Abandoned broadcast outcomes by any tracked lineage transaction ID.
  • Retain terminal outcomes without TTL until explicit consumer acknowledgement, including after restart, confirmation, payment-history cleanup, and abandonment.
  • Arm outcome retention before every explicit dispatch, then persist accepted and abandoned outcomes before removing their active intent; persistence failures stay queryable and fail closed.
  • Persist on-chain event delivery state so accepted and acceptance-unknown sends emit one recoverable received or confirmed event across every chain backend, restart, and concurrent wallet syncs.
  • Retire the previous confirmation receipt atomically with each reorg event so a later confirmation is not suppressed after restart.
  • Preserve legacy event receipts during upgrade, including already-acknowledged confirmations whose delivery-marker cleanup failed.
  • Serialize broadcast dispatch and abandonment, retain complete RBF lineage across active replacements, and defer cleanup until every participating wallet account is loaded.
  • Keep LDK-managed broadcasts fire-and-forget on a dedicated queue, coalesce identical live packages, and dispatch explicit backend requests independently.
  • Require backend evidence before reconciliation and classify Bitcoin Core, Electrum, and Esplora responses conservatively.
  • Publish the breaking binding contract as 0.7.0-rc.67.

Out of Scope

Preview

N/A — no user-visible changes.

QA Notes

  • A deterministic backend rejection does not reach the consumer success path and releases the pending spend.
  • A timeout or operational failure after dispatch carries the transaction ID. Consumers must reconcile or call rebroadcast_transaction for that exact ID; they must not create a second payment.
  • broadcast_outcome accepts any lineage transaction ID and returns the canonical active or terminal transaction ID plus the complete lineage.
  • Only BroadcastOutcomeStatus::Accepted proves backend acceptance. Pending, an unknown result, and a query failure remain unresolved.
  • acknowledge_broadcast_outcome removes one root-keyed terminal record and every lineage alias after downstream handling is durable. It fails while the lineage remains active and is idempotent after removal.
  • list_pending_broadcasts returns every unresolved transaction and its full RBF lineage.
  • abandon_pending_broadcast is valid only after an independent source proves every lineage member absent from mempool and chain and no other process can rebroadcast it.
  • Swift unit NodeError cases no longer carry the legacy generated message value. The four broadcast-result cases carry txid.
  • Kotlin and Python fieldless exceptions have empty generated messages. The four broadcast-result cases carry txid.
  • Version-one persisted receipts contain only a transaction ID. Queued events recover the known transition type; already-acknowledged receipts retain the old broad suppression until marker retirement because their original type is irrecoverable. This prevents duplicate confirmations but cannot reconstruct transitions the old format never recorded.

Validation

  • Validated head: 9a5cba0; current main at 2666461 is included.
  • cargo fmt --check passed.
  • cargo test --lib passed: 205 tests.
  • cargo test --features uniffi --lib passed: 215 tests.
  • cargo clippy --lib --tests --locked exited successfully with existing warnings; this is not a warning-free result.
  • Added regressions for interrupted confirmation cleanup followed by restart/reorg/reconfirmation, failed reorg persistence, serialized event acknowledgement, and upgrades with an acknowledged received or confirmed event and a stale delivery marker.
  • Canonical ./bindgen.sh completed: macOS, iOS device/simulator, Android native libraries, JVM build, and Android AAR packaging. Android native debug symbols and 16 KB load alignment checks passed.
  • Regenerated Swift, Kotlin Android/JVM, and Python API sources are unchanged. All package versions remain 0.7.0-rc.67.
  • Fresh Swift archive SHA-256: 65faaedeb2ec7b182e31c1e3960e59491aa3992ad9e82be7e9ab07ea9d7cdfdf. Both independent SHA-256 and SwiftPM checksum computation match Package.swift; archive integrity passed.
  • Validation used Rust 1.98.1, Xcode 26.6, and Android NDK 28.1.13356709. Full integration, device, and downstream consumer tests were not run in this validation follow-up.
  • No PR-triggered CI checks are configured. The Rust workflow is manually dispatched only.

Consumer contract

  • Android generated API: BroadcastOutcomeStatus.PENDING|ACCEPTED|ABANDONED, BroadcastOutcome(status, txid, lineage), broadcastOutcome(txid), and acknowledgeBroadcastOutcome(txid).
  • Swift generated API: BroadcastOutcomeStatus.pending|accepted|abandoned, BroadcastOutcome(status:txid:lineage:), broadcastOutcome(txid:), and acknowledgeBroadcastOutcome(txid:).
  • Android fix: require accepted onchain broadcasts bitkit-android#1225 and iOS fix: prevent false on-chain send success bitkit-ios#727 confirmed these exact names and semantics before generation.
  • Android artifact coordinates: com.synonym:ldk-node-android:0.7.0-rc.67.

Release

  • v0.7.0-rc.67 is not tagged or released, and the Maven version is not published. The refreshed local artifacts are not yet available to downstream consumers.
  • Fresh non-author approval is required on 9a5cba0. Recorded approvals from ben-kaufman and coreyphillips cover older commits; both review requests are pending.
  • After approval and publication authorization, tag the final pushed rc.67 commit and publish the release with the checksum-matching LDKNodeFFI.xcframework.zip. This supersedes earlier local rc.67 archives.
  • Publishing the release triggers Android Publish, which must successfully publish com.synonym:ldk-node-android:0.7.0-rc.67 to GitHub Packages.
  • Android #1225 must resolve the published Maven artifact. iOS Expose a VssStoreBuilder allowing to build VssStore independently lightningdevkit/ldk-node#727 must pin the rc.67 SwiftPM tag and resolve its XCFramework; iOS does not consume the Maven artifact. Each downstream PR still needs its own integration validation against those published artifacts.

@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 Outdated
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 Outdated
@ovitrif
ovitrif requested a review from ben-kaufman September 3, 2026 23:01
Comment thread src/payment/onchain.rs Outdated
@ovitrif
ovitrif force-pushed the codex/112-onchain-broadcast-result branch from 3b1f49f to 2f6d7c8 Compare September 4, 2026 14:51
@ovitrif ovitrif mentioned this pull request Sep 4, 2026
@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
Comment thread src/payment/onchain.rs
Comment thread src/wallet/mod.rs
Comment thread src/chain/mod.rs Outdated
@ovitrif
ovitrif requested a review from ben-kaufman September 9, 2026 16:11
Comment thread src/wallet/mod.rs
Comment thread src/wallet/mod.rs Outdated
@ovitrif
ovitrif requested a review from ben-kaufman September 9, 2026 22:57
@coreyphillips

Copy link
Copy Markdown
Collaborator

I found three issues that should be addressed before merge:

  1. prepare_pending_broadcast persists the intent before note_locally_applied_unconfirmed. If marker persistence fails, the API returns PersistenceFailed without a txid before BDK reserves the inputs. A retry can create a conflicting transaction. The RBF path has the same ordering. Please roll back the intent or return a txid-keyed recoverable error, and test both paths.

  2. process_wallet_events drains ready broadcast event markers for Esplora and Electrum, but Bitcoind applies mempool updates without calling it. Bitcoind explicit sends can therefore miss OnchainTransactionReceived indefinitely. Please pump the markers after Bitcoind wallet application and add coverage.

  3. supersede_from permits replacing any lineage member, but rejection rollback stores only that predecessor and whether it was pending. For lineage [R0, R1] with active accepted R1, rejecting R2 built from R0 restores R0 as authoritative. Please restrict RBF to the active member or persist and restore the previous active index and state.

@ovitrif

ovitrif commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

I addressed all three findings in 4f7f0c6:

  • Initial and RBF marker-write failures now return OnchainTxBroadcastFailed { txid } while preserving the durable intent for outcome lookup and exact recovery.
  • Bitcoind chain/mempool sync now pumps ready explicit-broadcast markers through the shared wallet-event path.
  • RBF creation now accepts only the active lineage member, preventing rejection rollback from restoring a stale predecessor.

The initial, RBF, Bitcoind, and inactive-lineage regression sources are included. No public API or generated binding changed.

Comment thread src/wallet/mod.rs
Comment thread src/chain/bitcoind.rs

@ben-kaufman ben-kaufman left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The earlier outcome-retention finding remains unresolved: #113 (comment).

Comment thread src/chain/mod.rs Outdated
Comment thread src/chain/bitcoind.rs Outdated
@ovitrif
ovitrif requested a review from ben-kaufman September 11, 2026 11:57
Comment thread src/chain/mod.rs Outdated
Comment thread src/payment/onchain.rs Outdated
Comment thread src/chain/mod.rs Outdated
@ovitrif
ovitrif requested a review from ben-kaufman September 18, 2026 18:40
Comment thread src/chain/mod.rs
Comment thread src/event.rs Outdated
@ovitrif

ovitrif commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

I pushed 9a5cba0 to address the two event-durability findings:

  • Reorg events now persist together with retirement of the previous confirmation receipt, allowing reconfirmation after interrupted cleanup and restart.
  • Legacy upgrades recover queued event types and preserve broad suppression for already-acknowledged, untyped receipts until marker retirement, preventing a handled confirmation from being replayed.

Local verification: formatting, 205 library tests, 215 UniFFI library tests, and Clippy passed (existing warnings). The new regressions cover restart/reorg/reconfirmation, failed persistence, acknowledgement concurrency, and legacy upgrades. All native bindings rebuilt; generated APIs are unchanged, and the refreshed Swift archive matches Package.swift.

Full integration, device, and downstream consumer tests were not run. Fresh approval and rc.67 publication are still pending.

@ovitrif
ovitrif requested a review from ben-kaufman September 22, 2026 17:23
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.

fix: On-chain send returns before broadcast result

3 participants