-
Notifications
You must be signed in to change notification settings - Fork 162
Keep funding payment records accurate #1057
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jkczyz
wants to merge
19
commits into
lightningdevkit:main
Choose a base branch
from
jkczyz:2026-08-funding-payment-bugfixes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
129005a
Only adopt a funding payment's own transactions from wallet sync
jkczyz bca2cdd
Retry funding-broadcast classification instead of dropping it
jkczyz 1ba8c5d
f - Retry funding-broadcast classification instead of dropping it
jkczyz a76d7dc
f - Retry funding-broadcast classification instead of dropping it
jkczyz 613afea
f - Retry funding-broadcast classification instead of dropping it
jkczyz a0d3a94
Fail funding payments lost to a confirmed conflict
jkczyz c71ce2f
f - Fail funding payments lost to a confirmed conflict
jkczyz 8fb3298
Record splice funding payments when signing
jkczyz 914f823
f - Record splice funding payments when signing
jkczyz 63a079b
f - Record splice funding payments when signing
jkczyz 2314b73
f - Record splice funding payments when signing
jkczyz ee6397c
f - Record splice funding payments when signing
jkczyz 475cfc3
f - Record splice funding payments when signing
jkczyz 4d4a85d
Resolve funding payments when LDK discards a splice round
jkczyz 24d1232
f - Resolve funding payments when LDK discards a splice round
jkczyz a9e2edb
f - Resolve funding payments when LDK discards a splice round
jkczyz 1e24246
f - Resolve funding payments when LDK discards a splice round
jkczyz a4aeaf9
Resolve a transaction to the record that owns it before one listing i…
jkczyz 5a889fc
Take the funding re-broadcast trace from the write's own read
jkczyz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,9 +37,15 @@ use crate::config::{BackgroundSyncConfig, Config, WALLET_SYNC_INTERVAL_MINIMUM_S | |
| use crate::fee_estimator::OnchainFeeEstimator; | ||
| use crate::logger::{log_debug, log_error, log_info, log_trace, LdkLogger, Logger}; | ||
| use crate::runtime::Runtime; | ||
| use crate::tx_broadcaster::BroadcastPackage; | ||
| use crate::types::{Broadcaster, ChainMonitor, ChannelManager, DynStore, Sweeper, Wallet}; | ||
| use crate::{Error, PersistedNodeMetrics}; | ||
|
|
||
| /// How long to wait before re-classifying a package whose classification failed. Long enough to | ||
| /// give a struggling store room to recover, short against the ~minutes until the transaction | ||
| /// could confirm. | ||
| pub(crate) const FAILED_CLASSIFY_RETRY_DELAY: Duration = Duration::from_secs(2); | ||
|
|
||
| /// We use this parent-child TRUC package to make sure the configured chain source supports | ||
| /// broadcasting packages via the `submitpackage` Bitcoin Core RPC. | ||
| const PARENT_TXID: &str = "9a015f93fac6cb203c2b994e18b85176eb0354a22a468255516f3c6002d3f696"; | ||
|
|
@@ -562,51 +568,62 @@ impl ChainSource { | |
| } | ||
| } | ||
|
|
||
| /// Classifies the package's funding broadcasts into payment records, then broadcasts it. | ||
| /// Returns the package back on classification failure so the caller can retry it after a | ||
| /// delay: broadcasting a tx we failed to record would leave it on-chain without a payment, | ||
| /// while dropping the package would keep a funding transaction off-chain until LDK re-hands | ||
| /// it when the channel next resumes — no timer re-broadcasts it, and the wallet's tip-change | ||
| /// re-broadcast covers recorded transactions only. | ||
| async fn classify_and_broadcast( | ||
| &self, package: BroadcastPackage, | ||
| ) -> Result<(), BroadcastPackage> { | ||
| if let Err(e) = self.tx_broadcaster.classify_package(&package).await { | ||
| log_error!( | ||
| self.logger, | ||
| "Delaying broadcast: failed to persist payment records, will retry: {:?}", | ||
| e, | ||
| ); | ||
| return Err(package); | ||
| } | ||
| let package = package.into_sorted_transactions(); | ||
| match &self.kind { | ||
| #[cfg(feature = "chain-esplora")] | ||
| ChainSourceKind::Esplora(esplora_chain_source) => { | ||
| esplora_chain_source.process_transaction_broadcast(package).await | ||
| }, | ||
| #[cfg(feature = "chain-electrum")] | ||
| ChainSourceKind::Electrum(electrum_chain_source) => { | ||
| electrum_chain_source.process_transaction_broadcast(package).await | ||
| }, | ||
| #[cfg(feature = "chain-bitcoind")] | ||
| ChainSourceKind::Bitcoind(bitcoind_chain_source) => { | ||
| bitcoind_chain_source.process_transaction_broadcast(package).await | ||
| }, | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub(crate) async fn continuously_process_broadcast_queue( | ||
| &self, mut stop_tx_bcast_receiver: tokio::sync::watch::Receiver<()>, | ||
| ) { | ||
| let mut receiver = self.tx_broadcaster.get_broadcast_queue().await; | ||
| loop { | ||
| let tx_bcast_logger = Arc::clone(&self.logger); | ||
| tokio::select! { | ||
| let package = tokio::select! { | ||
| // A stop request is polled first, so a queue that always has a package ready | ||
| // cannot starve it. Which package comes next — a fresh one before a due retry — | ||
| // is decided in `BroadcastQueue::next`. | ||
| biased; | ||
| _ = stop_tx_bcast_receiver.changed() => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Codex:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed when merging the queues into |
||
| log_debug!( | ||
| tx_bcast_logger, | ||
| self.logger, | ||
| "Stopping broadcasting transactions.", | ||
| ); | ||
| return; | ||
| } | ||
| Some(next_package) = receiver.recv() => { | ||
| // Classify funding broadcasts into payment records before sending. If | ||
| // classification fails we skip the broadcast, since broadcasting a tx we | ||
| // failed to record would leave it on-chain without a payment. | ||
| let package = match self.tx_broadcaster.classify_package(next_package).await { | ||
| Ok(package) => package, | ||
| Err(e) => { | ||
| log_error!( | ||
| tx_bcast_logger, | ||
| "Skipping broadcast: failed to persist payment records: {:?}", | ||
| e, | ||
| ); | ||
| continue; | ||
| }, | ||
| }; | ||
| let package = package.into_sorted_transactions(); | ||
| match &self.kind { | ||
| #[cfg(feature = "chain-esplora")] | ||
| ChainSourceKind::Esplora(esplora_chain_source) => { | ||
| esplora_chain_source.process_transaction_broadcast(package).await | ||
| }, | ||
| #[cfg(feature = "chain-electrum")] | ||
| ChainSourceKind::Electrum(electrum_chain_source) => { | ||
| electrum_chain_source.process_transaction_broadcast(package).await | ||
| }, | ||
| #[cfg(feature = "chain-bitcoind")] | ||
| ChainSourceKind::Bitcoind(bitcoind_chain_source) => { | ||
| bitcoind_chain_source.process_transaction_broadcast(package).await | ||
| }, | ||
| } | ||
| } | ||
| package = self.tx_broadcaster.next_package() => package, | ||
| }; | ||
| if let Err(package) = self.classify_and_broadcast(package).await { | ||
| let retry_at = tokio::time::Instant::now() + FAILED_CLASSIFY_RETRY_DELAY; | ||
| self.tx_broadcaster.retry_package(package, retry_at); | ||
| } | ||
| } | ||
| } | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why maintain separate immediate and retry paths instead of treating every broadcast as scheduled retryable work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor the duplicated code, but kept the paths separate. Now that we have a bounded queue and deduplication, using the same path would mean we'd drop newer packages.