Expose new BumpChannelClose event for channels with anchor outputs #1689

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.

lightning/src/util/events.rs

lightning/src/chain/channelmonitor.rs

lightning/src/chain/onchaintx.rs

TheBlueMatt

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

lightning/src/chain/package.rs

lightning/src/util/events.rs

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.

lightning/src/util/events.rs

lightning/src/chain/package.rs

lightning/src/chain/onchaintx.rs

lightning/src/chain/channelmonitor.rs

lightning/src/util/events.rs

lightning/src/chain/channelmonitor.rs

lightning/src/chain/onchaintx.rs

lightning/src/chain/package.rs

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

lightning/src/chain/package.rs

+		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.

lightning/src/chain/package.rs

TheBlueMatt

Some quick comments, will take a further look later.

lightning/src/ln/chan_utils.rs

lightning/src/chain/onchaintx.rs

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

lightning/src/chain/package.rs

+		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

lightning/src/chain/onchaintx.rs

+#[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.

lightning/src/util/events.rs

lightning/src/chain/package.rs

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.

lightning/src/chain/onchaintx.rs

lightning/src/chain/package.rs

lightning/src/chain/onchaintx.rs

TheBlueMatt

Basically LGTM.

lightning/src/util/events.rs

lightning/src/chain/onchaintx.rs

lightning/src/util/events.rs

lightning/src/chain/keysinterface.rs

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

lightning/src/chain/channelmonitor.rs

+							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.

lightning/src/chain/keysinterface.rs

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

lightning/src/chain/onchaintx.rs

+						// 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.

lightning/src/chain/package.rs

lightning/src/chain/channelmonitor.rs

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

lightning/src/chain/channelmonitor.rs

@@ -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.

lightning/src/chain/channelmonitor.rs

lightning/src/chain/keysinterface.rs

lightning/src/chain/onchaintx.rs

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

lightning/src/chain/onchaintx.rs

lightning/Cargo.toml

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

lightning/src/chain/onchaintx.rs

+						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.

lightning/src/chain/package.rs

lightning/src/util/events.rs

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

lightning/src/util/events.rs

+			&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 0e079cd Compare August 31, 2022 22:58

ariard reviewed Sep 7, 2022

View reviewed changes

ariard added the Review club label Sep 7, 2022

TheBlueMatt reviewed Sep 13, 2022

View reviewed changes

lightning/src/chain/package.rs Outdated Show resolved Hide resolved

lightning/src/chain/package.rs Outdated Show resolved Hide resolved

lightning/src/util/events.rs Outdated Show resolved Hide resolved

lightning/src/util/events.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the anchors-bump-channel-close-event branch from 0e079cd to 937017e Compare 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

arik-so reviewed Sep 18, 2022

View reviewed changes

wpaulino force-pushed the anchors-bump-channel-close-event branch from 937017e to 21ad8f4 Compare 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 974c96f Compare September 23, 2022 21:51

dunxen reviewed Sep 27, 2022

View reviewed changes

ariard reviewed Sep 28, 2022

View reviewed changes

wpaulino force-pushed the anchors-bump-channel-close-event branch from 974c96f to 5748ec9 Compare 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 5478ef6 Compare October 6, 2022 19:38

TheBlueMatt assigned arik-so and ariard Oct 7, 2022

ariard reviewed Oct 9, 2022

View reviewed changes

arik-so reviewed Oct 10, 2022

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

arik-so reviewed Oct 11, 2022

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

arik-so reviewed Oct 11, 2022

View reviewed changes

lightning/src/chain/keysinterface.rs Outdated Show resolved Hide resolved

arik-so reviewed Oct 11, 2022

View reviewed changes

lightning/src/chain/onchaintx.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the anchors-bump-channel-close-event branch 2 times, most recently from 46f35c0 to e159214 Compare 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

lightning/src/chain/onchaintx.rs Show resolved Hide resolved

TheBlueMatt reviewed Oct 12, 2022

View reviewed changes

lightning/Cargo.toml Outdated Show resolved Hide resolved

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

wpaulino force-pushed the anchors-bump-channel-close-event branch from e159214 to c963d3a Compare 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 abe85a1 October 18, 2022 19:27

wpaulino force-pushed the anchors-bump-channel-close-event branch from c963d3a to abe85a1 Compare 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:main Oct 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

	target_feerate_sat_per_1000_weight: u32,
	target_feerate_sats_per_1000_weight: u32,

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

Expose new BumpChannelClose event for channels with anchor outputs #1689

Expose new BumpChannelClose event for channels with anchor outputs #1689

Uh oh!

Conversation

wpaulino commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Sep 14, 2022

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wpaulino commented Aug 31, 2022 •

edited

Loading

codecov-commenter commented Aug 31, 2022 •

edited

Loading