Expose channel_id / counterparty_node_id in BumpTransaction event

[tnull]

A client node might choose not to handle Event::BumptTransaction events and leave bumping / Anchor output spending to a trusted counterparty.

However, Event::BumptTransaction currently doesn't offer any clear indication what channel and/or counterparty it is referring to. In order to allow filtering these events, we here expose the channel_id and counterparty_node_id fields.

[coderabbitai]

Note

Reviews Paused

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[tnull]

@coderabbitai pause

[shaavan]

Concept ACK

The description of BumpTransactionEvent reads that:

/// Indicates that a channel featuring anchor outputs is to be closed by broadcasting the local
/// commitment transaction.

Hence it seems most logical for this event to contain information about the Channel_id. Also, the counterparty_node_id is an important piece of information and can be useful when handling channel close events like BumpTransactionEvent. Adding it preemptively to the event makes sure that it's readily available when needed, leading to clean code.

I think we can go about adding the test coverage for the added changes!

[tnull]

Force-pushed with minor fixups:

> git diff-tree -U2 e9e4e4582 40ef505bd
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index 638785c9f..dc461c6a9 100644
--- a/lightning/src/events/bump_transaction.rs
+++ b/lightning/src/events/bump_transaction.rs
@@ -150,10 +150,6 @@ pub enum BumpTransactionEvent {
        ChannelClose {
                /// The `channel_id` of the channel which has been closed.
-               ///
-               /// This field will be `None` for objects serialized prior to LDK 0.0.122.
                channel_id: Option<ChannelId>,
                /// Counterparty in the closed channel.
-               ///
-               /// This field will be `None` for objects serialized prior to LDK 0.0.122.
                counterparty_node_id: Option<PublicKey>,
                /// The unique identifier for the claim of the anchor output in the commitment transaction.
@@ -211,10 +207,6 @@ pub enum BumpTransactionEvent {
        HTLCResolution {
                /// The `channel_id` of the channel which has been closed.
-               ///
-               /// This field will be `None` for objects serialized prior to LDK 0.0.122.
                channel_id: Option<ChannelId>,
                /// Counterparty in the closed channel.
-               ///
-               /// This field will be `None` for objects serialized prior to LDK 0.0.122.
                counterparty_node_id: Option<PublicKey>,
                /// The unique identifier for the claim of the HTLCs in the confirmed commitment

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7c94636) 89.17% compared to head (5b5c874) 90.39%.
Report is 31 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2873      +/-   ##
==========================================
+ Coverage   89.17%   90.39%   +1.22%     
==========================================
  Files         116      116              
  Lines       93252   100679    +7427     
  Branches    93252   100679    +7427     
==========================================
+ Hits        83157    91011    +7854     
+ Misses       7567     7247     -320     
+ Partials     2528     2421     -107     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[shaavan]

LGTM!

Makes perfect sense to not say This field will be None for objects serialized prior to LDK 0.0.122. for fields that are not serialized!
Btw, great catch, @dunxen :)

Also, a small test coverage might be good to have, but the PR works just fine with it too IMO!

[wpaulino]

We should be able to get rid of the options as well since the event isn't serialized.

[tnull]

We should be able to get rid of the options as well since the event isn't serialized.

True, now did that for the channel_id, but we can't for counterparty_node_id I'm afraid as that will only be present for ChannelMonitors serialized with v0.0.110 and above, and we support back to v0.0.99.

Amended to add the following changes:

> git diff-tree -U2 40ef505bd 3f746a300
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 7a3ebc676..e7e68297a 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2932,5 +2932,5 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
                                        package_target_feerate_sat_per_1000_weight, commitment_tx, anchor_output_idx,
                                } => {
-                                       let channel_id = Some(self.channel_id);
+                                       let channel_id = self.channel_id;
                                        let counterparty_node_id = self.counterparty_node_id;
                                        let commitment_txid = commitment_tx.txid();
@@ -2963,5 +2963,5 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
                                        target_feerate_sat_per_1000_weight, htlcs, tx_lock_time,
                                } => {
-                                       let channel_id = Some(self.channel_id);
+                                       let channel_id = self.channel_id;
                                        let counterparty_node_id = self.counterparty_node_id;
                                        let mut htlc_descriptors = Vec::with_capacity(htlcs.len());
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index dc461c6a9..fac561763 100644
--- a/lightning/src/events/bump_transaction.rs
+++ b/lightning/src/events/bump_transaction.rs
@@ -150,5 +150,5 @@ pub enum BumpTransactionEvent {
        ChannelClose {
                /// The `channel_id` of the channel which has been closed.
-               channel_id: Option<ChannelId>,
+               channel_id: ChannelId,
                /// Counterparty in the closed channel.
                counterparty_node_id: Option<PublicKey>,
@@ -207,5 +207,5 @@ pub enum BumpTransactionEvent {
        HTLCResolution {
                /// The `channel_id` of the channel which has been closed.
-               channel_id: Option<ChannelId>,
+               channel_id: ChannelId,
                /// Counterparty in the closed channel.
                counterparty_node_id: Option<PublicKey>,

[wpaulino]

True, now did that for the channel_id, but we can't for counterparty_node_id I'm afraid as that will only be present for ChannelMonitors serialized with v0.0.110 and above, and we support back to v0.0.99.

Since the event is anchors-specific, which was introduced in 116, it should be fine?

[tnull]

True, now did that for the channel_id, but we can't for counterparty_node_id I'm afraid as that will only be present for ChannelMonitors serialized with v0.0.110 and above, and we support back to v0.0.99.

Since the event is anchors-specific, which was introduced in 116, it should be fine?

But in order to populate it we'd need to call unwrap on ChannelMonitor::counterparty_node_id which would potentially panic, no?

[wpaulino]

ChannelMonitors for anchor channels can only be generated with 0.0.116 or later so the counterparty_node_id should be set.

[tnull]

ChannelMonitors for anchor channels can only be generated with 0.0.116 or later so the counterparty_node_id should be set.

Yeah, you're right. Grrr, really dislike introducing more unwraps, still:

> git diff-tree -U2 3f746a300 5b5c87464
diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index e7e68297a..cadc3524e 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2933,5 +2933,8 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
                                } => {
                                        let channel_id = self.channel_id;
-                                       let counterparty_node_id = self.counterparty_node_id;
+                                       // unwrap safety: `ClaimEvent`s are only available for Anchor channels,
+                                       // introduced with v0.0.116. counterparty_node_id is guaranteed to be `Some`
+                                       // since v0.0.110.
+                                       let counterparty_node_id = self.counterparty_node_id.unwrap();
                                        let commitment_txid = commitment_tx.txid();
                                        debug_assert_eq!(self.current_holder_commitment_tx.txid, commitment_txid);
@@ -2964,5 +2967,8 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
                                } => {
                                        let channel_id = self.channel_id;
-                                       let counterparty_node_id = self.counterparty_node_id;
+                                       // unwrap safety: `ClaimEvent`s are only available for Anchor channels,
+                                       // introduced with v0.0.116. counterparty_node_id is guaranteed to be `Some`
+                                       // since v0.0.110.
+                                       let counterparty_node_id = self.counterparty_node_id.unwrap();
                                        let mut htlc_descriptors = Vec::with_capacity(htlcs.len());
                                        for htlc in htlcs {
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index fac561763..cb011b79a 100644
--- a/lightning/src/events/bump_transaction.rs
+++ b/lightning/src/events/bump_transaction.rs
@@ -152,5 +152,5 @@ pub enum BumpTransactionEvent {
                channel_id: ChannelId,
                /// Counterparty in the closed channel.
-               counterparty_node_id: Option<PublicKey>,
+               counterparty_node_id: PublicKey,
                /// The unique identifier for the claim of the anchor output in the commitment transaction.
                ///
@@ -209,5 +209,5 @@ pub enum BumpTransactionEvent {
                channel_id: ChannelId,
                /// Counterparty in the closed channel.
-               counterparty_node_id: Option<PublicKey>,
+               counterparty_node_id: PublicKey,
                /// The unique identifier for the claim of the HTLCs in the confirmed commitment
                /// transaction.