-
Notifications
You must be signed in to change notification settings - Fork 433
Refactor BroadcasterInterface to include TransactionType
#4353
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
base: main
Are you sure you want to change the base?
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
4f6a8a4 to
be02cec
Compare
TheBlueMatt
left a comment
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.
docs need a lot of love
| /// A single funding transaction may establish multiple channels when using batch funding. | ||
| channel_ids: Vec<ChannelId>, | ||
| }, | ||
| /// A cooperative close transaction mutually agreed upon by both parties. |
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.
I mean all transactions are mutually agreed upon by both parties? Maybe describe what a coop close tx means
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.
Hmm, changed it up a bit, but let me know what exactly is missing for you.
be02cec to
4f6a8a4
Compare
4f6a8a4 to
fb8378c
Compare
Add a `BroadcastType` enum to provide context about the type of transaction being broadcast. This information can be useful for logging, filtering, or prioritization purposes. The `BroadcastType` variants are: - `Funding`: A funding transaction establishing a new channel - `CooperativeClose`: A cooperative close transaction - `UnilateralClose`: A force-close transaction - `AnchorBump`: An anchor transaction for CPFP fee-bumping - `Claim`: A transaction claiming outputs from commitment tx - `Sweep`: A transaction sweeping spendable outputs to wallet Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
We add the `ChannelId` as context to the just-added `BroadcastType` enum. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
fb8378c to
54b52f3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4353 +/- ##
==========================================
- Coverage 86.08% 86.08% -0.01%
==========================================
Files 156 156
Lines 102428 102488 +60
Branches 102428 102488 +60
==========================================
+ Hits 88179 88227 +48
- Misses 11754 11767 +13
+ Partials 2495 2494 -1
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:
|
| #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] | ||
| pub enum BroadcastType { | ||
| /// A funding transaction establishing a new channel. | ||
| Funding, |
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.
Splice transactions should have their own variant, no?
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.
Right, hence the question above regarding which variants we want. So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?
And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?
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.
So, post #4261 we'll now three variants (SpliceIn/SpliceOut/SpliceInAndOut)?
If we want more splicing context we should include the splice metadata in the splice variant rather than having a separate variant for each splice init logic. This feels like a followup though.
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.
To clarify a bit - the differentiation between "splice out" and "splice in" at the top-level API is a bug that we're fixing - there is only a "splice" which can have constituent parts that are in + out. Its a useful differentiation when building splicing instructions but after that it should dissapear.
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.
And AFAICT we need to track the TransactionType across the entire flow (i.e., inititally set in SpliceContribution, track in SpliceInstructions, PendingFunding, and then return via FundingTxSigned to hand it to broadcast_interactive_funding? Or do you see a way to shortcut or re-derive the type of splice contribution at the last step somehow?
The raw transaction is already available since we're broadcasting it so they can inspect its inputs and outputs if they want to distinguish between splice in/out.
| /// A single funding transaction may establish multiple channels when using batch funding. | ||
| channel_ids: Vec<ChannelId>, | ||
| }, | ||
| /// A transaction cooperatively closing a channel. |
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.
I think the thing missing here and elsewhere is the why - if I'm using this for improving my logging I want to understand what led to this transaction and what the implications of it are. eg this have a link to ChannelManager::close_channel (noting of course that the counterparty could have initiated as well), UnilateralClose should have a link to ChannelManager::force_close_* and note that it could also happen due to other errors.
We add the `ChannelId` as context to the just-added `TransactionType` enum. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
54b52f3 to
5912fd4
Compare
|
FWIW, I also now pushed a fixup renaming to |
BroadcasterInterface to include BroadcastTypeBroadcasterInterface to include TransactionType
Fixes #3566.
and: