Support broadcasting multiple transactions at once #2272
Conversation
| } | ||
| broadcaster.broadcast_transactions(&txs); |
There was a problem hiding this comment.
this was the only place I found where we could make use of the change
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2272 +/- ##
==========================================
- Coverage 91.63% 90.88% -0.75%
==========================================
Files 104 104
Lines 51985 52843 +858
Branches 51985 52843 +858
==========================================
+ Hits 47638 48028 +390
- Misses 4347 4815 +468
☔ View full report in Codecov by Sentry. |
benthecarman
force-pushed
the
package-broadcast
branch
from
e1c4647 to
9fae7e3
Compare
| /// In some cases 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 |
There was a problem hiding this comment.
Do we want to say its a package if they're not a package? Maybe transactions may be a package? Dunno what the right phrasing is to make clear it (a) may be a package, and (b) if it is a package (but we're not telling you if it is or not) you need to make sure its handled as a package. Dont think we need to update the API for that given Core handles it the same either way, but I wonder if there's any P2P-based clients that would benefit from a flag?
There was a problem hiding this comment.
yeah that's why I said these transactions should be considered to be a package. so they assume it is one but doesn't sound like it always is one.
wording is hard here
There was a problem hiding this comment.
Wording is always hard :(
There was a problem hiding this comment.
But, yea, I think this LGTM.
There was a problem hiding this comment.
but I wonder if there's any P2P-based clients that would benefit from a flag?
Note there is a) Core’s RPC definition of a package (submitpackage), a mining code definition of a package CreateNewBlock and c) the WIP BIP331 defining package at the p2p-level, so which definition the user is expected to respect here ?
There was a problem hiding this comment.
Good to use the P2P definition, can we reference to BIP331 directly in the code/documentation (nothing it's not final as of LDK upcoming v0.0.116 release) ?
There was a problem hiding this comment.
We could also/instead link to https://github.com/bitcoin/bitcoin/blob/master/doc/policy/packages.md.
There was a problem hiding this comment.
We could link to that, but that feels more like "the things LDK needs to do to create its packages" than "what the user needs to be aware of to implement this" - rather, we can just be explicit and say "via BIP331 over the P2P protocol or via the XXX Bitcoin Core RPC"?
There was a problem hiding this comment.
Sure, just thought it'd be nice to provide some more literature if they're not too familiar with the concept and that breaks it down a bit better than diving straight into the BIP.
There was a problem hiding this comment.
Yes with the warning than packages.md might be more loose than p2p packages as defined in BIP331.
benthecarman
force-pushed
the
package-broadcast
branch
from
9fae7e3 to
9f1feca
Compare
There was a problem hiding this comment.
If we document and extend our BroadcasterInterface, I think at least we should aim to make it clear in the state of package development in the wider ecosystem. Our LDK interfaces are already complex enough and I don’t think we’re making a gift to our users to lack a clarity on “package” handling (or we just giving the “hot” potato to LDK-node)
| /// In some cases 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 |
There was a problem hiding this comment.
but I wonder if there's any P2P-based clients that would benefit from a flag?
Note there is a) Core’s RPC definition of a package (submitpackage), a mining code definition of a package CreateNewBlock and c) the WIP BIP331 defining package at the p2p-level, so which definition the user is expected to respect here ?
benthecarman
force-pushed
the
package-broadcast
branch
from
9f1feca to
66104a7
Compare
|
See #2272 (comment) as a PR blocker, I think. |
| /// In some cases 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 |
There was a problem hiding this comment.
We could also/instead link to https://github.com/bitcoin/bitcoin/blob/master/doc/policy/packages.md.
| /// 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. | ||
| fn broadcast_transactions(&self, txs: &[&Transaction]); |
There was a problem hiding this comment.
Not a blocker, but we could make this a bit more flexible with the use of Borrow.
diff --git a/lightning/src/chain/chaininterface.rs b/lightning/src/chain/chaininterface.rs
index 8bbcb56e..62ee8b41 100644
--- a/lightning/src/chain/chaininterface.rs
+++ b/lightning/src/chain/chaininterface.rs
@@ -13,7 +13,7 @@
//! Includes traits for monitoring and receiving notifications of new blocks and block
//! disconnections, transaction broadcasting, and feerate information requests.
-use core::{cmp, ops::Deref};
+use core::{borrow::Borrow, cmp, ops::Deref};
use bitcoin::blockdata::transaction::Transaction;
@@ -29,7 +29,7 @@ pub trait BroadcasterInterface {
/// 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.
- fn broadcast_transactions(&self, txs: &[&Transaction]);
+ fn broadcast_transactions<T: Borrow<Transaction>>(&self, txs: &[T]);
}
/// An enum that represents the speed at which we want a transaction to confirm used for feerate
diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs
index 4b3cc411..4c4b39c7 100644
--- a/lightning/src/util/test_utils.rs
+++ b/lightning/src/util/test_utils.rs
@@ -48,6 +48,7 @@ use regex;
use crate::io;
use crate::prelude::*;
+use core::borrow::Borrow;
use core::cell::RefCell;
use core::time::Duration;
use crate::sync::{Mutex, Arc};
@@ -188,7 +189,7 @@ pub struct TestChainMonitor<'a> {
pub added_monitors: Mutex<Vec<(OutPoint, channelmonitor::ChannelMonitor<EnforcingSigner>)>>,
pub monitor_updates: Mutex<HashMap<[u8; 32], Vec<channelmonitor::ChannelMonitorUpdate>>>,
pub latest_monitor_update_id: Mutex<HashMap<[u8; 32], (OutPoint, u64, MonitorUpdateId)>>,
- pub chain_monitor: chainmonitor::ChainMonitor<EnforcingSigner, &'a TestChainSource, &'a chaininterface::BroadcasterInterface, &'a TestFeeEstimator, &'a TestLogger, &'a chainmonitor::Persist<EnforcingSigner>>,
+ pub chain_monitor: chainmonitor::ChainMonitor<EnforcingSigner, &'a TestChainSource, &'a TestBroadcaster, &'a TestFeeEstimator, &'a TestLogger, &'a chainmonitor::Persist<EnforcingSigner>>,
pub keys_manager: &'a TestKeysInterface,
/// If this is set to Some(), the next update_channel call (not watch_channel) must be a
/// ChannelForceClosed event for the given channel_id with should_broadcast set to the given
@@ -196,7 +197,7 @@ pub struct TestChainMonitor<'a> {
pub expect_channel_force_closed: Mutex<Option<([u8; 32], bool)>>,
}
impl<'a> TestChainMonitor<'a> {
- pub fn new(chain_source: Option<&'a TestChainSource>, broadcaster: &'a chaininterface::BroadcasterInterface, logger: &'a TestLogger, fee_estimator: &'a TestFeeEstimator, persister: &'a chainmonitor::Persist<EnforcingSigner>, keys_manager: &'a TestKeysInterface) -> Self {
+ pub fn new(chain_source: Option<&'a TestChainSource>, broadcaster: &'a TestBroadcaster, logger: &'a TestLogger, fee_estimator: &'a TestFeeEstimator, persister: &'a chainmonitor::Persist<EnforcingSigner>, keys_manager: &'a TestKeysInterface) -> Self {
Self {
added_monitors: Mutex::new(Vec::new()),
monitor_updates: Mutex::new(HashMap::new()),
@@ -341,8 +342,9 @@ impl TestBroadcaster {
}
impl chaininterface::BroadcasterInterface for TestBroadcaster {
- fn broadcast_transactions(&self, txs: &[&Transaction]) {
+ fn broadcast_transactions<T: Borrow<Transaction>>(&self, txs: &[T]) {
for tx in txs {
+ let tx = tx.borrow();
let lock_time = tx.lock_time.0;
assert!(lock_time < 1_500_000_000);
if bitcoin::LockTime::from(tx.lock_time).is_block_height() && lock_time > self.blocks.lock().unwrap().last().unwrap().1 {
@@ -353,7 +355,7 @@ impl chaininterface::BroadcasterInterface for TestBroadcaster {
}
}
}
- let owned_txs: Vec<Transaction> = txs.iter().map(|tx| (*tx).clone()).collect();
+ let owned_txs: Vec<Transaction> = txs.iter().map(|tx| tx.borrow().clone()).collect();
self.txn_broadcasted.lock().unwrap().extend(owned_txs);
}
}
There was a problem hiding this comment.
I think for transactions we can just do straight &[Transaction] if its practical, making it generic is a lot of compiler overhead, and we don't need the &[&X] indirection for bindings for transaction specifically.
There was a problem hiding this comment.
Fair enough, I don't think we ever return a Vec of transaction references anyway that would force us to clone.
benthecarman
force-pushed
the
package-broadcast
branch
from
66104a7 to
29b392a
Compare
|
Added a reference to BIP 331 and https://github.com/bitcoin/bitcoin/blob/master/doc/policy/packages.md Not sure the best way to word this here either 😅 |
Eventually once we have package relay this will get it's full benefit. For now will allow people to join futures to make things slightly more efficient and will setup for eventually supporting package relay.