Assure BroadcasterInterface packages of len > 1 are child-with-parents #4173
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4173 +/- ##
==========================================
+ Coverage 88.78% 88.82% +0.03%
==========================================
Files 180 180
Lines 137066 137278 +212
Branches 137066 137278 +212
==========================================
+ Hits 121694 121936 +242
+ Misses 12552 12531 -21
+ Partials 2820 2811 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// 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 considered to be a | ||
| /// child-with-parents package and be broadcast together. Implementations MUST NOT |
There was a problem hiding this comment.
nit: might be worth clarifying a single child and linking the Bitcoin Core docs (if they're online or just saying "the submitpackage RPC").
tankyleo
force-pushed
the
25-10-broadcaster-docs
branch
from
4ba0e53 to
5916b2a
Compare
| /// If more than one transaction is given, these transactions MUST be considered to be a | ||
| /// single-child-with-parents package and be broadcast together |
There was a problem hiding this comment.
| /// If more than one transaction is given, these transactions MUST be considered to be a | |
| /// single-child-with-parents package and be broadcast together | |
| /// If more than one transaction is given, these transactions MUST be a | |
| /// single child and its parents and be broadcast together as a package |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
tankyleo
force-pushed
the
25-10-broadcaster-docs
branch
from
5916b2a to
aac2b29
Compare
| /// single child and its parents and be broadcast together as a package | ||
| /// (see the `submitpackage` Bitcoin Core RPC). | ||
| /// | ||
| /// Implementations MUST NOT assume any topological order on the transactions. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Generally LGTM, one comment.
So we remove the possibility that a batch of transactions passed to a BroadcasterInterface implementation contains unrelated transactions, or multiple children.
Good, that possibly makes things a bit easier down the line also, when add a BroadcastType or similar to the BroadcasterInterface. (Will see to open a draft for this soonish)
| /// 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` Bitcoin Core RPC). |
There was a problem hiding this comment.
Can we add a link here, especially since Core docs for RPCs aren't super discoverable?
tankyleo
force-pushed
the
25-10-broadcaster-docs
branch
from
aac2b29 to
10a403e
Compare
Implementations MUST NOT assume any topological order on the transactions. While Bitcoin Core v29+ `submitpackage` RPC allows packages of length 1 to be submitted via `submitpackage`, it still requires any package submitted there to be a `child-with-parents` package. So we remove the possibility that a batch of transactions passed to a `BroadcasterInterface` implementation contains unrelated transactions, or multiple children.
tankyleo
force-pushed
the
25-10-broadcaster-docs
branch
from
10a403e to
7c9b21f
Compare
|
Backported to 0.2 in #4185. |
5 participants
Implementations MUST NOT assume any topological order on the transactions.
While Bitcoin Core v29+
submitpackageRPC allows packages of length 1 to be submitted viasubmitpackage, it still requires all packages submitted there to bechild-with-parents.So we remove the possibility that a batch of transactions passed to a
BroadcasterInterfaceimplementation contains unrelated transactions, or multiple children.