Skip to content

Assure BroadcasterInterface packages of len > 1 are child-with-parents #4173

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 27, 2025
Merged

Assure BroadcasterInterface packages of len > 1 are child-with-parents #4173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions lightning/src/chain/chaininterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,12 @@ pub trait BroadcasterInterface {
/// In some cases LDK may attempt to broadcast a transaction which double-spends another
/// and this isn't a bug and can be safely ignored.
///
/// If more than one transaction is given, these transactions should be considered to be a
/// package and broadcast together. Some of the transactions may or may not depend on each other,
/// be sure to manage both cases correctly.
/// If more than one transaction is given, these transactions MUST be a
/// single child and its parents and be broadcast together as a package
/// (see the [`submitpackage`](https://bitcoincore.org/en/doc/30.0.0/rpc/rawtransactions/submitpackage)
/// Bitcoin Core RPC).
///
/// Implementations MUST NOT assume any topological order on the transactions.
Copy link
Contributor

@benthecarman benthecarman Oct 27, 2025

Choose a reason for hiding this comment

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

in submitpackage's docs it says

The package must be topologically sorted, with the child being the last element in the array.

making the user handle this seems like extra complexity that LDK should be able to handle for them

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Oct 27, 2025

Choose a reason for hiding this comment

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

LDK will still broadcast transactions topologically (and maybe we should check that in the tests @tankyleo) but the API is also sometimes used by dowstream code (eg ldk-node) and it seems like a good thing for the BroadcasterInterface implementation to re-sort topologically.

///
/// Bitcoin transaction packages are defined in BIP 331 and here:
/// <https://github.com/bitcoin/bitcoin/blob/master/doc/policy/packages.md>
Expand Down
3 changes: 2 additions & 1 deletion lightning/src/events/bump_transaction/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1135,6 +1135,7 @@ mod tests {
let commitment_tx_bytes = Vec::<u8>::from_hex("02000000000101cc6b0a9dd84b52c07340fff6fab002fc37b4bdccfdce9f39c5ec8391a56b652907000000009b948b80044a01000000000000220020b4182433fdfdfbf894897c98f84d92cec815cee222755ffd000ae091c9dadc2d4a01000000000000220020f83f7dbf90e2de325b5bb6bab0ae370151278c6964739242b2e7ce0cb68a5d81cb4a02000000000022002024add256b3dccee772610caef82a601045ab6f98fd6d5df608cc756b891ccfe63ffa490000000000220020894bf32b37906a643625e87131897c3714c71b3ac9b161862c9aa6c8d468b4c70400473044022060abd347bff2cca0212b660e6addff792b3356bd4a1b5b26672dc2e694c3c5f002202b40b7e346b494a7b1d048b4ec33ba99c90a09ab48eb1df64ccdc768066c865c014730440220554d8361e04dc0ee178dcb23d2d23f53ec7a1ae4312a5be76bd9e83ab8981f3d0220501f23ffb18cb81ccea72d30252f88d5e69fd28ba4992803d03c00d06fa8899e0147522102817f6ce189ab7114f89e8d5df58cdbbaf272dc8e71b92982d47456a0b6a0ceee2102c9b4d2f24aca54f65e13f4c83e2a8d8e877e12d3c71a76e81f28a5cabc652aa352ae626c7620").unwrap();
let commitment_tx: Transaction =
Readable::read(&mut Cursor::new(&commitment_tx_bytes)).unwrap();
let commitment_txid = commitment_tx.compute_txid();
let total_commitment_weight =
commitment_tx.weight().to_wu() + ANCHOR_INPUT_WITNESS_WEIGHT + EMPTY_SCRIPT_SIG_WEIGHT;
let commitment_and_anchor_fee = 930 + 330;
Expand Down Expand Up @@ -1191,7 +1192,7 @@ mod tests {
keys_id: [42; 32],
transaction_parameters,
},
outpoint: OutPoint { txid: Txid::from_byte_array([42; 32]), vout: 0 },
outpoint: OutPoint { txid: commitment_txid, vout: 0 },
value: Amount::from_sat(ANCHOR_OUTPUT_VALUE_SATOSHI),
},
pending_htlcs: Vec::new(),
Expand Down
24 changes: 24 additions & 0 deletions lightning/src/util/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,30 @@ impl TestBroadcaster {

impl chaininterface::BroadcasterInterface for TestBroadcaster {
fn broadcast_transactions(&self, txs: &[&Transaction]) {
// Assert that any batch of transactions of length greater than 1 is sorted
// topologically, and is a `child-with-parents` package as defined in
// <https://github.com/bitcoin/bitcoin/blob/master/doc/policy/packages.md>.
//
// Implementations MUST NOT rely on this, and must re-sort the transactions
// themselves.
//
// Right now LDK only ever broadcasts packages of length 2.
assert!(txs.len() <= 2);
if txs.len() == 2 {
let parent_txid = txs[0].compute_txid();
assert!(txs[1]
.input
.iter()
.map(|input| input.previous_output.txid)
.any(|txid| txid == parent_txid));
let child_txid = txs[1].compute_txid();
assert!(txs[0]
.input
.iter()
.map(|input| input.previous_output.txid)
.all(|txid| txid != child_txid));
}

for tx in txs {
let lock_time = tx.lock_time.to_consensus_u32();
assert!(lock_time < 1_500_000_000);
Expand Down
Loading