Expose new BumpChannelClose event for channels with anchor outputs by wpaulino · Pull Request #1689 · lightningdevkit/rust-lightning

wpaulino · 2022-08-31T22:36:00Z

This PR introduces a new BumpChannelClose event to be consumed by users indicating that they should craft a proper child anchor transaction with a sufficient feerate to increase the commitment transaction's (its parent) feerate and help get it confirmed.

Future work will also feature a similar event to bump the fees of HTLC success and timeout transactions.

codecov-commenter · 2022-08-31T23:24:33Z

Codecov Report

Base: 90.79% // Head: 90.49% // Decreases project coverage by -0.29%⚠️

Coverage data is based on head (5478ef6) compared to base (6738fd5).
Patch coverage: 58.70% of modified lines in pull request are covered.

❗ Current head 5478ef6 differs from pull request most recent head e159214. Consider uploading reports for the commit e159214 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1689 +/- ##
==========================================
- Coverage 90.79% 90.49% -0.30% 
==========================================
Files 87 87 Lines 46969 46863 -106 Branches 46969 46863 -106 ==========================================
- Hits 42646 42410 -236 - Misses 4323 4453 +130

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`90.89% <0.00%> (-1.65%)`	⬇️
lightning/src/util/events.rs	`36.95% <0.00%> (-1.19%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.99% <14.28%> (-0.56%)`	⬇️
lightning/src/chain/channelmonitor.rs	`89.77% <52.08%> (-1.44%)`	⬇️
lightning/src/chain/onchaintx.rs	`88.16% <54.16%> (-7.47%)`	⬇️
lightning/src/chain/package.rs	`91.59% <80.80%> (-1.46%)`	⬇️
lightning/src/util/wakers.rs	`86.70% <0.00%> (-4.86%)`	⬇️
lightning/src/chain/mod.rs	`63.63% <0.00%> (-4.55%)`	⬇️
lightning/src/routing/gossip.rs	`91.44% <0.00%> (-0.72%)`	⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Didn't really dig into anything, just looking at the high-level.

wpaulino · 2022-09-14T23:24:05Z

Rebased on latest main and addressed pending comments.

ariard

I'm Approach ACK on both the BumpTransaction enum, the data scope returned back to the user and the event itself. To be looked more, the callsites where we're triggering the event, to see if there is way to simplify or clean paths.

arik-so · 2022-09-18T22:02:38Z

+ log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid());
+ Some(bumped_tx)
+	}
+	pub(crate) fn finalize_untractable_package<L: Deref, Signer: Sign>(


I think "intractable" is the more common form

"untractable" is already used quite a bit across the codebase, I'd prefer having a separate PR to only do the rename.

TheBlueMatt

Some quick comments, will take a further look later.

TheBlueMatt · 2022-09-21T11:28:04Z

+ let package_feerate = Self::compute_feerate(previous_package_feerate, fee_estimator, conf_target);
+ // With the new package feerate obtained, compute the feerate the child transaction must
+ // meet such that the package feerate is met.
+ (package_feerate as u64).checked_mul(2).map(|f| f.checked_sub(parent_feerate)).flatten()


Hmmmm, how do we handle the 1-sat/vb initial fee requirement? If our target ends up being, eg, 1sat/vb can we end up returning something lower here? Also, can we just subtract here? Don't we have to weigh it by the weight of the two transactions?

The issue here is that we don't know the weight of the child transaction yet as it will be created by the event consumer.
After dwelling on #1689 (comment), it may be better to expose a ConfirmationTarget instead of a feerate to indicate the confirmation urgency. The yet to be developed middleware can then handle mapping it to an appropriate feerate as it will have all the context required to do so properly.

Hmm, yea....somehow the consumer has to be able to track the specific transaction, though. We generally try to avoid assuming the fee estimator is correct, or that it responds quickly if the mempool is actively filling up (because some explicitly don't). Thus, we really need to make sure it's easy to track what feerate was used on the previous transaction not just what the target is.
I guess we could just expose the target feerate and let the user do the averaging?

Yeah that should work. We can then expose a helper method that does the averaging for the user.

dunxen · 2022-09-23T14:50:13Z

+#[derive(PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum ClaimEvent {
+	BumpCommitment {
+ target_feerate_sat_per_1000_weight: u32,


nit: Just grepping around and see we have this identifier but with plural sats.
Suggested change
target_feerate_sat_per_1000_weight:u32,
target_feerate_sats_per_1000_weight:u32,

Looks like we just use both interchangeably across the codebase. Would prefer deferring a consistent rename change to a distinct PR.

ariard

Still have to review some of the changes in package.rs, looks good overall.

Thanks for dealing with the rather murky package API, my apologies for the lack of documentation there.

TheBlueMatt

Basically LGTM.

ariard

I think we're good though I'm blocking a bit as using #[cfg(test)] TODO: remove-on-anchors-release to feature gate anchor output support. I can't remember we have gradually release sophisticated feature in the past, so sounds a first time. While I think it's worthy to do so in the present case I don't know if it's the best approach, see comment.

ariard · 2022-10-09T21:23:30Z

+ let commitment_package = PackageTemplate::build_package(
+ self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
+ PackageSolvingData::HolderFundingOutput(funding_output),
+ best_block_height, false, best_block_height,


For builders/helpers in safety-critical parts, we could start to document the boolean argument at the caller call-site, i.e "/* aggregable */ false". It sounds a code style sophistication, though we already do that on the Core-side. As the codebase keeps growing I think this could be a good practice as a argument misusage could introduce a logical bug. E.g, aggregating a PackageMalleability::Untractable and a PackageMalleability::Malleable together triggering a panic!(). Already seen that type of bug sticking for years in Core.

Will address this as a separate PR.

ariard · 2022-10-09T23:26:04Z

+ // attempt to broadcast the transaction with its current fee rate and hope
+ // it confirms. This is essentially the same behavior as a commitment
+ // transaction without anchor outputs.
+ None => Some((None, 0, OnchainClaim::Tx(tx.clone()))),


IIRC, the case where we don't find a second anchor output for the channel is when we have no fund at stakes in the channel (no-htlc, no-balance) output. We could consider the channel early closed and return a OnchainEvent::ChannelClosed, to allow consumer to react in consequence.

I like the idea, but seems out of scope for this PR.

ariard · 2022-10-09T23:50:48Z

@@ -2299,6 +2321,39 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 	pub fn get_and_clear_pending_events(&mut self) -> Vec<Event> {


I think we should update ChainMonitor::process_pending_events in consequence, noticing the user that this call is from now on important for the funds safety and call frequence should at least match every block ?

Done. I wonder if the ChainMonitor should just do this itself if any anchor channels are present. Would it be considered a violation of the events API?

Yeah we can do in a follow-up.

wpaulino · 2022-10-11T20:25:48Z

Had to rebase on main to address a conflict. The previous push includes changes addressing latest round of feedback.

ariard

ACK e159214

ariard

ACK c963d3a

TheBlueMatt

LGTM. There's a few followups but ideally let's just land this and we can do them later.

TheBlueMatt · 2022-10-17T15:01:48Z

+ Some((idx, _)) => {
+ // Our target feerate will depend on whether we have any HTLCs present
+ // within our commitment.
+ let conf_target = if self.holder_commitment.trust().htlcs().is_empty() {


We may have a unrevoked counterparty commitment that does contain HTLCs, I think, so we should check for that or just always use highpriority. Happy to see it as a followup so we can land this.

Since the counterparty commitment isn't exposed to the OnchainTxHandler, I reverted to always using HighPriority and added a todo.

TheBlueMatt · 2022-10-17T15:44:31Z

+ &Event::BumpTransaction(ref event)=> {
+ 27u8.write(writer)?;
+ match event {
+ // We never write the ChannelClose events as they'll be replayed upon restarting


I'm a little dubious of the never-write here. While we should generate a new bump eventually, we may take a few blocks to do so, and dropping the event may waste a few blocks' time, which seems bad?

I was thinking it would be fine as long as we always use a soonest_conf_deadline of our best known height (which is currently the case). get_height_timer will then force a retry at the next block.

Yea, we should reconsider that at some point, though, IIUC that will do an RBF bump at every block, which may mean bumping the fee by 25% every block, which seems like a lot? Not something for this PR, though.

TheBlueMatt · 2022-10-17T17:37:31Z

Oops, needs rebase now, sorry about that. Feel free to tackle some of the above when you do so.

As we integrate the support of anchor outputs, we'll want to know if each input we're working with came from an anchor outputs channel. Instead of threading through a `opt_anchors` boolean across several methods on `PackageSolvingData` and `PackageTemplate`, we decide to store a reference in each `PackageSolvingData` variant instead that features a change in behavior between channels with and without anchor outputs.

This will be useful later on when determining the appropriate fee rate to use on the anchor transaction to bump its commitment transaction.

wpaulino · 2022-08-31T22:36:00Z

This PR introduces a new BumpChannelClose event to be consumed by users indicating that they should craft a proper child anchor transaction with a sufficient feerate to increase the commitment transaction's (its parent) feerate and help get it confirmed.

Future work will also feature a similar event to bump the fees of HTLC success and timeout transactions.

codecov-commenter · 2022-08-31T23:24:33Z

Codecov Report

Base: 90.79% // Head: 90.49% // Decreases project coverage by -0.29%⚠️

Coverage data is based on head (5478ef6) compared to base (6738fd5).
Patch coverage: 58.70% of modified lines in pull request are covered.

❗ Current head 5478ef6 differs from pull request most recent head e159214. Consider uploading reports for the commit e159214 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1689 +/- ##
==========================================
- Coverage 90.79% 90.49% -0.30% 
==========================================
Files 87 87 Lines 46969 46863 -106 Branches 46969 46863 -106 ==========================================
- Hits 42646 42410 -236 - Misses 4323 4453 +130

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`90.89% <0.00%> (-1.65%)`	⬇️
lightning/src/util/events.rs	`36.95% <0.00%> (-1.19%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.99% <14.28%> (-0.56%)`	⬇️
lightning/src/chain/channelmonitor.rs	`89.77% <52.08%> (-1.44%)`	⬇️
lightning/src/chain/onchaintx.rs	`88.16% <54.16%> (-7.47%)`	⬇️
lightning/src/chain/package.rs	`91.59% <80.80%> (-1.46%)`	⬇️
lightning/src/util/wakers.rs	`86.70% <0.00%> (-4.86%)`	⬇️
lightning/src/chain/mod.rs	`63.63% <0.00%> (-4.55%)`	⬇️
lightning/src/routing/gossip.rs	`91.44% <0.00%> (-0.72%)`	⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Didn't really dig into anything, just looking at the high-level.

wpaulino · 2022-09-14T23:24:05Z

Rebased on latest main and addressed pending comments.

ariard

I'm Approach ACK on both the BumpTransaction enum, the data scope returned back to the user and the event itself. To be looked more, the callsites where we're triggering the event, to see if there is way to simplify or clean paths.

arik-so · 2022-09-18T22:02:38Z

+ log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid());
+ Some(bumped_tx)
+	}
+	pub(crate) fn finalize_untractable_package<L: Deref, Signer: Sign>(


I think "intractable" is the more common form

"untractable" is already used quite a bit across the codebase, I'd prefer having a separate PR to only do the rename.

TheBlueMatt

Some quick comments, will take a further look later.

TheBlueMatt · 2022-09-21T11:28:04Z

+ let package_feerate = Self::compute_feerate(previous_package_feerate, fee_estimator, conf_target);
+ // With the new package feerate obtained, compute the feerate the child transaction must
+ // meet such that the package feerate is met.
+ (package_feerate as u64).checked_mul(2).map(|f| f.checked_sub(parent_feerate)).flatten()


Hmmmm, how do we handle the 1-sat/vb initial fee requirement? If our target ends up being, eg, 1sat/vb can we end up returning something lower here? Also, can we just subtract here? Don't we have to weigh it by the weight of the two transactions?

The issue here is that we don't know the weight of the child transaction yet as it will be created by the event consumer.
After dwelling on #1689 (comment), it may be better to expose a ConfirmationTarget instead of a feerate to indicate the confirmation urgency. The yet to be developed middleware can then handle mapping it to an appropriate feerate as it will have all the context required to do so properly.

Hmm, yea....somehow the consumer has to be able to track the specific transaction, though. We generally try to avoid assuming the fee estimator is correct, or that it responds quickly if the mempool is actively filling up (because some explicitly don't). Thus, we really need to make sure it's easy to track what feerate was used on the previous transaction not just what the target is.
I guess we could just expose the target feerate and let the user do the averaging?

Yeah that should work. We can then expose a helper method that does the averaging for the user.

dunxen · 2022-09-23T14:50:13Z

+#[derive(PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum ClaimEvent {
+	BumpCommitment {
+ target_feerate_sat_per_1000_weight: u32,


nit: Just grepping around and see we have this identifier but with plural sats.
Suggested change
target_feerate_sat_per_1000_weight:u32,
target_feerate_sats_per_1000_weight:u32,

Looks like we just use both interchangeably across the codebase. Would prefer deferring a consistent rename change to a distinct PR.

ariard

Still have to review some of the changes in package.rs, looks good overall.

Thanks for dealing with the rather murky package API, my apologies for the lack of documentation there.

TheBlueMatt

Basically LGTM.

ariard

I think we're good though I'm blocking a bit as using #[cfg(test)] TODO: remove-on-anchors-release to feature gate anchor output support. I can't remember we have gradually release sophisticated feature in the past, so sounds a first time. While I think it's worthy to do so in the present case I don't know if it's the best approach, see comment.

ariard · 2022-10-09T21:23:30Z

+ let commitment_package = PackageTemplate::build_package(
+ self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
+ PackageSolvingData::HolderFundingOutput(funding_output),
+ best_block_height, false, best_block_height,


For builders/helpers in safety-critical parts, we could start to document the boolean argument at the caller call-site, i.e "/* aggregable */ false". It sounds a code style sophistication, though we already do that on the Core-side. As the codebase keeps growing I think this could be a good practice as a argument misusage could introduce a logical bug. E.g, aggregating a PackageMalleability::Untractable and a PackageMalleability::Malleable together triggering a panic!(). Already seen that type of bug sticking for years in Core.

Will address this as a separate PR.

ariard · 2022-10-09T23:26:04Z

+ // attempt to broadcast the transaction with its current fee rate and hope
+ // it confirms. This is essentially the same behavior as a commitment
+ // transaction without anchor outputs.
+ None => Some((None, 0, OnchainClaim::Tx(tx.clone()))),


IIRC, the case where we don't find a second anchor output for the channel is when we have no fund at stakes in the channel (no-htlc, no-balance) output. We could consider the channel early closed and return a OnchainEvent::ChannelClosed, to allow consumer to react in consequence.

I like the idea, but seems out of scope for this PR.

ariard · 2022-10-09T23:50:48Z

@@ -2299,6 +2321,39 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 	pub fn get_and_clear_pending_events(&mut self) -> Vec<Event> {


I think we should update ChainMonitor::process_pending_events in consequence, noticing the user that this call is from now on important for the funds safety and call frequence should at least match every block ?

Done. I wonder if the ChainMonitor should just do this itself if any anchor channels are present. Would it be considered a violation of the events API?

Yeah we can do in a follow-up.

wpaulino · 2022-10-11T20:25:48Z

Had to rebase on main to address a conflict. The previous push includes changes addressing latest round of feedback.

ariard

ACK e159214

ariard

ACK c963d3a

TheBlueMatt

LGTM. There's a few followups but ideally let's just land this and we can do them later.

TheBlueMatt · 2022-10-17T15:01:48Z

+ Some((idx, _)) => {
+ // Our target feerate will depend on whether we have any HTLCs present
+ // within our commitment.
+ let conf_target = if self.holder_commitment.trust().htlcs().is_empty() {


We may have a unrevoked counterparty commitment that does contain HTLCs, I think, so we should check for that or just always use highpriority. Happy to see it as a followup so we can land this.

Since the counterparty commitment isn't exposed to the OnchainTxHandler, I reverted to always using HighPriority and added a todo.

TheBlueMatt · 2022-10-17T15:44:31Z

+ &Event::BumpTransaction(ref event)=> {
+ 27u8.write(writer)?;
+ match event {
+ // We never write the ChannelClose events as they'll be replayed upon restarting


I'm a little dubious of the never-write here. While we should generate a new bump eventually, we may take a few blocks to do so, and dropping the event may waste a few blocks' time, which seems bad?

I was thinking it would be fine as long as we always use a soonest_conf_deadline of our best known height (which is currently the case). get_height_timer will then force a retry at the next block.

Yea, we should reconsider that at some point, though, IIUC that will do an RBF bump at every block, which may mean bumping the fee by 25% every block, which seems like a lot? Not something for this PR, though.

TheBlueMatt · 2022-10-17T17:37:31Z

Oops, needs rebase now, sorry about that. Feel free to tackle some of the above when you do so.

As we integrate the support of anchor outputs, we'll want to know if each input we're working with came from an anchor outputs channel. Instead of threading through a `opt_anchors` boolean across several methods on `PackageSolvingData` and `PackageTemplate`, we decide to store a reference in each `PackageSolvingData` variant instead that features a change in behavior between channels with and without anchor outputs.

This will be useful later on when determining the appropriate fee rate to use on the anchor transaction to bump its commitment transaction.

wpaulino · 2022-08-31T22:36:00Z

This PR introduces a new BumpChannelClose event to be consumed by users indicating that they should craft a proper child anchor transaction with a sufficient feerate to increase the commitment transaction's (its parent) feerate and help get it confirmed.

Future work will also feature a similar event to bump the fees of HTLC success and timeout transactions.

codecov-commenter · 2022-08-31T23:24:33Z

Codecov Report

Base: 90.79% // Head: 90.49% // Decreases project coverage by -0.29%⚠️

Coverage data is based on head (5478ef6) compared to base (6738fd5).
Patch coverage: 58.70% of modified lines in pull request are covered.

❗ Current head 5478ef6 differs from pull request most recent head e159214. Consider uploading reports for the commit e159214 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1689 +/- ##
==========================================
- Coverage 90.79% 90.49% -0.30% 
==========================================
Files 87 87 Lines 46969 46863 -106 Branches 46969 46863 -106 ==========================================
- Hits 42646 42410 -236 - Misses 4323 4453 +130

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`90.89% <0.00%> (-1.65%)`	⬇️
lightning/src/util/events.rs	`36.95% <0.00%> (-1.19%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.99% <14.28%> (-0.56%)`	⬇️
lightning/src/chain/channelmonitor.rs	`89.77% <52.08%> (-1.44%)`	⬇️
lightning/src/chain/onchaintx.rs	`88.16% <54.16%> (-7.47%)`	⬇️
lightning/src/chain/package.rs	`91.59% <80.80%> (-1.46%)`	⬇️
lightning/src/util/wakers.rs	`86.70% <0.00%> (-4.86%)`	⬇️
lightning/src/chain/mod.rs	`63.63% <0.00%> (-4.55%)`	⬇️
lightning/src/routing/gossip.rs	`91.44% <0.00%> (-0.72%)`	⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Didn't really dig into anything, just looking at the high-level.

wpaulino · 2022-09-14T23:24:05Z

Rebased on latest main and addressed pending comments.

ariard

I'm Approach ACK on both the BumpTransaction enum, the data scope returned back to the user and the event itself. To be looked more, the callsites where we're triggering the event, to see if there is way to simplify or clean paths.

arik-so · 2022-09-18T22:02:38Z

+ log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid());
+ Some(bumped_tx)
+	}
+	pub(crate) fn finalize_untractable_package<L: Deref, Signer: Sign>(


I think "intractable" is the more common form

"untractable" is already used quite a bit across the codebase, I'd prefer having a separate PR to only do the rename.

TheBlueMatt

Some quick comments, will take a further look later.

TheBlueMatt · 2022-09-21T11:28:04Z

+ let package_feerate = Self::compute_feerate(previous_package_feerate, fee_estimator, conf_target);
+ // With the new package feerate obtained, compute the feerate the child transaction must
+ // meet such that the package feerate is met.
+ (package_feerate as u64).checked_mul(2).map(|f| f.checked_sub(parent_feerate)).flatten()


Hmmmm, how do we handle the 1-sat/vb initial fee requirement? If our target ends up being, eg, 1sat/vb can we end up returning something lower here? Also, can we just subtract here? Don't we have to weigh it by the weight of the two transactions?

The issue here is that we don't know the weight of the child transaction yet as it will be created by the event consumer.
After dwelling on #1689 (comment), it may be better to expose a ConfirmationTarget instead of a feerate to indicate the confirmation urgency. The yet to be developed middleware can then handle mapping it to an appropriate feerate as it will have all the context required to do so properly.

Hmm, yea....somehow the consumer has to be able to track the specific transaction, though. We generally try to avoid assuming the fee estimator is correct, or that it responds quickly if the mempool is actively filling up (because some explicitly don't). Thus, we really need to make sure it's easy to track what feerate was used on the previous transaction not just what the target is.
I guess we could just expose the target feerate and let the user do the averaging?

Yeah that should work. We can then expose a helper method that does the averaging for the user.

dunxen · 2022-09-23T14:50:13Z

+#[derive(PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum ClaimEvent {
+	BumpCommitment {
+ target_feerate_sat_per_1000_weight: u32,


nit: Just grepping around and see we have this identifier but with plural sats.
Suggested change
target_feerate_sat_per_1000_weight:u32,
target_feerate_sats_per_1000_weight:u32,

Looks like we just use both interchangeably across the codebase. Would prefer deferring a consistent rename change to a distinct PR.

ariard

Still have to review some of the changes in package.rs, looks good overall.

Thanks for dealing with the rather murky package API, my apologies for the lack of documentation there.

TheBlueMatt

Basically LGTM.

ariard

I think we're good though I'm blocking a bit as using #[cfg(test)] TODO: remove-on-anchors-release to feature gate anchor output support. I can't remember we have gradually release sophisticated feature in the past, so sounds a first time. While I think it's worthy to do so in the present case I don't know if it's the best approach, see comment.

ariard · 2022-10-09T21:23:30Z

+ let commitment_package = PackageTemplate::build_package(
+ self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
+ PackageSolvingData::HolderFundingOutput(funding_output),
+ best_block_height, false, best_block_height,


For builders/helpers in safety-critical parts, we could start to document the boolean argument at the caller call-site, i.e "/* aggregable */ false". It sounds a code style sophistication, though we already do that on the Core-side. As the codebase keeps growing I think this could be a good practice as a argument misusage could introduce a logical bug. E.g, aggregating a PackageMalleability::Untractable and a PackageMalleability::Malleable together triggering a panic!(). Already seen that type of bug sticking for years in Core.

Will address this as a separate PR.

ariard · 2022-10-09T23:26:04Z

+ // attempt to broadcast the transaction with its current fee rate and hope
+ // it confirms. This is essentially the same behavior as a commitment
+ // transaction without anchor outputs.
+ None => Some((None, 0, OnchainClaim::Tx(tx.clone()))),


IIRC, the case where we don't find a second anchor output for the channel is when we have no fund at stakes in the channel (no-htlc, no-balance) output. We could consider the channel early closed and return a OnchainEvent::ChannelClosed, to allow consumer to react in consequence.

I like the idea, but seems out of scope for this PR.

ariard · 2022-10-09T23:50:48Z

@@ -2299,6 +2321,39 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 	pub fn get_and_clear_pending_events(&mut self) -> Vec<Event> {


I think we should update ChainMonitor::process_pending_events in consequence, noticing the user that this call is from now on important for the funds safety and call frequence should at least match every block ?

Done. I wonder if the ChainMonitor should just do this itself if any anchor channels are present. Would it be considered a violation of the events API?

Yeah we can do in a follow-up.

wpaulino · 2022-10-11T20:25:48Z

Had to rebase on main to address a conflict. The previous push includes changes addressing latest round of feedback.

ariard

ACK e159214

ariard

ACK c963d3a

TheBlueMatt

LGTM. There's a few followups but ideally let's just land this and we can do them later.

TheBlueMatt · 2022-10-17T15:01:48Z

+ Some((idx, _)) => {
+ // Our target feerate will depend on whether we have any HTLCs present
+ // within our commitment.
+ let conf_target = if self.holder_commitment.trust().htlcs().is_empty() {


We may have a unrevoked counterparty commitment that does contain HTLCs, I think, so we should check for that or just always use highpriority. Happy to see it as a followup so we can land this.

Since the counterparty commitment isn't exposed to the OnchainTxHandler, I reverted to always using HighPriority and added a todo.

TheBlueMatt · 2022-10-17T15:44:31Z

+ &Event::BumpTransaction(ref event)=> {
+ 27u8.write(writer)?;
+ match event {
+ // We never write the ChannelClose events as they'll be replayed upon restarting


I'm a little dubious of the never-write here. While we should generate a new bump eventually, we may take a few blocks to do so, and dropping the event may waste a few blocks' time, which seems bad?

I was thinking it would be fine as long as we always use a soonest_conf_deadline of our best known height (which is currently the case). get_height_timer will then force a retry at the next block.

Yea, we should reconsider that at some point, though, IIUC that will do an RBF bump at every block, which may mean bumping the fee by 25% every block, which seems like a lot? Not something for this PR, though.

TheBlueMatt · 2022-10-17T17:37:31Z

Oops, needs rebase now, sorry about that. Feel free to tackle some of the above when you do so.

As we integrate the support of anchor outputs, we'll want to know if each input we're working with came from an anchor outputs channel. Instead of threading through a `opt_anchors` boolean across several methods on `PackageSolvingData` and `PackageTemplate`, we decide to store a reference in each `PackageSolvingData` variant instead that features a change in behavior between channels with and without anchor outputs.

This will be useful later on when determining the appropriate fee rate to use on the anchor transaction to bump its commitment transaction.

wpaulino · 2022-08-31T22:36:00Z

This PR introduces a new BumpChannelClose event to be consumed by users indicating that they should craft a proper child anchor transaction with a sufficient feerate to increase the commitment transaction's (its parent) feerate and help get it confirmed.

Future work will also feature a similar event to bump the fees of HTLC success and timeout transactions.

codecov-commenter · 2022-08-31T23:24:33Z

Codecov Report

Base: 90.79% // Head: 90.49% // Decreases project coverage by -0.29%⚠️

Coverage data is based on head (5478ef6) compared to base (6738fd5).
Patch coverage: 58.70% of modified lines in pull request are covered.

❗ Current head 5478ef6 differs from pull request most recent head e159214. Consider uploading reports for the commit e159214 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1689 +/- ##
==========================================
- Coverage 90.79% 90.49% -0.30% 
==========================================
Files 87 87 Lines 46969 46863 -106 Branches 46969 46863 -106 ==========================================
- Hits 42646 42410 -236 - Misses 4323 4453 +130

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`90.89% <0.00%> (-1.65%)`	⬇️
lightning/src/util/events.rs	`36.95% <0.00%> (-1.19%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.99% <14.28%> (-0.56%)`	⬇️
lightning/src/chain/channelmonitor.rs	`89.77% <52.08%> (-1.44%)`	⬇️
lightning/src/chain/onchaintx.rs	`88.16% <54.16%> (-7.47%)`	⬇️
lightning/src/chain/package.rs	`91.59% <80.80%> (-1.46%)`	⬇️
lightning/src/util/wakers.rs	`86.70% <0.00%> (-4.86%)`	⬇️
lightning/src/chain/mod.rs	`63.63% <0.00%> (-4.55%)`	⬇️
lightning/src/routing/gossip.rs	`91.44% <0.00%> (-0.72%)`	⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Didn't really dig into anything, just looking at the high-level.

wpaulino · 2022-09-14T23:24:05Z

Rebased on latest main and addressed pending comments.

ariard

I'm Approach ACK on both the BumpTransaction enum, the data scope returned back to the user and the event itself. To be looked more, the callsites where we're triggering the event, to see if there is way to simplify or clean paths.

arik-so · 2022-09-18T22:02:38Z

+ log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid());
+ Some(bumped_tx)
+	}
+	pub(crate) fn finalize_untractable_package<L: Deref, Signer: Sign>(


I think "intractable" is the more common form

"untractable" is already used quite a bit across the codebase, I'd prefer having a separate PR to only do the rename.

TheBlueMatt

Some quick comments, will take a further look later.

TheBlueMatt · 2022-09-21T11:28:04Z

+ let package_feerate = Self::compute_feerate(previous_package_feerate, fee_estimator, conf_target);
+ // With the new package feerate obtained, compute the feerate the child transaction must
+ // meet such that the package feerate is met.
+ (package_feerate as u64).checked_mul(2).map(|f| f.checked_sub(parent_feerate)).flatten()


Hmmmm, how do we handle the 1-sat/vb initial fee requirement? If our target ends up being, eg, 1sat/vb can we end up returning something lower here? Also, can we just subtract here? Don't we have to weigh it by the weight of the two transactions?

The issue here is that we don't know the weight of the child transaction yet as it will be created by the event consumer.
After dwelling on #1689 (comment), it may be better to expose a ConfirmationTarget instead of a feerate to indicate the confirmation urgency. The yet to be developed middleware can then handle mapping it to an appropriate feerate as it will have all the context required to do so properly.

Hmm, yea....somehow the consumer has to be able to track the specific transaction, though. We generally try to avoid assuming the fee estimator is correct, or that it responds quickly if the mempool is actively filling up (because some explicitly don't). Thus, we really need to make sure it's easy to track what feerate was used on the previous transaction not just what the target is.
I guess we could just expose the target feerate and let the user do the averaging?

Yeah that should work. We can then expose a helper method that does the averaging for the user.

dunxen · 2022-09-23T14:50:13Z

+#[derive(PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum ClaimEvent {
+	BumpCommitment {
+ target_feerate_sat_per_1000_weight: u32,


nit: Just grepping around and see we have this identifier but with plural sats.
Suggested change
target_feerate_sat_per_1000_weight:u32,
target_feerate_sats_per_1000_weight:u32,

Looks like we just use both interchangeably across the codebase. Would prefer deferring a consistent rename change to a distinct PR.

ariard

Still have to review some of the changes in package.rs, looks good overall.

Thanks for dealing with the rather murky package API, my apologies for the lack of documentation there.

TheBlueMatt

Basically LGTM.

ariard

I think we're good though I'm blocking a bit as using #[cfg(test)] TODO: remove-on-anchors-release to feature gate anchor output support. I can't remember we have gradually release sophisticated feature in the past, so sounds a first time. While I think it's worthy to do so in the present case I don't know if it's the best approach, see comment.

ariard · 2022-10-09T21:23:30Z

+ let commitment_package = PackageTemplate::build_package(
+ self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
+ PackageSolvingData::HolderFundingOutput(funding_output),
+ best_block_height, false, best_block_height,


For builders/helpers in safety-critical parts, we could start to document the boolean argument at the caller call-site, i.e "/* aggregable */ false". It sounds a code style sophistication, though we already do that on the Core-side. As the codebase keeps growing I think this could be a good practice as a argument misusage could introduce a logical bug. E.g, aggregating a PackageMalleability::Untractable and a PackageMalleability::Malleable together triggering a panic!(). Already seen that type of bug sticking for years in Core.

Will address this as a separate PR.

ariard · 2022-10-09T23:26:04Z

+ // attempt to broadcast the transaction with its current fee rate and hope
+ // it confirms. This is essentially the same behavior as a commitment
+ // transaction without anchor outputs.
+ None => Some((None, 0, OnchainClaim::Tx(tx.clone()))),


IIRC, the case where we don't find a second anchor output for the channel is when we have no fund at stakes in the channel (no-htlc, no-balance) output. We could consider the channel early closed and return a OnchainEvent::ChannelClosed, to allow consumer to react in consequence.

I like the idea, but seems out of scope for this PR.

ariard · 2022-10-09T23:50:48Z

@@ -2299,6 +2321,39 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 	pub fn get_and_clear_pending_events(&mut self) -> Vec<Event> {


I think we should update ChainMonitor::process_pending_events in consequence, noticing the user that this call is from now on important for the funds safety and call frequence should at least match every block ?

Done. I wonder if the ChainMonitor should just do this itself if any anchor channels are present. Would it be considered a violation of the events API?

Yeah we can do in a follow-up.

wpaulino · 2022-10-11T20:25:48Z

Had to rebase on main to address a conflict. The previous push includes changes addressing latest round of feedback.

ariard

ACK e159214

ariard

ACK c963d3a

TheBlueMatt

LGTM. There's a few followups but ideally let's just land this and we can do them later.

TheBlueMatt · 2022-10-17T15:01:48Z

+ Some((idx, _)) => {
+ // Our target feerate will depend on whether we have any HTLCs present
+ // within our commitment.
+ let conf_target = if self.holder_commitment.trust().htlcs().is_empty() {


We may have a unrevoked counterparty commitment that does contain HTLCs, I think, so we should check for that or just always use highpriority. Happy to see it as a followup so we can land this.

Since the counterparty commitment isn't exposed to the OnchainTxHandler, I reverted to always using HighPriority and added a todo.

TheBlueMatt · 2022-10-17T15:44:31Z

+ &Event::BumpTransaction(ref event)=> {
+ 27u8.write(writer)?;
+ match event {
+ // We never write the ChannelClose events as they'll be replayed upon restarting


I'm a little dubious of the never-write here. While we should generate a new bump eventually, we may take a few blocks to do so, and dropping the event may waste a few blocks' time, which seems bad?

I was thinking it would be fine as long as we always use a soonest_conf_deadline of our best known height (which is currently the case). get_height_timer will then force a retry at the next block.

Yea, we should reconsider that at some point, though, IIUC that will do an RBF bump at every block, which may mean bumping the fee by 25% every block, which seems like a lot? Not something for this PR, though.

TheBlueMatt · 2022-10-17T17:37:31Z

Oops, needs rebase now, sorry about that. Feel free to tackle some of the above when you do so.

As we integrate the support of anchor outputs, we'll want to know if each input we're working with came from an anchor outputs channel. Instead of threading through a `opt_anchors` boolean across several methods on `PackageSolvingData` and `PackageTemplate`, we decide to store a reference in each `PackageSolvingData` variant instead that features a change in behavior between channels with and without anchor outputs.

This will be useful later on when determining the appropriate fee rate to use on the anchor transaction to bump its commitment transaction.

wpaulino · 2022-08-31T22:36:00Z

This PR introduces a new BumpChannelClose event to be consumed by users indicating that they should craft a proper child anchor transaction with a sufficient feerate to increase the commitment transaction's (its parent) feerate and help get it confirmed.

Future work will also feature a similar event to bump the fees of HTLC success and timeout transactions.

codecov-commenter · 2022-08-31T23:24:33Z

Codecov Report

Base: 90.79% // Head: 90.49% // Decreases project coverage by -0.29%⚠️

Coverage data is based on head (5478ef6) compared to base (6738fd5).
Patch coverage: 58.70% of modified lines in pull request are covered.

❗ Current head 5478ef6 differs from pull request most recent head e159214. Consider uploading reports for the commit e159214 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1689 +/- ##
==========================================
- Coverage 90.79% 90.49% -0.30% 
==========================================
Files 87 87 Lines 46969 46863 -106 Branches 46969 46863 -106 ==========================================
- Hits 42646 42410 -236 - Misses 4323 4453 +130

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`90.89% <0.00%> (-1.65%)`	⬇️
lightning/src/util/events.rs	`36.95% <0.00%> (-1.19%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.99% <14.28%> (-0.56%)`	⬇️
lightning/src/chain/channelmonitor.rs	`89.77% <52.08%> (-1.44%)`	⬇️
lightning/src/chain/onchaintx.rs	`88.16% <54.16%> (-7.47%)`	⬇️
lightning/src/chain/package.rs	`91.59% <80.80%> (-1.46%)`	⬇️
lightning/src/util/wakers.rs	`86.70% <0.00%> (-4.86%)`	⬇️
lightning/src/chain/mod.rs	`63.63% <0.00%> (-4.55%)`	⬇️
lightning/src/routing/gossip.rs	`91.44% <0.00%> (-0.72%)`	⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Didn't really dig into anything, just looking at the high-level.

wpaulino · 2022-09-14T23:24:05Z

Rebased on latest main and addressed pending comments.

ariard

I'm Approach ACK on both the BumpTransaction enum, the data scope returned back to the user and the event itself. To be looked more, the callsites where we're triggering the event, to see if there is way to simplify or clean paths.

arik-so · 2022-09-18T22:02:38Z

+ log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid());
+ Some(bumped_tx)
+	}
+	pub(crate) fn finalize_untractable_package<L: Deref, Signer: Sign>(


I think "intractable" is the more common form

"untractable" is already used quite a bit across the codebase, I'd prefer having a separate PR to only do the rename.

TheBlueMatt

Some quick comments, will take a further look later.

TheBlueMatt · 2022-09-21T11:28:04Z

+ let package_feerate = Self::compute_feerate(previous_package_feerate, fee_estimator, conf_target);
+ // With the new package feerate obtained, compute the feerate the child transaction must
+ // meet such that the package feerate is met.
+ (package_feerate as u64).checked_mul(2).map(|f| f.checked_sub(parent_feerate)).flatten()


Hmmmm, how do we handle the 1-sat/vb initial fee requirement? If our target ends up being, eg, 1sat/vb can we end up returning something lower here? Also, can we just subtract here? Don't we have to weigh it by the weight of the two transactions?

The issue here is that we don't know the weight of the child transaction yet as it will be created by the event consumer.
After dwelling on #1689 (comment), it may be better to expose a ConfirmationTarget instead of a feerate to indicate the confirmation urgency. The yet to be developed middleware can then handle mapping it to an appropriate feerate as it will have all the context required to do so properly.

Hmm, yea....somehow the consumer has to be able to track the specific transaction, though. We generally try to avoid assuming the fee estimator is correct, or that it responds quickly if the mempool is actively filling up (because some explicitly don't). Thus, we really need to make sure it's easy to track what feerate was used on the previous transaction not just what the target is.
I guess we could just expose the target feerate and let the user do the averaging?

Yeah that should work. We can then expose a helper method that does the averaging for the user.

dunxen · 2022-09-23T14:50:13Z

+#[derive(PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum ClaimEvent {
+	BumpCommitment {
+ target_feerate_sat_per_1000_weight: u32,


nit: Just grepping around and see we have this identifier but with plural sats.
Suggested change
target_feerate_sat_per_1000_weight:u32,
target_feerate_sats_per_1000_weight:u32,

Looks like we just use both interchangeably across the codebase. Would prefer deferring a consistent rename change to a distinct PR.

ariard

Still have to review some of the changes in package.rs, looks good overall.

Thanks for dealing with the rather murky package API, my apologies for the lack of documentation there.

TheBlueMatt

Basically LGTM.

ariard

I think we're good though I'm blocking a bit as using #[cfg(test)] TODO: remove-on-anchors-release to feature gate anchor output support. I can't remember we have gradually release sophisticated feature in the past, so sounds a first time. While I think it's worthy to do so in the present case I don't know if it's the best approach, see comment.

ariard · 2022-10-09T21:23:30Z

+ let commitment_package = PackageTemplate::build_package(
+ self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
+ PackageSolvingData::HolderFundingOutput(funding_output),
+ best_block_height, false, best_block_height,


For builders/helpers in safety-critical parts, we could start to document the boolean argument at the caller call-site, i.e "/* aggregable */ false". It sounds a code style sophistication, though we already do that on the Core-side. As the codebase keeps growing I think this could be a good practice as a argument misusage could introduce a logical bug. E.g, aggregating a PackageMalleability::Untractable and a PackageMalleability::Malleable together triggering a panic!(). Already seen that type of bug sticking for years in Core.

Will address this as a separate PR.

ariard · 2022-10-09T23:26:04Z

+ // attempt to broadcast the transaction with its current fee rate and hope
+ // it confirms. This is essentially the same behavior as a commitment
+ // transaction without anchor outputs.
+ None => Some((None, 0, OnchainClaim::Tx(tx.clone()))),


IIRC, the case where we don't find a second anchor output for the channel is when we have no fund at stakes in the channel (no-htlc, no-balance) output. We could consider the channel early closed and return a OnchainEvent::ChannelClosed, to allow consumer to react in consequence.

I like the idea, but seems out of scope for this PR.

ariard · 2022-10-09T23:50:48Z

@@ -2299,6 +2321,39 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 	pub fn get_and_clear_pending_events(&mut self) -> Vec<Event> {


I think we should update ChainMonitor::process_pending_events in consequence, noticing the user that this call is from now on important for the funds safety and call frequence should at least match every block ?

Done. I wonder if the ChainMonitor should just do this itself if any anchor channels are present. Would it be considered a violation of the events API?

Yeah we can do in a follow-up.

wpaulino · 2022-10-11T20:25:48Z

Had to rebase on main to address a conflict. The previous push includes changes addressing latest round of feedback.

ariard

ACK e159214

ariard

ACK c963d3a

TheBlueMatt

LGTM. There's a few followups but ideally let's just land this and we can do them later.

TheBlueMatt · 2022-10-17T15:01:48Z

+ Some((idx, _)) => {
+ // Our target feerate will depend on whether we have any HTLCs present
+ // within our commitment.
+ let conf_target = if self.holder_commitment.trust().htlcs().is_empty() {


We may have a unrevoked counterparty commitment that does contain HTLCs, I think, so we should check for that or just always use highpriority. Happy to see it as a followup so we can land this.

Since the counterparty commitment isn't exposed to the OnchainTxHandler, I reverted to always using HighPriority and added a todo.

TheBlueMatt · 2022-10-17T15:44:31Z

+ &Event::BumpTransaction(ref event)=> {
+ 27u8.write(writer)?;
+ match event {
+ // We never write the ChannelClose events as they'll be replayed upon restarting


I'm a little dubious of the never-write here. While we should generate a new bump eventually, we may take a few blocks to do so, and dropping the event may waste a few blocks' time, which seems bad?

I was thinking it would be fine as long as we always use a soonest_conf_deadline of our best known height (which is currently the case). get_height_timer will then force a retry at the next block.

Yea, we should reconsider that at some point, though, IIUC that will do an RBF bump at every block, which may mean bumping the fee by 25% every block, which seems like a lot? Not something for this PR, though.

TheBlueMatt · 2022-10-17T17:37:31Z

Oops, needs rebase now, sorry about that. Feel free to tackle some of the above when you do so.

As we integrate the support of anchor outputs, we'll want to know if each input we're working with came from an anchor outputs channel. Instead of threading through a `opt_anchors` boolean across several methods on `PackageSolvingData` and `PackageTemplate`, we decide to store a reference in each `PackageSolvingData` variant instead that features a change in behavior between channels with and without anchor outputs.

This will be useful later on when determining the appropriate fee rate to use on the anchor transaction to bump its commitment transaction.

wpaulino · 2022-08-31T22:36:00Z

This PR introduces a new BumpChannelClose event to be consumed by users indicating that they should craft a proper child anchor transaction with a sufficient feerate to increase the commitment transaction's (its parent) feerate and help get it confirmed.

Future work will also feature a similar event to bump the fees of HTLC success and timeout transactions.

codecov-commenter · 2022-08-31T23:24:33Z

Codecov Report

Base: 90.79% // Head: 90.49% // Decreases project coverage by -0.29%⚠️

Coverage data is based on head (5478ef6) compared to base (6738fd5).
Patch coverage: 58.70% of modified lines in pull request are covered.

❗ Current head 5478ef6 differs from pull request most recent head e159214. Consider uploading reports for the commit e159214 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1689 +/- ##
==========================================
- Coverage 90.79% 90.49% -0.30% 
==========================================
Files 87 87 Lines 46969 46863 -106 Branches 46969 46863 -106 ==========================================
- Hits 42646 42410 -236 - Misses 4323 4453 +130

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`90.89% <0.00%> (-1.65%)`	⬇️
lightning/src/util/events.rs	`36.95% <0.00%> (-1.19%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.99% <14.28%> (-0.56%)`	⬇️
lightning/src/chain/channelmonitor.rs	`89.77% <52.08%> (-1.44%)`	⬇️
lightning/src/chain/onchaintx.rs	`88.16% <54.16%> (-7.47%)`	⬇️
lightning/src/chain/package.rs	`91.59% <80.80%> (-1.46%)`	⬇️
lightning/src/util/wakers.rs	`86.70% <0.00%> (-4.86%)`	⬇️
lightning/src/chain/mod.rs	`63.63% <0.00%> (-4.55%)`	⬇️
lightning/src/routing/gossip.rs	`91.44% <0.00%> (-0.72%)`	⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Didn't really dig into anything, just looking at the high-level.

wpaulino · 2022-09-14T23:24:05Z

Rebased on latest main and addressed pending comments.

ariard

I'm Approach ACK on both the BumpTransaction enum, the data scope returned back to the user and the event itself. To be looked more, the callsites where we're triggering the event, to see if there is way to simplify or clean paths.

arik-so · 2022-09-18T22:02:38Z

+ log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid());
+ Some(bumped_tx)
+	}
+	pub(crate) fn finalize_untractable_package<L: Deref, Signer: Sign>(


I think "intractable" is the more common form

"untractable" is already used quite a bit across the codebase, I'd prefer having a separate PR to only do the rename.

TheBlueMatt

Some quick comments, will take a further look later.

TheBlueMatt · 2022-09-21T11:28:04Z

+ let package_feerate = Self::compute_feerate(previous_package_feerate, fee_estimator, conf_target);
+ // With the new package feerate obtained, compute the feerate the child transaction must
+ // meet such that the package feerate is met.
+ (package_feerate as u64).checked_mul(2).map(|f| f.checked_sub(parent_feerate)).flatten()


Hmmmm, how do we handle the 1-sat/vb initial fee requirement? If our target ends up being, eg, 1sat/vb can we end up returning something lower here? Also, can we just subtract here? Don't we have to weigh it by the weight of the two transactions?

The issue here is that we don't know the weight of the child transaction yet as it will be created by the event consumer.
After dwelling on #1689 (comment), it may be better to expose a ConfirmationTarget instead of a feerate to indicate the confirmation urgency. The yet to be developed middleware can then handle mapping it to an appropriate feerate as it will have all the context required to do so properly.

Hmm, yea....somehow the consumer has to be able to track the specific transaction, though. We generally try to avoid assuming the fee estimator is correct, or that it responds quickly if the mempool is actively filling up (because some explicitly don't). Thus, we really need to make sure it's easy to track what feerate was used on the previous transaction not just what the target is.
I guess we could just expose the target feerate and let the user do the averaging?

Yeah that should work. We can then expose a helper method that does the averaging for the user.

dunxen · 2022-09-23T14:50:13Z

+#[derive(PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum ClaimEvent {
+	BumpCommitment {
+ target_feerate_sat_per_1000_weight: u32,


nit: Just grepping around and see we have this identifier but with plural sats.
Suggested change
target_feerate_sat_per_1000_weight:u32,
target_feerate_sats_per_1000_weight:u32,

Looks like we just use both interchangeably across the codebase. Would prefer deferring a consistent rename change to a distinct PR.

ariard

Still have to review some of the changes in package.rs, looks good overall.

Thanks for dealing with the rather murky package API, my apologies for the lack of documentation there.

TheBlueMatt

Basically LGTM.

ariard

I think we're good though I'm blocking a bit as using #[cfg(test)] TODO: remove-on-anchors-release to feature gate anchor output support. I can't remember we have gradually release sophisticated feature in the past, so sounds a first time. While I think it's worthy to do so in the present case I don't know if it's the best approach, see comment.

ariard · 2022-10-09T21:23:30Z

+ let commitment_package = PackageTemplate::build_package(
+ self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
+ PackageSolvingData::HolderFundingOutput(funding_output),
+ best_block_height, false, best_block_height,


For builders/helpers in safety-critical parts, we could start to document the boolean argument at the caller call-site, i.e "/* aggregable */ false". It sounds a code style sophistication, though we already do that on the Core-side. As the codebase keeps growing I think this could be a good practice as a argument misusage could introduce a logical bug. E.g, aggregating a PackageMalleability::Untractable and a PackageMalleability::Malleable together triggering a panic!(). Already seen that type of bug sticking for years in Core.

Will address this as a separate PR.

ariard · 2022-10-09T23:26:04Z

+ // attempt to broadcast the transaction with its current fee rate and hope
+ // it confirms. This is essentially the same behavior as a commitment
+ // transaction without anchor outputs.
+ None => Some((None, 0, OnchainClaim::Tx(tx.clone()))),


IIRC, the case where we don't find a second anchor output for the channel is when we have no fund at stakes in the channel (no-htlc, no-balance) output. We could consider the channel early closed and return a OnchainEvent::ChannelClosed, to allow consumer to react in consequence.

I like the idea, but seems out of scope for this PR.

ariard · 2022-10-09T23:50:48Z

@@ -2299,6 +2321,39 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 	pub fn get_and_clear_pending_events(&mut self) -> Vec<Event> {


I think we should update ChainMonitor::process_pending_events in consequence, noticing the user that this call is from now on important for the funds safety and call frequence should at least match every block ?

Done. I wonder if the ChainMonitor should just do this itself if any anchor channels are present. Would it be considered a violation of the events API?

Yeah we can do in a follow-up.

wpaulino · 2022-10-11T20:25:48Z

Had to rebase on main to address a conflict. The previous push includes changes addressing latest round of feedback.

ariard

ACK e159214

ariard

ACK c963d3a

TheBlueMatt

LGTM. There's a few followups but ideally let's just land this and we can do them later.

TheBlueMatt · 2022-10-17T15:01:48Z

+ Some((idx, _)) => {
+ // Our target feerate will depend on whether we have any HTLCs present
+ // within our commitment.
+ let conf_target = if self.holder_commitment.trust().htlcs().is_empty() {


We may have a unrevoked counterparty commitment that does contain HTLCs, I think, so we should check for that or just always use highpriority. Happy to see it as a followup so we can land this.

Since the counterparty commitment isn't exposed to the OnchainTxHandler, I reverted to always using HighPriority and added a todo.

TheBlueMatt · 2022-10-17T15:44:31Z

+ &Event::BumpTransaction(ref event)=> {
+ 27u8.write(writer)?;
+ match event {
+ // We never write the ChannelClose events as they'll be replayed upon restarting


I'm a little dubious of the never-write here. While we should generate a new bump eventually, we may take a few blocks to do so, and dropping the event may waste a few blocks' time, which seems bad?

I was thinking it would be fine as long as we always use a soonest_conf_deadline of our best known height (which is currently the case). get_height_timer will then force a retry at the next block.

Yea, we should reconsider that at some point, though, IIUC that will do an RBF bump at every block, which may mean bumping the fee by 25% every block, which seems like a lot? Not something for this PR, though.

TheBlueMatt · 2022-10-17T17:37:31Z

Oops, needs rebase now, sorry about that. Feel free to tackle some of the above when you do so.

As we integrate the support of anchor outputs, we'll want to know if each input we're working with came from an anchor outputs channel. Instead of threading through a `opt_anchors` boolean across several methods on `PackageSolvingData` and `PackageTemplate`, we decide to store a reference in each `PackageSolvingData` variant instead that features a change in behavior between channels with and without anchor outputs.

This will be useful later on when determining the appropriate fee rate to use on the anchor transaction to bump its commitment transaction.

wpaulino · 2022-08-31T22:36:00Z

This PR introduces a new BumpChannelClose event to be consumed by users indicating that they should craft a proper child anchor transaction with a sufficient feerate to increase the commitment transaction's (its parent) feerate and help get it confirmed.

Future work will also feature a similar event to bump the fees of HTLC success and timeout transactions.

codecov-commenter · 2022-08-31T23:24:33Z

Codecov Report

Base: 90.79% // Head: 90.49% // Decreases project coverage by -0.29%⚠️

Coverage data is based on head (5478ef6) compared to base (6738fd5).
Patch coverage: 58.70% of modified lines in pull request are covered.

❗ Current head 5478ef6 differs from pull request most recent head e159214. Consider uploading reports for the commit e159214 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1689 +/- ##
==========================================
- Coverage 90.79% 90.49% -0.30% 
==========================================
Files 87 87 Lines 46969 46863 -106 Branches 46969 46863 -106 ==========================================
- Hits 42646 42410 -236 - Misses 4323 4453 +130

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`90.89% <0.00%> (-1.65%)`	⬇️
lightning/src/util/events.rs	`36.95% <0.00%> (-1.19%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.99% <14.28%> (-0.56%)`	⬇️
lightning/src/chain/channelmonitor.rs	`89.77% <52.08%> (-1.44%)`	⬇️
lightning/src/chain/onchaintx.rs	`88.16% <54.16%> (-7.47%)`	⬇️
lightning/src/chain/package.rs	`91.59% <80.80%> (-1.46%)`	⬇️
lightning/src/util/wakers.rs	`86.70% <0.00%> (-4.86%)`	⬇️
lightning/src/chain/mod.rs	`63.63% <0.00%> (-4.55%)`	⬇️
lightning/src/routing/gossip.rs	`91.44% <0.00%> (-0.72%)`	⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Didn't really dig into anything, just looking at the high-level.

wpaulino · 2022-09-14T23:24:05Z

Rebased on latest main and addressed pending comments.

ariard

I'm Approach ACK on both the BumpTransaction enum, the data scope returned back to the user and the event itself. To be looked more, the callsites where we're triggering the event, to see if there is way to simplify or clean paths.

arik-so · 2022-09-18T22:02:38Z

+ log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid());
+ Some(bumped_tx)
+	}
+	pub(crate) fn finalize_untractable_package<L: Deref, Signer: Sign>(


I think "intractable" is the more common form

"untractable" is already used quite a bit across the codebase, I'd prefer having a separate PR to only do the rename.

TheBlueMatt

Some quick comments, will take a further look later.

TheBlueMatt · 2022-09-21T11:28:04Z

+ let package_feerate = Self::compute_feerate(previous_package_feerate, fee_estimator, conf_target);
+ // With the new package feerate obtained, compute the feerate the child transaction must
+ // meet such that the package feerate is met.
+ (package_feerate as u64).checked_mul(2).map(|f| f.checked_sub(parent_feerate)).flatten()


Hmmmm, how do we handle the 1-sat/vb initial fee requirement? If our target ends up being, eg, 1sat/vb can we end up returning something lower here? Also, can we just subtract here? Don't we have to weigh it by the weight of the two transactions?

The issue here is that we don't know the weight of the child transaction yet as it will be created by the event consumer.
After dwelling on #1689 (comment), it may be better to expose a ConfirmationTarget instead of a feerate to indicate the confirmation urgency. The yet to be developed middleware can then handle mapping it to an appropriate feerate as it will have all the context required to do so properly.

Hmm, yea....somehow the consumer has to be able to track the specific transaction, though. We generally try to avoid assuming the fee estimator is correct, or that it responds quickly if the mempool is actively filling up (because some explicitly don't). Thus, we really need to make sure it's easy to track what feerate was used on the previous transaction not just what the target is.
I guess we could just expose the target feerate and let the user do the averaging?

Yeah that should work. We can then expose a helper method that does the averaging for the user.

dunxen · 2022-09-23T14:50:13Z

+#[derive(PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum ClaimEvent {
+	BumpCommitment {
+ target_feerate_sat_per_1000_weight: u32,


nit: Just grepping around and see we have this identifier but with plural sats.
Suggested change
target_feerate_sat_per_1000_weight:u32,
target_feerate_sats_per_1000_weight:u32,

Looks like we just use both interchangeably across the codebase. Would prefer deferring a consistent rename change to a distinct PR.

ariard

Still have to review some of the changes in package.rs, looks good overall.

Thanks for dealing with the rather murky package API, my apologies for the lack of documentation there.

TheBlueMatt

Basically LGTM.

ariard

I think we're good though I'm blocking a bit as using #[cfg(test)] TODO: remove-on-anchors-release to feature gate anchor output support. I can't remember we have gradually release sophisticated feature in the past, so sounds a first time. While I think it's worthy to do so in the present case I don't know if it's the best approach, see comment.

ariard · 2022-10-09T21:23:30Z

+ let commitment_package = PackageTemplate::build_package(
+ self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
+ PackageSolvingData::HolderFundingOutput(funding_output),
+ best_block_height, false, best_block_height,


For builders/helpers in safety-critical parts, we could start to document the boolean argument at the caller call-site, i.e "/* aggregable */ false". It sounds a code style sophistication, though we already do that on the Core-side. As the codebase keeps growing I think this could be a good practice as a argument misusage could introduce a logical bug. E.g, aggregating a PackageMalleability::Untractable and a PackageMalleability::Malleable together triggering a panic!(). Already seen that type of bug sticking for years in Core.

Will address this as a separate PR.

ariard · 2022-10-09T23:26:04Z

+ // attempt to broadcast the transaction with its current fee rate and hope
+ // it confirms. This is essentially the same behavior as a commitment
+ // transaction without anchor outputs.
+ None => Some((None, 0, OnchainClaim::Tx(tx.clone()))),


IIRC, the case where we don't find a second anchor output for the channel is when we have no fund at stakes in the channel (no-htlc, no-balance) output. We could consider the channel early closed and return a OnchainEvent::ChannelClosed, to allow consumer to react in consequence.

I like the idea, but seems out of scope for this PR.

ariard · 2022-10-09T23:50:48Z

@@ -2299,6 +2321,39 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 	pub fn get_and_clear_pending_events(&mut self) -> Vec<Event> {


I think we should update ChainMonitor::process_pending_events in consequence, noticing the user that this call is from now on important for the funds safety and call frequence should at least match every block ?

Done. I wonder if the ChainMonitor should just do this itself if any anchor channels are present. Would it be considered a violation of the events API?

Yeah we can do in a follow-up.

wpaulino · 2022-10-11T20:25:48Z

Had to rebase on main to address a conflict. The previous push includes changes addressing latest round of feedback.

ariard

ACK e159214

ariard

ACK c963d3a

TheBlueMatt

LGTM. There's a few followups but ideally let's just land this and we can do them later.

TheBlueMatt · 2022-10-17T15:01:48Z

+ Some((idx, _)) => {
+ // Our target feerate will depend on whether we have any HTLCs present
+ // within our commitment.
+ let conf_target = if self.holder_commitment.trust().htlcs().is_empty() {


We may have a unrevoked counterparty commitment that does contain HTLCs, I think, so we should check for that or just always use highpriority. Happy to see it as a followup so we can land this.

Since the counterparty commitment isn't exposed to the OnchainTxHandler, I reverted to always using HighPriority and added a todo.

TheBlueMatt · 2022-10-17T15:44:31Z

+ &Event::BumpTransaction(ref event)=> {
+ 27u8.write(writer)?;
+ match event {
+ // We never write the ChannelClose events as they'll be replayed upon restarting


I'm a little dubious of the never-write here. While we should generate a new bump eventually, we may take a few blocks to do so, and dropping the event may waste a few blocks' time, which seems bad?

I was thinking it would be fine as long as we always use a soonest_conf_deadline of our best known height (which is currently the case). get_height_timer will then force a retry at the next block.

Yea, we should reconsider that at some point, though, IIUC that will do an RBF bump at every block, which may mean bumping the fee by 25% every block, which seems like a lot? Not something for this PR, though.

TheBlueMatt · 2022-10-17T17:37:31Z

Oops, needs rebase now, sorry about that. Feel free to tackle some of the above when you do so.

As we integrate the support of anchor outputs, we'll want to know if each input we're working with came from an anchor outputs channel. Instead of threading through a `opt_anchors` boolean across several methods on `PackageSolvingData` and `PackageTemplate`, we decide to store a reference in each `PackageSolvingData` variant instead that features a change in behavior between channels with and without anchor outputs.

This will be useful later on when determining the appropriate fee rate to use on the anchor transaction to bump its commitment transaction.

wpaulino · 2022-08-31T22:36:00Z

This PR introduces a new BumpChannelClose event to be consumed by users indicating that they should craft a proper child anchor transaction with a sufficient feerate to increase the commitment transaction's (its parent) feerate and help get it confirmed.

Future work will also feature a similar event to bump the fees of HTLC success and timeout transactions.

codecov-commenter · 2022-08-31T23:24:33Z

Codecov Report

Base: 90.79% // Head: 90.49% // Decreases project coverage by -0.29%⚠️

Coverage data is based on head (5478ef6) compared to base (6738fd5).
Patch coverage: 58.70% of modified lines in pull request are covered.

❗ Current head 5478ef6 differs from pull request most recent head e159214. Consider uploading reports for the commit e159214 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1689 +/- ##
==========================================
- Coverage 90.79% 90.49% -0.30% 
==========================================
Files 87 87 Lines 46969 46863 -106 Branches 46969 46863 -106 ==========================================
- Hits 42646 42410 -236 - Misses 4323 4453 +130

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	`90.89% <0.00%> (-1.65%)`	⬇️
lightning/src/util/events.rs	`36.95% <0.00%> (-1.19%)`	⬇️
lightning/src/ln/chan_utils.rs	`93.99% <14.28%> (-0.56%)`	⬇️
lightning/src/chain/channelmonitor.rs	`89.77% <52.08%> (-1.44%)`	⬇️
lightning/src/chain/onchaintx.rs	`88.16% <54.16%> (-7.47%)`	⬇️
lightning/src/chain/package.rs	`91.59% <80.80%> (-1.46%)`	⬇️
lightning/src/util/wakers.rs	`86.70% <0.00%> (-4.86%)`	⬇️
lightning/src/chain/mod.rs	`63.63% <0.00%> (-4.55%)`	⬇️
lightning/src/routing/gossip.rs	`91.44% <0.00%> (-0.72%)`	⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt

Didn't really dig into anything, just looking at the high-level.

wpaulino · 2022-09-14T23:24:05Z

Rebased on latest main and addressed pending comments.

ariard

I'm Approach ACK on both the BumpTransaction enum, the data scope returned back to the user and the event itself. To be looked more, the callsites where we're triggering the event, to see if there is way to simplify or clean paths.

arik-so · 2022-09-18T22:02:38Z

+ log_debug!(logger, "Finalized transaction {} ready to broadcast", bumped_tx.txid());
+ Some(bumped_tx)
+	}
+	pub(crate) fn finalize_untractable_package<L: Deref, Signer: Sign>(


I think "intractable" is the more common form

"untractable" is already used quite a bit across the codebase, I'd prefer having a separate PR to only do the rename.

TheBlueMatt

Some quick comments, will take a further look later.

TheBlueMatt · 2022-09-21T11:28:04Z

+ let package_feerate = Self::compute_feerate(previous_package_feerate, fee_estimator, conf_target);
+ // With the new package feerate obtained, compute the feerate the child transaction must
+ // meet such that the package feerate is met.
+ (package_feerate as u64).checked_mul(2).map(|f| f.checked_sub(parent_feerate)).flatten()


Hmmmm, how do we handle the 1-sat/vb initial fee requirement? If our target ends up being, eg, 1sat/vb can we end up returning something lower here? Also, can we just subtract here? Don't we have to weigh it by the weight of the two transactions?

The issue here is that we don't know the weight of the child transaction yet as it will be created by the event consumer.
After dwelling on #1689 (comment), it may be better to expose a ConfirmationTarget instead of a feerate to indicate the confirmation urgency. The yet to be developed middleware can then handle mapping it to an appropriate feerate as it will have all the context required to do so properly.

Hmm, yea....somehow the consumer has to be able to track the specific transaction, though. We generally try to avoid assuming the fee estimator is correct, or that it responds quickly if the mempool is actively filling up (because some explicitly don't). Thus, we really need to make sure it's easy to track what feerate was used on the previous transaction not just what the target is.
I guess we could just expose the target feerate and let the user do the averaging?

Yeah that should work. We can then expose a helper method that does the averaging for the user.

dunxen · 2022-09-23T14:50:13Z

+#[derive(PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum ClaimEvent {
+	BumpCommitment {
+ target_feerate_sat_per_1000_weight: u32,


nit: Just grepping around and see we have this identifier but with plural sats.
Suggested change
target_feerate_sat_per_1000_weight:u32,
target_feerate_sats_per_1000_weight:u32,

Looks like we just use both interchangeably across the codebase. Would prefer deferring a consistent rename change to a distinct PR.

ariard

Still have to review some of the changes in package.rs, looks good overall.

Thanks for dealing with the rather murky package API, my apologies for the lack of documentation there.

TheBlueMatt

Basically LGTM.

ariard

I think we're good though I'm blocking a bit as using #[cfg(test)] TODO: remove-on-anchors-release to feature gate anchor output support. I can't remember we have gradually release sophisticated feature in the past, so sounds a first time. While I think it's worthy to do so in the present case I don't know if it's the best approach, see comment.

ariard · 2022-10-09T21:23:30Z

+ let commitment_package = PackageTemplate::build_package(
+ self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
+ PackageSolvingData::HolderFundingOutput(funding_output),
+ best_block_height, false, best_block_height,


For builders/helpers in safety-critical parts, we could start to document the boolean argument at the caller call-site, i.e "/* aggregable */ false". It sounds a code style sophistication, though we already do that on the Core-side. As the codebase keeps growing I think this could be a good practice as a argument misusage could introduce a logical bug. E.g, aggregating a PackageMalleability::Untractable and a PackageMalleability::Malleable together triggering a panic!(). Already seen that type of bug sticking for years in Core.

Will address this as a separate PR.

ariard · 2022-10-09T23:26:04Z

+ // attempt to broadcast the transaction with its current fee rate and hope
+ // it confirms. This is essentially the same behavior as a commitment
+ // transaction without anchor outputs.
+ None => Some((None, 0, OnchainClaim::Tx(tx.clone()))),


IIRC, the case where we don't find a second anchor output for the channel is when we have no fund at stakes in the channel (no-htlc, no-balance) output. We could consider the channel early closed and return a OnchainEvent::ChannelClosed, to allow consumer to react in consequence.

I like the idea, but seems out of scope for this PR.

ariard · 2022-10-09T23:50:48Z

@@ -2299,6 +2321,39 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 	pub fn get_and_clear_pending_events(&mut self) -> Vec<Event> {


I think we should update ChainMonitor::process_pending_events in consequence, noticing the user that this call is from now on important for the funds safety and call frequence should at least match every block ?

Done. I wonder if the ChainMonitor should just do this itself if any anchor channels are present. Would it be considered a violation of the events API?

Yeah we can do in a follow-up.

wpaulino · 2022-10-11T20:25:48Z

Had to rebase on main to address a conflict. The previous push includes changes addressing latest round of feedback.

ariard

ACK e159214

ariard

ACK c963d3a

TheBlueMatt

LGTM. There's a few followups but ideally let's just land this and we can do them later.

TheBlueMatt · 2022-10-17T15:01:48Z

+ Some((idx, _)) => {
+ // Our target feerate will depend on whether we have any HTLCs present
+ // within our commitment.
+ let conf_target = if self.holder_commitment.trust().htlcs().is_empty() {


We may have a unrevoked counterparty commitment that does contain HTLCs, I think, so we should check for that or just always use highpriority. Happy to see it as a followup so we can land this.

Since the counterparty commitment isn't exposed to the OnchainTxHandler, I reverted to always using HighPriority and added a todo.

TheBlueMatt · 2022-10-17T15:44:31Z

+ &Event::BumpTransaction(ref event)=> {
+ 27u8.write(writer)?;
+ match event {
+ // We never write the ChannelClose events as they'll be replayed upon restarting


I'm a little dubious of the never-write here. While we should generate a new bump eventually, we may take a few blocks to do so, and dropping the event may waste a few blocks' time, which seems bad?

I was thinking it would be fine as long as we always use a soonest_conf_deadline of our best known height (which is currently the case). get_height_timer will then force a retry at the next block.

Yea, we should reconsider that at some point, though, IIUC that will do an RBF bump at every block, which may mean bumping the fee by 25% every block, which seems like a lot? Not something for this PR, though.

TheBlueMatt · 2022-10-17T17:37:31Z

Oops, needs rebase now, sorry about that. Feel free to tackle some of the above when you do so.

As we integrate the support of anchor outputs, we'll want to know if each input we're working with came from an anchor outputs channel. Instead of threading through a `opt_anchors` boolean across several methods on `PackageSolvingData` and `PackageTemplate`, we decide to store a reference in each `PackageSolvingData` variant instead that features a change in behavior between channels with and without anchor outputs.

This will be useful later on when determining the appropriate fee rate to use on the anchor transaction to bump its commitment transaction.

wpaulino added the blocked on dependent pr label Aug 31, 2022

wpaulino force-pushed the anchors-bump-channel-close-event branch from ab79496 to 0e079cdCompare August 31, 2022 22:58

ariard reviewed Sep 7, 2022
View reviewed changes

Comment threadlightning/src/util/events.rs Outdated
Comment threadlightning/src/util/events.rs Outdated
Comment threadlightning/src/util/events.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
Comment threadlightning/src/chain/onchaintx.rs

TheBlueMatt reviewed Sep 13, 2022
View reviewed changes

Comment threadlightning/src/chain/package.rs Outdated
Comment threadlightning/src/chain/package.rs Outdated
Comment threadlightning/src/util/events.rs Outdated
Comment threadlightning/src/util/events.rs Outdated

wpaulino force-pushed the anchors-bump-channel-close-event branch from 0e079cd to 937017eCompare September 14, 2022 23:23

TheBlueMatt removed the blocked on dependent pr label Sep 15, 2022

dunxen removed the Review club label Sep 16, 2022

ariard reviewed Sep 17, 2022
View reviewed changes

Comment threadlightning/src/util/events.rs Outdated
Comment threadlightning/src/chain/package.rs Outdated
Comment threadlightning/src/chain/onchaintx.rs
Comment threadlightning/src/chain/channelmonitor.rs
Comment threadlightning/src/util/events.rs Outdated

arik-so reviewed Sep 18, 2022
View reviewed changes

wpaulino force-pushed the anchors-bump-channel-close-event branch from 937017e to 21ad8f4Compare September 19, 2022 23:24

TheBlueMatt reviewed Sep 21, 2022
View reviewed changes

wpaulino force-pushed the anchors-bump-channel-close-event branch 2 times, most recently from 4aff3e2 to 974c96fCompare September 23, 2022 21:51

dunxen reviewed Sep 27, 2022
View reviewed changes

ariard reviewed Sep 28, 2022
View reviewed changes

Comment threadlightning/src/chain/onchaintx.rs
Comment threadlightning/src/chain/onchaintx.rs
Comment threadlightning/src/chain/package.rs
Comment threadlightning/src/chain/onchaintx.rs
Comment threadlightning/src/chain/onchaintx.rs

wpaulino force-pushed the anchors-bump-channel-close-event branch from 974c96f to 5748ec9Compare September 29, 2022 19:27

TheBlueMatt reviewed Oct 4, 2022
View reviewed changes

wpaulino force-pushed the anchors-bump-channel-close-event branch 3 times, most recently from 0a0216b to 5478ef6Compare October 6, 2022 19:38

TheBlueMatt assigned arik-so Oct 7, 2022

ariard reviewed Oct 9, 2022
View reviewed changes

arik-so reviewed Oct 10, 2022
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated

arik-so reviewed Oct 11, 2022
View reviewed changes

Comment threadlightning/src/chain/channelmonitor.rs Outdated

arik-so reviewed Oct 11, 2022
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated

arik-so reviewed Oct 11, 2022
View reviewed changes

Comment threadlightning/src/chain/onchaintx.rs Outdated

wpaulino force-pushed the anchors-bump-channel-close-event branch 2 times, most recently from 004aedf to 46f35c0Compare October 11, 2022 20:22

wpaulino force-pushed the anchors-bump-channel-close-event branch from 46f35c0 to e159214Compare October 11, 2022 20:25

arik-so previously approved these changes Oct 11, 2022
View reviewed changes

ariard reviewed Oct 12, 2022
View reviewed changes

Comment threadlightning/src/chain/onchaintx.rs

TheBlueMatt reviewed Oct 12, 2022
View reviewed changes

Comment threadlightning/Cargo.toml Outdated

wpaulino dismissed arik-so’s stale review via c963d3aOctober 13, 2022 15:44

wpaulino force-pushed the anchors-bump-channel-close-event branch from e159214 to c963d3aCompare October 13, 2022 15:44

ariard reviewed Oct 14, 2022
View reviewed changes

TheBlueMatt previously approved these changes Oct 17, 2022
View reviewed changes

arik-so previously approved these changes Oct 18, 2022
View reviewed changes

wpaulino added 10 commits October 18, 2022 12:22

Track whether HolderFundingOutput originated from an anchor channel
492b240

Track funding amount in HolderFundingOutput
ccf318e
This will be useful later on when determining the appropriate fee rate to use on the anchor transaction to bump its commitment transaction.

Split finalize_package into separate methods per malleability
40b1d4f

Refactor generate_claim_tx to return OnchainClaim enum
bac1b79

Generate ClaimEvent for HolderFundingOutput inputs from anchor channels
e9ac2b1

Yield channel close bump events
892f8fe

Document expected call frequency of ChainMonitor::process_pending_events
843b826

Add holder anchor signing support to BaseSign
2fa45ae

Add public helper to build anchor input witnesses
abe85a1

wpaulino dismissed stale reviews from arik-so and TheBlueMatt via abe85a1October 18, 2022 19:27

wpaulino force-pushed the anchors-bump-channel-close-event branch from c963d3a to abe85a1Compare October 18, 2022 19:27

TheBlueMatt approved these changes Oct 18, 2022
View reviewed changes

arik-so approved these changes Oct 18, 2022
View reviewed changes

TheBlueMatt merged commit 95bb27a into lightningdevkit:mainOct 19, 2022

wpaulino deleted the anchors-bump-channel-close-event branch October 19, 2022 00:33

wpaulino mentioned this pull request Nov 2, 2022
Introduce new BumpTransactionEvent variant HTLCResolution #1825
Merged