Introduce new BumpTransactionEvent variant HTLCResolution

[wpaulino]

This PR continues the work laid out in #1689 and introduces a new BumpTransactionEvent variant: HTLCResolution. Similarly, this event is to be consumed by users and indicates that a channel's commitment transaction with unresolved HTLCs has confirmed onchain, requiring HTLC transactions to be broadcast with additional inputs and/or outputs attached to satisfy feerate demands at the time of broadcast.

[wpaulino]

Rebased on latest to address an import conflict.

[codecov-commenter]

Codecov Report

Base: 90.57% // Head: 90.50% // Decreases project coverage by -0.07%⚠️

Coverage data is based on head (6a99960) compared to base (36e6023).
Patch coverage: 86.98% of modified lines in pull request are covered.

❗ Current head 6a99960 differs from pull request most recent head ec1f334. Consider uploading reports for the commit ec1f334 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1825 +/- ##
==========================================
- Coverage 90.57% 90.50% -0.08% 
==========================================
Files 91 91 Lines 48556 48731 +175 Branches 48556 48731 +175 ==========================================
+ Hits 43982 44102 +120 - Misses 4574 4629 +55 

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	83.14% <ø> (ø)
lightning/src/util/enforcing_trait_impls.rs	82.92% <ø> (ø)
lightning/src/util/events.rs	25.45% <ø> (ø)
lightning/src/chain/onchaintx.rs	92.73% <66.66%> (-2.62%)	⬇️
lightning/src/chain/package.rs	91.76% <68.57%> (-1.09%)	⬇️
lightning/src/ln/chan_utils.rs	93.61% <94.59%> (-0.02%)	⬇️
lightning/src/chain/channelmonitor.rs	91.04% <98.63%> (+0.11%)	⬆️
lightning/src/ln/onion_utils.rs	93.56% <0.00%> (-1.37%)	⬇️
lightning/src/ln/functional_tests.rs	96.65% <0.00%> (-0.35%)	⬇️
... and 5 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.

[wpaulino]

Pushed a new update addressing some remaining comments and a missing change noted by @ariard in #1860.

[TheBlueMatt]

Feel free to squash.

[wpaulino]

Squashed and rebased on latest to resolve a conflict.

[wpaulino]

> git diff -U1 a6eff8a f62e96f

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index b70121ba..cb31783d 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2651,13 +2651,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) where L::Target: Logger {
- macro_rules! ignore_error {
- ( $thing : expr ) => {
- match $thing {
- Ok(a) => a,
- Err(_) => return (Vec::new(), None)
- }
- };
- }
-
let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); };
- let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
+ let per_commitment_key = match SecretKey::from_slice(&secret) {
+ Ok(key) => key,
+ Err(_) => return (Vec::new(), None)
+ };
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
@@ -2667,6 +2661,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
let mut outputs_to_watch = None;
+ // Previously, we would only claim HTLCs from revoked HTLC transactions if they had 1 input
+ // with a witness of 5 elements and 1 output. This wasn't enough for anchor outputs, as the
+ // counterparty can now aggregate multiple HTLCs into a single transaction thanks to
+ // `SIGHASH_SINGLE` remote signatures, leading us to not claim any HTLCs upon seeing a
+ // confirmed revoked HTLC transaction (for more details, see
+ // https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-April/003561.html).
+ //
+ // We make sure we're not vulnerable to this case by checking all inputs of the transaction,
+ // and claim those which spend the commitment transaction, have a witness of 5 elements, and
+ // have a corresponding output at the same index within the transaction.
for (idx, input) in tx.input.iter().enumerate() {
- // HTLC transactions always spend an output on the commitment transaction with a witness
- // of 5 elements. The HTLC input will always have a corresponding output at the same
- // index within the transaction.
if input.previous_output.txid == *commitment_txid && input.witness.len() == 5 && tx.output.get(idx).is_some() {

[wpaulino]

@TheBlueMatt I addressed your remaining comments. Will open a new PR to address the serialization changes discussed in #1825 (comment).

[TheBlueMatt]

I left a handful of comments, ranging from mildly annoyed at what we have to trivial nits, but only one (the question about the 0th output) actually needs a response for this to land, I think.

[TheBlueMatt]

For both this and sign_holder_anchor_input I'm really not a fan of passing in a full tx-to-sign. I'm not sure what to do about it, though, we don't really want to constrain the tx contents really. Luckily segwit commits to the amount, but I could see someone forgetting to check the fee, for example. I guess we're really relying on the other input being signed as "the" security control here?

[wpaulino]

I'm not following -- we need to pass in the full transaction since it includes the additional fee inputs/outputs and our local signature is SIGHASH_ALL.

[TheBlueMatt]

I guess I was thinking more that we could pass the set of non-HTLC inputs, the output script and fee and then have the siger recreate the transaction. This avoids having to calculate the fee, check the locktime, etc. In general the principle for the signing interfaces should be (but isn't always) that we pass the user the things they need to construct the object to sign (and a utility method to do so) and make them do the construction. That way everything they need to validate is shoved in their face and they can't avoid looking at it (and hopefully remember to check it). Given our other HTLC-sign transactions already have this mistake it's fine for now, but in the future we should refactor all those methods to do the HTLC tx construction on the signer's end.

[wpaulino]

Updated docs and rebased on top of #1887 since it gets rid of some Results on HTLCDescriptor methods.

[ariard]

See last comment.

[ariard]

Here I think we can debug_assert!(seff.onchain_tx_handler.opt_anchors()).

[wpaulino]

We can probably replace most #[cfg(anchors)] with this assertion so will leave it for a follow-up.

[ariard]

Note if you would like to add a commit to recall this enum field FundingUnilateralResolution. A ChannelClose in Lightning parlance is really ambiguous as it designates also the cooperative case. This cooperative case itself could be a target for fee-bumping in the future (e.g splicing support). A small bikeshedding as we're around.

[TheBlueMatt]

If we want to rename it, let's do it in a followup. This PR has been delayed enough and isn't even touching this code.

[ariard]

There is no current indication on the level of external fee-bumping reserves a user is supposed to keep. For HTLC resolution, we would have to encompass the worst-case of holder's max_accepted_htlcs + counterparty's max_accepted_htlcs * 706 WU (for post-anchor HTLC-Success) * worst-historical mempool feerate. And this is only per-channel. Anyway, I don't know where we would like to start document those requirements here, in #1860 or even after ?

[TheBlueMatt]

Let's do it later.

[TheBlueMatt]

Needs rebase, think we can land this today or tomorrow?

[TheBlueMatt]

Looks like CI is sad? Feel free to squash the fixups.

[wpaulino]

There was a stray else causing the failures. Test suite passes locally now so we should be good.

[ariard]

On my side, I'll try to review back tomorrow. Checking the full transaction processing flow from ChainMonitor.

[arik-so]

I really like the comments explaining the potential vulnerability that might arise with anchor HTLC transactions.

[ariard]

See second-last comment.

[ariard]

I think this is assumption is broken, and per se our code will still miss the detection and claim of revoked second-stage HTLC transactions.

With SIGHASH_SINGLE | SIGHASH_ANYONECANPAY, the second-stage HTLC transaction claims can be aggregated across commitment transactions, as there is no commitment to the spend txid in the shared transaction fields (e.g the parent txid could be in the nLocktime field). Our logic iterate on all the inputs, and at the first one with a confirmed commitment txid in counterparty_commitment_txn_on_chain, we'll call in check_spend_counterparty_htlc() L3028. There, we iterate against all inputs and if the input outpoint txid is different from the passed commitment_txid, we skip it further processing. Once we're out of check_spend_counterparty_htlc(), we end up here and break from the for control flow, am I correct ? At the very least I don't think we have a test coverage for this case.

[TheBlueMatt]

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

[wpaulino]

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

[ariard]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

Note, here we're checking the second-stage HTLC transactions, and we're filtering out the input/output pair to be claimed based on the spent commitment transaction id. Post-anchor output, second-stage HTLC transactions are signed with SIGHASH_SINGLE enabling aggregation of HTLC claims from the same commitment transaction, but there is no constraint restraining the aggregation of HTLC claims across commitment transactions. Even if for one ChannelMonitor there is a single commitment_txid that can confirms.

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

This is a correct, the claim of HTLCs is a per-ChannelMonitor responsibility, and I don't think there is a way open by malleability where the aggregated HTLC claims crafted by our counterparty could blind our parsing logic. Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

[wpaulino]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

Since the ChannelMonitor is already on a per-channel basis, the comment really means "all from the same commitment for this channel". I agree it could definitely be re-worded better though.

Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

Doesn't seem like there's much we can do other than document it? Even then, I would imagine all LDK users want to use our ChainMonitor as is. Rather than re-implementing it for their own needs, they just need to implement the trait dependencies on ChainMonitor.

[ariard]

Doesn't seem like there's much we can do other than document it?

More documentation of our assumptions is always better. I think with the extended flexibility offered by our interfaces we have far less visibility on how it could be re-implemented by users rather than a monolithic Lightning node directly consuming from standard Core interfaces. This is raising the bar for funds safety, as implicit assumptions of our low-level code could be silently broken. Beyond, we might have in the future to re-implement a ChainMonitor for "monitor-replica" support, a bit more sophisticated than the standard one.

[wpaulino]

This PR continues the work laid out in #1689 and introduces a new BumpTransactionEvent variant: HTLCResolution. Similarly, this event is to be consumed by users and indicates that a channel's commitment transaction with unresolved HTLCs has confirmed onchain, requiring HTLC transactions to be broadcast with additional inputs and/or outputs attached to satisfy feerate demands at the time of broadcast.

[wpaulino]

Rebased on latest to address an import conflict.

[codecov-commenter]

Codecov Report

Base: 90.57% // Head: 90.50% // Decreases project coverage by -0.07%⚠️

Coverage data is based on head (6a99960) compared to base (36e6023).
Patch coverage: 86.98% of modified lines in pull request are covered.

❗ Current head 6a99960 differs from pull request most recent head ec1f334. Consider uploading reports for the commit ec1f334 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1825 +/- ##
==========================================
- Coverage 90.57% 90.50% -0.08% 
==========================================
Files 91 91 Lines 48556 48731 +175 Branches 48556 48731 +175 ==========================================
+ Hits 43982 44102 +120 - Misses 4574 4629 +55 

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	83.14% <ø> (ø)
lightning/src/util/enforcing_trait_impls.rs	82.92% <ø> (ø)
lightning/src/util/events.rs	25.45% <ø> (ø)
lightning/src/chain/onchaintx.rs	92.73% <66.66%> (-2.62%)	⬇️
lightning/src/chain/package.rs	91.76% <68.57%> (-1.09%)	⬇️
lightning/src/ln/chan_utils.rs	93.61% <94.59%> (-0.02%)	⬇️
lightning/src/chain/channelmonitor.rs	91.04% <98.63%> (+0.11%)	⬆️
lightning/src/ln/onion_utils.rs	93.56% <0.00%> (-1.37%)	⬇️
lightning/src/ln/functional_tests.rs	96.65% <0.00%> (-0.35%)	⬇️
... and 5 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.

[wpaulino]

Pushed a new update addressing some remaining comments and a missing change noted by @ariard in #1860.

[TheBlueMatt]

Feel free to squash.

[wpaulino]

Squashed and rebased on latest to resolve a conflict.

[wpaulino]

> git diff -U1 a6eff8a f62e96f

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index b70121ba..cb31783d 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2651,13 +2651,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) where L::Target: Logger {
- macro_rules! ignore_error {
- ( $thing : expr ) => {
- match $thing {
- Ok(a) => a,
- Err(_) => return (Vec::new(), None)
- }
- };
- }
-
let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); };
- let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
+ let per_commitment_key = match SecretKey::from_slice(&secret) {
+ Ok(key) => key,
+ Err(_) => return (Vec::new(), None)
+ };
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
@@ -2667,6 +2661,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
let mut outputs_to_watch = None;
+ // Previously, we would only claim HTLCs from revoked HTLC transactions if they had 1 input
+ // with a witness of 5 elements and 1 output. This wasn't enough for anchor outputs, as the
+ // counterparty can now aggregate multiple HTLCs into a single transaction thanks to
+ // `SIGHASH_SINGLE` remote signatures, leading us to not claim any HTLCs upon seeing a
+ // confirmed revoked HTLC transaction (for more details, see
+ // https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-April/003561.html).
+ //
+ // We make sure we're not vulnerable to this case by checking all inputs of the transaction,
+ // and claim those which spend the commitment transaction, have a witness of 5 elements, and
+ // have a corresponding output at the same index within the transaction.
for (idx, input) in tx.input.iter().enumerate() {
- // HTLC transactions always spend an output on the commitment transaction with a witness
- // of 5 elements. The HTLC input will always have a corresponding output at the same
- // index within the transaction.
if input.previous_output.txid == *commitment_txid && input.witness.len() == 5 && tx.output.get(idx).is_some() {

[wpaulino]

@TheBlueMatt I addressed your remaining comments. Will open a new PR to address the serialization changes discussed in #1825 (comment).

[TheBlueMatt]

I left a handful of comments, ranging from mildly annoyed at what we have to trivial nits, but only one (the question about the 0th output) actually needs a response for this to land, I think.

[TheBlueMatt]

For both this and sign_holder_anchor_input I'm really not a fan of passing in a full tx-to-sign. I'm not sure what to do about it, though, we don't really want to constrain the tx contents really. Luckily segwit commits to the amount, but I could see someone forgetting to check the fee, for example. I guess we're really relying on the other input being signed as "the" security control here?

[wpaulino]

I'm not following -- we need to pass in the full transaction since it includes the additional fee inputs/outputs and our local signature is SIGHASH_ALL.

[TheBlueMatt]

I guess I was thinking more that we could pass the set of non-HTLC inputs, the output script and fee and then have the siger recreate the transaction. This avoids having to calculate the fee, check the locktime, etc. In general the principle for the signing interfaces should be (but isn't always) that we pass the user the things they need to construct the object to sign (and a utility method to do so) and make them do the construction. That way everything they need to validate is shoved in their face and they can't avoid looking at it (and hopefully remember to check it). Given our other HTLC-sign transactions already have this mistake it's fine for now, but in the future we should refactor all those methods to do the HTLC tx construction on the signer's end.

[wpaulino]

Updated docs and rebased on top of #1887 since it gets rid of some Results on HTLCDescriptor methods.

[ariard]

See last comment.

[ariard]

Here I think we can debug_assert!(seff.onchain_tx_handler.opt_anchors()).

[wpaulino]

We can probably replace most #[cfg(anchors)] with this assertion so will leave it for a follow-up.

[ariard]

Note if you would like to add a commit to recall this enum field FundingUnilateralResolution. A ChannelClose in Lightning parlance is really ambiguous as it designates also the cooperative case. This cooperative case itself could be a target for fee-bumping in the future (e.g splicing support). A small bikeshedding as we're around.

[TheBlueMatt]

If we want to rename it, let's do it in a followup. This PR has been delayed enough and isn't even touching this code.

[ariard]

There is no current indication on the level of external fee-bumping reserves a user is supposed to keep. For HTLC resolution, we would have to encompass the worst-case of holder's max_accepted_htlcs + counterparty's max_accepted_htlcs * 706 WU (for post-anchor HTLC-Success) * worst-historical mempool feerate. And this is only per-channel. Anyway, I don't know where we would like to start document those requirements here, in #1860 or even after ?

[TheBlueMatt]

Let's do it later.

[TheBlueMatt]

Needs rebase, think we can land this today or tomorrow?

[TheBlueMatt]

Looks like CI is sad? Feel free to squash the fixups.

[wpaulino]

There was a stray else causing the failures. Test suite passes locally now so we should be good.

[ariard]

On my side, I'll try to review back tomorrow. Checking the full transaction processing flow from ChainMonitor.

[arik-so]

I really like the comments explaining the potential vulnerability that might arise with anchor HTLC transactions.

[ariard]

See second-last comment.

[ariard]

I think this is assumption is broken, and per se our code will still miss the detection and claim of revoked second-stage HTLC transactions.

With SIGHASH_SINGLE | SIGHASH_ANYONECANPAY, the second-stage HTLC transaction claims can be aggregated across commitment transactions, as there is no commitment to the spend txid in the shared transaction fields (e.g the parent txid could be in the nLocktime field). Our logic iterate on all the inputs, and at the first one with a confirmed commitment txid in counterparty_commitment_txn_on_chain, we'll call in check_spend_counterparty_htlc() L3028. There, we iterate against all inputs and if the input outpoint txid is different from the passed commitment_txid, we skip it further processing. Once we're out of check_spend_counterparty_htlc(), we end up here and break from the for control flow, am I correct ? At the very least I don't think we have a test coverage for this case.

[TheBlueMatt]

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

[wpaulino]

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

[ariard]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

Note, here we're checking the second-stage HTLC transactions, and we're filtering out the input/output pair to be claimed based on the spent commitment transaction id. Post-anchor output, second-stage HTLC transactions are signed with SIGHASH_SINGLE enabling aggregation of HTLC claims from the same commitment transaction, but there is no constraint restraining the aggregation of HTLC claims across commitment transactions. Even if for one ChannelMonitor there is a single commitment_txid that can confirms.

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

This is a correct, the claim of HTLCs is a per-ChannelMonitor responsibility, and I don't think there is a way open by malleability where the aggregated HTLC claims crafted by our counterparty could blind our parsing logic. Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

[wpaulino]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

Since the ChannelMonitor is already on a per-channel basis, the comment really means "all from the same commitment for this channel". I agree it could definitely be re-worded better though.

Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

Doesn't seem like there's much we can do other than document it? Even then, I would imagine all LDK users want to use our ChainMonitor as is. Rather than re-implementing it for their own needs, they just need to implement the trait dependencies on ChainMonitor.

[ariard]

Doesn't seem like there's much we can do other than document it?

More documentation of our assumptions is always better. I think with the extended flexibility offered by our interfaces we have far less visibility on how it could be re-implemented by users rather than a monolithic Lightning node directly consuming from standard Core interfaces. This is raising the bar for funds safety, as implicit assumptions of our low-level code could be silently broken. Beyond, we might have in the future to re-implement a ChainMonitor for "monitor-replica" support, a bit more sophisticated than the standard one.

[wpaulino]

This PR continues the work laid out in #1689 and introduces a new BumpTransactionEvent variant: HTLCResolution. Similarly, this event is to be consumed by users and indicates that a channel's commitment transaction with unresolved HTLCs has confirmed onchain, requiring HTLC transactions to be broadcast with additional inputs and/or outputs attached to satisfy feerate demands at the time of broadcast.

[wpaulino]

Rebased on latest to address an import conflict.

[codecov-commenter]

Codecov Report

Base: 90.57% // Head: 90.50% // Decreases project coverage by -0.07%⚠️

Coverage data is based on head (6a99960) compared to base (36e6023).
Patch coverage: 86.98% of modified lines in pull request are covered.

❗ Current head 6a99960 differs from pull request most recent head ec1f334. Consider uploading reports for the commit ec1f334 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1825 +/- ##
==========================================
- Coverage 90.57% 90.50% -0.08% 
==========================================
Files 91 91 Lines 48556 48731 +175 Branches 48556 48731 +175 ==========================================
+ Hits 43982 44102 +120 - Misses 4574 4629 +55 

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	83.14% <ø> (ø)
lightning/src/util/enforcing_trait_impls.rs	82.92% <ø> (ø)
lightning/src/util/events.rs	25.45% <ø> (ø)
lightning/src/chain/onchaintx.rs	92.73% <66.66%> (-2.62%)	⬇️
lightning/src/chain/package.rs	91.76% <68.57%> (-1.09%)	⬇️
lightning/src/ln/chan_utils.rs	93.61% <94.59%> (-0.02%)	⬇️
lightning/src/chain/channelmonitor.rs	91.04% <98.63%> (+0.11%)	⬆️
lightning/src/ln/onion_utils.rs	93.56% <0.00%> (-1.37%)	⬇️
lightning/src/ln/functional_tests.rs	96.65% <0.00%> (-0.35%)	⬇️
... and 5 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.

[wpaulino]

Pushed a new update addressing some remaining comments and a missing change noted by @ariard in #1860.

[TheBlueMatt]

Feel free to squash.

[wpaulino]

Squashed and rebased on latest to resolve a conflict.

[wpaulino]

> git diff -U1 a6eff8a f62e96f

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index b70121ba..cb31783d 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2651,13 +2651,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) where L::Target: Logger {
- macro_rules! ignore_error {
- ( $thing : expr ) => {
- match $thing {
- Ok(a) => a,
- Err(_) => return (Vec::new(), None)
- }
- };
- }
-
let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); };
- let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
+ let per_commitment_key = match SecretKey::from_slice(&secret) {
+ Ok(key) => key,
+ Err(_) => return (Vec::new(), None)
+ };
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
@@ -2667,6 +2661,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
let mut outputs_to_watch = None;
+ // Previously, we would only claim HTLCs from revoked HTLC transactions if they had 1 input
+ // with a witness of 5 elements and 1 output. This wasn't enough for anchor outputs, as the
+ // counterparty can now aggregate multiple HTLCs into a single transaction thanks to
+ // `SIGHASH_SINGLE` remote signatures, leading us to not claim any HTLCs upon seeing a
+ // confirmed revoked HTLC transaction (for more details, see
+ // https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-April/003561.html).
+ //
+ // We make sure we're not vulnerable to this case by checking all inputs of the transaction,
+ // and claim those which spend the commitment transaction, have a witness of 5 elements, and
+ // have a corresponding output at the same index within the transaction.
for (idx, input) in tx.input.iter().enumerate() {
- // HTLC transactions always spend an output on the commitment transaction with a witness
- // of 5 elements. The HTLC input will always have a corresponding output at the same
- // index within the transaction.
if input.previous_output.txid == *commitment_txid && input.witness.len() == 5 && tx.output.get(idx).is_some() {

[wpaulino]

@TheBlueMatt I addressed your remaining comments. Will open a new PR to address the serialization changes discussed in #1825 (comment).

[TheBlueMatt]

I left a handful of comments, ranging from mildly annoyed at what we have to trivial nits, but only one (the question about the 0th output) actually needs a response for this to land, I think.

[TheBlueMatt]

For both this and sign_holder_anchor_input I'm really not a fan of passing in a full tx-to-sign. I'm not sure what to do about it, though, we don't really want to constrain the tx contents really. Luckily segwit commits to the amount, but I could see someone forgetting to check the fee, for example. I guess we're really relying on the other input being signed as "the" security control here?

[wpaulino]

I'm not following -- we need to pass in the full transaction since it includes the additional fee inputs/outputs and our local signature is SIGHASH_ALL.

[TheBlueMatt]

I guess I was thinking more that we could pass the set of non-HTLC inputs, the output script and fee and then have the siger recreate the transaction. This avoids having to calculate the fee, check the locktime, etc. In general the principle for the signing interfaces should be (but isn't always) that we pass the user the things they need to construct the object to sign (and a utility method to do so) and make them do the construction. That way everything they need to validate is shoved in their face and they can't avoid looking at it (and hopefully remember to check it). Given our other HTLC-sign transactions already have this mistake it's fine for now, but in the future we should refactor all those methods to do the HTLC tx construction on the signer's end.

[wpaulino]

Updated docs and rebased on top of #1887 since it gets rid of some Results on HTLCDescriptor methods.

[ariard]

See last comment.

[ariard]

Here I think we can debug_assert!(seff.onchain_tx_handler.opt_anchors()).

[wpaulino]

We can probably replace most #[cfg(anchors)] with this assertion so will leave it for a follow-up.

[ariard]

Note if you would like to add a commit to recall this enum field FundingUnilateralResolution. A ChannelClose in Lightning parlance is really ambiguous as it designates also the cooperative case. This cooperative case itself could be a target for fee-bumping in the future (e.g splicing support). A small bikeshedding as we're around.

[TheBlueMatt]

If we want to rename it, let's do it in a followup. This PR has been delayed enough and isn't even touching this code.

[ariard]

There is no current indication on the level of external fee-bumping reserves a user is supposed to keep. For HTLC resolution, we would have to encompass the worst-case of holder's max_accepted_htlcs + counterparty's max_accepted_htlcs * 706 WU (for post-anchor HTLC-Success) * worst-historical mempool feerate. And this is only per-channel. Anyway, I don't know where we would like to start document those requirements here, in #1860 or even after ?

[TheBlueMatt]

Let's do it later.

[TheBlueMatt]

Needs rebase, think we can land this today or tomorrow?

[TheBlueMatt]

Looks like CI is sad? Feel free to squash the fixups.

[wpaulino]

There was a stray else causing the failures. Test suite passes locally now so we should be good.

[ariard]

On my side, I'll try to review back tomorrow. Checking the full transaction processing flow from ChainMonitor.

[arik-so]

I really like the comments explaining the potential vulnerability that might arise with anchor HTLC transactions.

[ariard]

See second-last comment.

[ariard]

I think this is assumption is broken, and per se our code will still miss the detection and claim of revoked second-stage HTLC transactions.

With SIGHASH_SINGLE | SIGHASH_ANYONECANPAY, the second-stage HTLC transaction claims can be aggregated across commitment transactions, as there is no commitment to the spend txid in the shared transaction fields (e.g the parent txid could be in the nLocktime field). Our logic iterate on all the inputs, and at the first one with a confirmed commitment txid in counterparty_commitment_txn_on_chain, we'll call in check_spend_counterparty_htlc() L3028. There, we iterate against all inputs and if the input outpoint txid is different from the passed commitment_txid, we skip it further processing. Once we're out of check_spend_counterparty_htlc(), we end up here and break from the for control flow, am I correct ? At the very least I don't think we have a test coverage for this case.

[TheBlueMatt]

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

[wpaulino]

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

[ariard]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

Note, here we're checking the second-stage HTLC transactions, and we're filtering out the input/output pair to be claimed based on the spent commitment transaction id. Post-anchor output, second-stage HTLC transactions are signed with SIGHASH_SINGLE enabling aggregation of HTLC claims from the same commitment transaction, but there is no constraint restraining the aggregation of HTLC claims across commitment transactions. Even if for one ChannelMonitor there is a single commitment_txid that can confirms.

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

This is a correct, the claim of HTLCs is a per-ChannelMonitor responsibility, and I don't think there is a way open by malleability where the aggregated HTLC claims crafted by our counterparty could blind our parsing logic. Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

[wpaulino]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

Since the ChannelMonitor is already on a per-channel basis, the comment really means "all from the same commitment for this channel". I agree it could definitely be re-worded better though.

Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

Doesn't seem like there's much we can do other than document it? Even then, I would imagine all LDK users want to use our ChainMonitor as is. Rather than re-implementing it for their own needs, they just need to implement the trait dependencies on ChainMonitor.

[ariard]

Doesn't seem like there's much we can do other than document it?

More documentation of our assumptions is always better. I think with the extended flexibility offered by our interfaces we have far less visibility on how it could be re-implemented by users rather than a monolithic Lightning node directly consuming from standard Core interfaces. This is raising the bar for funds safety, as implicit assumptions of our low-level code could be silently broken. Beyond, we might have in the future to re-implement a ChainMonitor for "monitor-replica" support, a bit more sophisticated than the standard one.

[wpaulino]

This PR continues the work laid out in #1689 and introduces a new BumpTransactionEvent variant: HTLCResolution. Similarly, this event is to be consumed by users and indicates that a channel's commitment transaction with unresolved HTLCs has confirmed onchain, requiring HTLC transactions to be broadcast with additional inputs and/or outputs attached to satisfy feerate demands at the time of broadcast.

[wpaulino]

Rebased on latest to address an import conflict.

[codecov-commenter]

Codecov Report

Base: 90.57% // Head: 90.50% // Decreases project coverage by -0.07%⚠️

Coverage data is based on head (6a99960) compared to base (36e6023).
Patch coverage: 86.98% of modified lines in pull request are covered.

❗ Current head 6a99960 differs from pull request most recent head ec1f334. Consider uploading reports for the commit ec1f334 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1825 +/- ##
==========================================
- Coverage 90.57% 90.50% -0.08% 
==========================================
Files 91 91 Lines 48556 48731 +175 Branches 48556 48731 +175 ==========================================
+ Hits 43982 44102 +120 - Misses 4574 4629 +55 

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	83.14% <ø> (ø)
lightning/src/util/enforcing_trait_impls.rs	82.92% <ø> (ø)
lightning/src/util/events.rs	25.45% <ø> (ø)
lightning/src/chain/onchaintx.rs	92.73% <66.66%> (-2.62%)	⬇️
lightning/src/chain/package.rs	91.76% <68.57%> (-1.09%)	⬇️
lightning/src/ln/chan_utils.rs	93.61% <94.59%> (-0.02%)	⬇️
lightning/src/chain/channelmonitor.rs	91.04% <98.63%> (+0.11%)	⬆️
lightning/src/ln/onion_utils.rs	93.56% <0.00%> (-1.37%)	⬇️
lightning/src/ln/functional_tests.rs	96.65% <0.00%> (-0.35%)	⬇️
... and 5 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.

[wpaulino]

Pushed a new update addressing some remaining comments and a missing change noted by @ariard in #1860.

[TheBlueMatt]

Feel free to squash.

[wpaulino]

Squashed and rebased on latest to resolve a conflict.

[wpaulino]

> git diff -U1 a6eff8a f62e96f

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index b70121ba..cb31783d 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2651,13 +2651,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) where L::Target: Logger {
- macro_rules! ignore_error {
- ( $thing : expr ) => {
- match $thing {
- Ok(a) => a,
- Err(_) => return (Vec::new(), None)
- }
- };
- }
-
let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); };
- let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
+ let per_commitment_key = match SecretKey::from_slice(&secret) {
+ Ok(key) => key,
+ Err(_) => return (Vec::new(), None)
+ };
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
@@ -2667,6 +2661,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
let mut outputs_to_watch = None;
+ // Previously, we would only claim HTLCs from revoked HTLC transactions if they had 1 input
+ // with a witness of 5 elements and 1 output. This wasn't enough for anchor outputs, as the
+ // counterparty can now aggregate multiple HTLCs into a single transaction thanks to
+ // `SIGHASH_SINGLE` remote signatures, leading us to not claim any HTLCs upon seeing a
+ // confirmed revoked HTLC transaction (for more details, see
+ // https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-April/003561.html).
+ //
+ // We make sure we're not vulnerable to this case by checking all inputs of the transaction,
+ // and claim those which spend the commitment transaction, have a witness of 5 elements, and
+ // have a corresponding output at the same index within the transaction.
for (idx, input) in tx.input.iter().enumerate() {
- // HTLC transactions always spend an output on the commitment transaction with a witness
- // of 5 elements. The HTLC input will always have a corresponding output at the same
- // index within the transaction.
if input.previous_output.txid == *commitment_txid && input.witness.len() == 5 && tx.output.get(idx).is_some() {

[wpaulino]

@TheBlueMatt I addressed your remaining comments. Will open a new PR to address the serialization changes discussed in #1825 (comment).

[TheBlueMatt]

I left a handful of comments, ranging from mildly annoyed at what we have to trivial nits, but only one (the question about the 0th output) actually needs a response for this to land, I think.

[TheBlueMatt]

For both this and sign_holder_anchor_input I'm really not a fan of passing in a full tx-to-sign. I'm not sure what to do about it, though, we don't really want to constrain the tx contents really. Luckily segwit commits to the amount, but I could see someone forgetting to check the fee, for example. I guess we're really relying on the other input being signed as "the" security control here?

[wpaulino]

I'm not following -- we need to pass in the full transaction since it includes the additional fee inputs/outputs and our local signature is SIGHASH_ALL.

[TheBlueMatt]

I guess I was thinking more that we could pass the set of non-HTLC inputs, the output script and fee and then have the siger recreate the transaction. This avoids having to calculate the fee, check the locktime, etc. In general the principle for the signing interfaces should be (but isn't always) that we pass the user the things they need to construct the object to sign (and a utility method to do so) and make them do the construction. That way everything they need to validate is shoved in their face and they can't avoid looking at it (and hopefully remember to check it). Given our other HTLC-sign transactions already have this mistake it's fine for now, but in the future we should refactor all those methods to do the HTLC tx construction on the signer's end.

[wpaulino]

Updated docs and rebased on top of #1887 since it gets rid of some Results on HTLCDescriptor methods.

[ariard]

See last comment.

[ariard]

Here I think we can debug_assert!(seff.onchain_tx_handler.opt_anchors()).

[wpaulino]

We can probably replace most #[cfg(anchors)] with this assertion so will leave it for a follow-up.

[ariard]

Note if you would like to add a commit to recall this enum field FundingUnilateralResolution. A ChannelClose in Lightning parlance is really ambiguous as it designates also the cooperative case. This cooperative case itself could be a target for fee-bumping in the future (e.g splicing support). A small bikeshedding as we're around.

[TheBlueMatt]

If we want to rename it, let's do it in a followup. This PR has been delayed enough and isn't even touching this code.

[ariard]

There is no current indication on the level of external fee-bumping reserves a user is supposed to keep. For HTLC resolution, we would have to encompass the worst-case of holder's max_accepted_htlcs + counterparty's max_accepted_htlcs * 706 WU (for post-anchor HTLC-Success) * worst-historical mempool feerate. And this is only per-channel. Anyway, I don't know where we would like to start document those requirements here, in #1860 or even after ?

[TheBlueMatt]

Let's do it later.

[TheBlueMatt]

Needs rebase, think we can land this today or tomorrow?

[TheBlueMatt]

Looks like CI is sad? Feel free to squash the fixups.

[wpaulino]

There was a stray else causing the failures. Test suite passes locally now so we should be good.

[ariard]

On my side, I'll try to review back tomorrow. Checking the full transaction processing flow from ChainMonitor.

[arik-so]

I really like the comments explaining the potential vulnerability that might arise with anchor HTLC transactions.

[ariard]

See second-last comment.

[ariard]

I think this is assumption is broken, and per se our code will still miss the detection and claim of revoked second-stage HTLC transactions.

With SIGHASH_SINGLE | SIGHASH_ANYONECANPAY, the second-stage HTLC transaction claims can be aggregated across commitment transactions, as there is no commitment to the spend txid in the shared transaction fields (e.g the parent txid could be in the nLocktime field). Our logic iterate on all the inputs, and at the first one with a confirmed commitment txid in counterparty_commitment_txn_on_chain, we'll call in check_spend_counterparty_htlc() L3028. There, we iterate against all inputs and if the input outpoint txid is different from the passed commitment_txid, we skip it further processing. Once we're out of check_spend_counterparty_htlc(), we end up here and break from the for control flow, am I correct ? At the very least I don't think we have a test coverage for this case.

[TheBlueMatt]

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

[wpaulino]

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

[ariard]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

Note, here we're checking the second-stage HTLC transactions, and we're filtering out the input/output pair to be claimed based on the spent commitment transaction id. Post-anchor output, second-stage HTLC transactions are signed with SIGHASH_SINGLE enabling aggregation of HTLC claims from the same commitment transaction, but there is no constraint restraining the aggregation of HTLC claims across commitment transactions. Even if for one ChannelMonitor there is a single commitment_txid that can confirms.

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

This is a correct, the claim of HTLCs is a per-ChannelMonitor responsibility, and I don't think there is a way open by malleability where the aggregated HTLC claims crafted by our counterparty could blind our parsing logic. Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

[wpaulino]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

Since the ChannelMonitor is already on a per-channel basis, the comment really means "all from the same commitment for this channel". I agree it could definitely be re-worded better though.

Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

Doesn't seem like there's much we can do other than document it? Even then, I would imagine all LDK users want to use our ChainMonitor as is. Rather than re-implementing it for their own needs, they just need to implement the trait dependencies on ChainMonitor.

[ariard]

Doesn't seem like there's much we can do other than document it?

More documentation of our assumptions is always better. I think with the extended flexibility offered by our interfaces we have far less visibility on how it could be re-implemented by users rather than a monolithic Lightning node directly consuming from standard Core interfaces. This is raising the bar for funds safety, as implicit assumptions of our low-level code could be silently broken. Beyond, we might have in the future to re-implement a ChainMonitor for "monitor-replica" support, a bit more sophisticated than the standard one.

[wpaulino]

This PR continues the work laid out in #1689 and introduces a new BumpTransactionEvent variant: HTLCResolution. Similarly, this event is to be consumed by users and indicates that a channel's commitment transaction with unresolved HTLCs has confirmed onchain, requiring HTLC transactions to be broadcast with additional inputs and/or outputs attached to satisfy feerate demands at the time of broadcast.

[wpaulino]

Rebased on latest to address an import conflict.

[codecov-commenter]

Codecov Report

Base: 90.57% // Head: 90.50% // Decreases project coverage by -0.07%⚠️

Coverage data is based on head (6a99960) compared to base (36e6023).
Patch coverage: 86.98% of modified lines in pull request are covered.

❗ Current head 6a99960 differs from pull request most recent head ec1f334. Consider uploading reports for the commit ec1f334 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1825 +/- ##
==========================================
- Coverage 90.57% 90.50% -0.08% 
==========================================
Files 91 91 Lines 48556 48731 +175 Branches 48556 48731 +175 ==========================================
+ Hits 43982 44102 +120 - Misses 4574 4629 +55 

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	83.14% <ø> (ø)
lightning/src/util/enforcing_trait_impls.rs	82.92% <ø> (ø)
lightning/src/util/events.rs	25.45% <ø> (ø)
lightning/src/chain/onchaintx.rs	92.73% <66.66%> (-2.62%)	⬇️
lightning/src/chain/package.rs	91.76% <68.57%> (-1.09%)	⬇️
lightning/src/ln/chan_utils.rs	93.61% <94.59%> (-0.02%)	⬇️
lightning/src/chain/channelmonitor.rs	91.04% <98.63%> (+0.11%)	⬆️
lightning/src/ln/onion_utils.rs	93.56% <0.00%> (-1.37%)	⬇️
lightning/src/ln/functional_tests.rs	96.65% <0.00%> (-0.35%)	⬇️
... and 5 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.

[wpaulino]

Pushed a new update addressing some remaining comments and a missing change noted by @ariard in #1860.

[TheBlueMatt]

Feel free to squash.

[wpaulino]

Squashed and rebased on latest to resolve a conflict.

[wpaulino]

> git diff -U1 a6eff8a f62e96f

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index b70121ba..cb31783d 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2651,13 +2651,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) where L::Target: Logger {
- macro_rules! ignore_error {
- ( $thing : expr ) => {
- match $thing {
- Ok(a) => a,
- Err(_) => return (Vec::new(), None)
- }
- };
- }
-
let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); };
- let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
+ let per_commitment_key = match SecretKey::from_slice(&secret) {
+ Ok(key) => key,
+ Err(_) => return (Vec::new(), None)
+ };
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
@@ -2667,6 +2661,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
let mut outputs_to_watch = None;
+ // Previously, we would only claim HTLCs from revoked HTLC transactions if they had 1 input
+ // with a witness of 5 elements and 1 output. This wasn't enough for anchor outputs, as the
+ // counterparty can now aggregate multiple HTLCs into a single transaction thanks to
+ // `SIGHASH_SINGLE` remote signatures, leading us to not claim any HTLCs upon seeing a
+ // confirmed revoked HTLC transaction (for more details, see
+ // https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-April/003561.html).
+ //
+ // We make sure we're not vulnerable to this case by checking all inputs of the transaction,
+ // and claim those which spend the commitment transaction, have a witness of 5 elements, and
+ // have a corresponding output at the same index within the transaction.
for (idx, input) in tx.input.iter().enumerate() {
- // HTLC transactions always spend an output on the commitment transaction with a witness
- // of 5 elements. The HTLC input will always have a corresponding output at the same
- // index within the transaction.
if input.previous_output.txid == *commitment_txid && input.witness.len() == 5 && tx.output.get(idx).is_some() {

[wpaulino]

@TheBlueMatt I addressed your remaining comments. Will open a new PR to address the serialization changes discussed in #1825 (comment).

[TheBlueMatt]

I left a handful of comments, ranging from mildly annoyed at what we have to trivial nits, but only one (the question about the 0th output) actually needs a response for this to land, I think.

[TheBlueMatt]

For both this and sign_holder_anchor_input I'm really not a fan of passing in a full tx-to-sign. I'm not sure what to do about it, though, we don't really want to constrain the tx contents really. Luckily segwit commits to the amount, but I could see someone forgetting to check the fee, for example. I guess we're really relying on the other input being signed as "the" security control here?

[wpaulino]

I'm not following -- we need to pass in the full transaction since it includes the additional fee inputs/outputs and our local signature is SIGHASH_ALL.

[TheBlueMatt]

I guess I was thinking more that we could pass the set of non-HTLC inputs, the output script and fee and then have the siger recreate the transaction. This avoids having to calculate the fee, check the locktime, etc. In general the principle for the signing interfaces should be (but isn't always) that we pass the user the things they need to construct the object to sign (and a utility method to do so) and make them do the construction. That way everything they need to validate is shoved in their face and they can't avoid looking at it (and hopefully remember to check it). Given our other HTLC-sign transactions already have this mistake it's fine for now, but in the future we should refactor all those methods to do the HTLC tx construction on the signer's end.

[wpaulino]

Updated docs and rebased on top of #1887 since it gets rid of some Results on HTLCDescriptor methods.

[ariard]

See last comment.

[ariard]

Here I think we can debug_assert!(seff.onchain_tx_handler.opt_anchors()).

[wpaulino]

We can probably replace most #[cfg(anchors)] with this assertion so will leave it for a follow-up.

[ariard]

Note if you would like to add a commit to recall this enum field FundingUnilateralResolution. A ChannelClose in Lightning parlance is really ambiguous as it designates also the cooperative case. This cooperative case itself could be a target for fee-bumping in the future (e.g splicing support). A small bikeshedding as we're around.

[TheBlueMatt]

If we want to rename it, let's do it in a followup. This PR has been delayed enough and isn't even touching this code.

[ariard]

There is no current indication on the level of external fee-bumping reserves a user is supposed to keep. For HTLC resolution, we would have to encompass the worst-case of holder's max_accepted_htlcs + counterparty's max_accepted_htlcs * 706 WU (for post-anchor HTLC-Success) * worst-historical mempool feerate. And this is only per-channel. Anyway, I don't know where we would like to start document those requirements here, in #1860 or even after ?

[TheBlueMatt]

Let's do it later.

[TheBlueMatt]

Needs rebase, think we can land this today or tomorrow?

[TheBlueMatt]

Looks like CI is sad? Feel free to squash the fixups.

[wpaulino]

There was a stray else causing the failures. Test suite passes locally now so we should be good.

[ariard]

On my side, I'll try to review back tomorrow. Checking the full transaction processing flow from ChainMonitor.

[arik-so]

I really like the comments explaining the potential vulnerability that might arise with anchor HTLC transactions.

[ariard]

See second-last comment.

[ariard]

I think this is assumption is broken, and per se our code will still miss the detection and claim of revoked second-stage HTLC transactions.

With SIGHASH_SINGLE | SIGHASH_ANYONECANPAY, the second-stage HTLC transaction claims can be aggregated across commitment transactions, as there is no commitment to the spend txid in the shared transaction fields (e.g the parent txid could be in the nLocktime field). Our logic iterate on all the inputs, and at the first one with a confirmed commitment txid in counterparty_commitment_txn_on_chain, we'll call in check_spend_counterparty_htlc() L3028. There, we iterate against all inputs and if the input outpoint txid is different from the passed commitment_txid, we skip it further processing. Once we're out of check_spend_counterparty_htlc(), we end up here and break from the for control flow, am I correct ? At the very least I don't think we have a test coverage for this case.

[TheBlueMatt]

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

[wpaulino]

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

[ariard]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

Note, here we're checking the second-stage HTLC transactions, and we're filtering out the input/output pair to be claimed based on the spent commitment transaction id. Post-anchor output, second-stage HTLC transactions are signed with SIGHASH_SINGLE enabling aggregation of HTLC claims from the same commitment transaction, but there is no constraint restraining the aggregation of HTLC claims across commitment transactions. Even if for one ChannelMonitor there is a single commitment_txid that can confirms.

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

This is a correct, the claim of HTLCs is a per-ChannelMonitor responsibility, and I don't think there is a way open by malleability where the aggregated HTLC claims crafted by our counterparty could blind our parsing logic. Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

[wpaulino]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

Since the ChannelMonitor is already on a per-channel basis, the comment really means "all from the same commitment for this channel". I agree it could definitely be re-worded better though.

Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

Doesn't seem like there's much we can do other than document it? Even then, I would imagine all LDK users want to use our ChainMonitor as is. Rather than re-implementing it for their own needs, they just need to implement the trait dependencies on ChainMonitor.

[ariard]

Doesn't seem like there's much we can do other than document it?

More documentation of our assumptions is always better. I think with the extended flexibility offered by our interfaces we have far less visibility on how it could be re-implemented by users rather than a monolithic Lightning node directly consuming from standard Core interfaces. This is raising the bar for funds safety, as implicit assumptions of our low-level code could be silently broken. Beyond, we might have in the future to re-implement a ChainMonitor for "monitor-replica" support, a bit more sophisticated than the standard one.

[wpaulino]

This PR continues the work laid out in #1689 and introduces a new BumpTransactionEvent variant: HTLCResolution. Similarly, this event is to be consumed by users and indicates that a channel's commitment transaction with unresolved HTLCs has confirmed onchain, requiring HTLC transactions to be broadcast with additional inputs and/or outputs attached to satisfy feerate demands at the time of broadcast.

[wpaulino]

Rebased on latest to address an import conflict.

[codecov-commenter]

Codecov Report

Base: 90.57% // Head: 90.50% // Decreases project coverage by -0.07%⚠️

Coverage data is based on head (6a99960) compared to base (36e6023).
Patch coverage: 86.98% of modified lines in pull request are covered.

❗ Current head 6a99960 differs from pull request most recent head ec1f334. Consider uploading reports for the commit ec1f334 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1825 +/- ##
==========================================
- Coverage 90.57% 90.50% -0.08% 
==========================================
Files 91 91 Lines 48556 48731 +175 Branches 48556 48731 +175 ==========================================
+ Hits 43982 44102 +120 - Misses 4574 4629 +55 

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	83.14% <ø> (ø)
lightning/src/util/enforcing_trait_impls.rs	82.92% <ø> (ø)
lightning/src/util/events.rs	25.45% <ø> (ø)
lightning/src/chain/onchaintx.rs	92.73% <66.66%> (-2.62%)	⬇️
lightning/src/chain/package.rs	91.76% <68.57%> (-1.09%)	⬇️
lightning/src/ln/chan_utils.rs	93.61% <94.59%> (-0.02%)	⬇️
lightning/src/chain/channelmonitor.rs	91.04% <98.63%> (+0.11%)	⬆️
lightning/src/ln/onion_utils.rs	93.56% <0.00%> (-1.37%)	⬇️
lightning/src/ln/functional_tests.rs	96.65% <0.00%> (-0.35%)	⬇️
... and 5 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.

[wpaulino]

Pushed a new update addressing some remaining comments and a missing change noted by @ariard in #1860.

[TheBlueMatt]

Feel free to squash.

[wpaulino]

Squashed and rebased on latest to resolve a conflict.

[wpaulino]

> git diff -U1 a6eff8a f62e96f

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index b70121ba..cb31783d 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2651,13 +2651,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) where L::Target: Logger {
- macro_rules! ignore_error {
- ( $thing : expr ) => {
- match $thing {
- Ok(a) => a,
- Err(_) => return (Vec::new(), None)
- }
- };
- }
-
let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); };
- let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
+ let per_commitment_key = match SecretKey::from_slice(&secret) {
+ Ok(key) => key,
+ Err(_) => return (Vec::new(), None)
+ };
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
@@ -2667,6 +2661,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
let mut outputs_to_watch = None;
+ // Previously, we would only claim HTLCs from revoked HTLC transactions if they had 1 input
+ // with a witness of 5 elements and 1 output. This wasn't enough for anchor outputs, as the
+ // counterparty can now aggregate multiple HTLCs into a single transaction thanks to
+ // `SIGHASH_SINGLE` remote signatures, leading us to not claim any HTLCs upon seeing a
+ // confirmed revoked HTLC transaction (for more details, see
+ // https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-April/003561.html).
+ //
+ // We make sure we're not vulnerable to this case by checking all inputs of the transaction,
+ // and claim those which spend the commitment transaction, have a witness of 5 elements, and
+ // have a corresponding output at the same index within the transaction.
for (idx, input) in tx.input.iter().enumerate() {
- // HTLC transactions always spend an output on the commitment transaction with a witness
- // of 5 elements. The HTLC input will always have a corresponding output at the same
- // index within the transaction.
if input.previous_output.txid == *commitment_txid && input.witness.len() == 5 && tx.output.get(idx).is_some() {

[wpaulino]

@TheBlueMatt I addressed your remaining comments. Will open a new PR to address the serialization changes discussed in #1825 (comment).

[TheBlueMatt]

I left a handful of comments, ranging from mildly annoyed at what we have to trivial nits, but only one (the question about the 0th output) actually needs a response for this to land, I think.

[TheBlueMatt]

For both this and sign_holder_anchor_input I'm really not a fan of passing in a full tx-to-sign. I'm not sure what to do about it, though, we don't really want to constrain the tx contents really. Luckily segwit commits to the amount, but I could see someone forgetting to check the fee, for example. I guess we're really relying on the other input being signed as "the" security control here?

[wpaulino]

I'm not following -- we need to pass in the full transaction since it includes the additional fee inputs/outputs and our local signature is SIGHASH_ALL.

[TheBlueMatt]

I guess I was thinking more that we could pass the set of non-HTLC inputs, the output script and fee and then have the siger recreate the transaction. This avoids having to calculate the fee, check the locktime, etc. In general the principle for the signing interfaces should be (but isn't always) that we pass the user the things they need to construct the object to sign (and a utility method to do so) and make them do the construction. That way everything they need to validate is shoved in their face and they can't avoid looking at it (and hopefully remember to check it). Given our other HTLC-sign transactions already have this mistake it's fine for now, but in the future we should refactor all those methods to do the HTLC tx construction on the signer's end.

[wpaulino]

Updated docs and rebased on top of #1887 since it gets rid of some Results on HTLCDescriptor methods.

[ariard]

See last comment.

[ariard]

Here I think we can debug_assert!(seff.onchain_tx_handler.opt_anchors()).

[wpaulino]

We can probably replace most #[cfg(anchors)] with this assertion so will leave it for a follow-up.

[ariard]

Note if you would like to add a commit to recall this enum field FundingUnilateralResolution. A ChannelClose in Lightning parlance is really ambiguous as it designates also the cooperative case. This cooperative case itself could be a target for fee-bumping in the future (e.g splicing support). A small bikeshedding as we're around.

[TheBlueMatt]

If we want to rename it, let's do it in a followup. This PR has been delayed enough and isn't even touching this code.

[ariard]

There is no current indication on the level of external fee-bumping reserves a user is supposed to keep. For HTLC resolution, we would have to encompass the worst-case of holder's max_accepted_htlcs + counterparty's max_accepted_htlcs * 706 WU (for post-anchor HTLC-Success) * worst-historical mempool feerate. And this is only per-channel. Anyway, I don't know where we would like to start document those requirements here, in #1860 or even after ?

[TheBlueMatt]

Let's do it later.

[TheBlueMatt]

Needs rebase, think we can land this today or tomorrow?

[TheBlueMatt]

Looks like CI is sad? Feel free to squash the fixups.

[wpaulino]

There was a stray else causing the failures. Test suite passes locally now so we should be good.

[ariard]

On my side, I'll try to review back tomorrow. Checking the full transaction processing flow from ChainMonitor.

[arik-so]

I really like the comments explaining the potential vulnerability that might arise with anchor HTLC transactions.

[ariard]

See second-last comment.

[ariard]

I think this is assumption is broken, and per se our code will still miss the detection and claim of revoked second-stage HTLC transactions.

With SIGHASH_SINGLE | SIGHASH_ANYONECANPAY, the second-stage HTLC transaction claims can be aggregated across commitment transactions, as there is no commitment to the spend txid in the shared transaction fields (e.g the parent txid could be in the nLocktime field). Our logic iterate on all the inputs, and at the first one with a confirmed commitment txid in counterparty_commitment_txn_on_chain, we'll call in check_spend_counterparty_htlc() L3028. There, we iterate against all inputs and if the input outpoint txid is different from the passed commitment_txid, we skip it further processing. Once we're out of check_spend_counterparty_htlc(), we end up here and break from the for control flow, am I correct ? At the very least I don't think we have a test coverage for this case.

[TheBlueMatt]

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

[wpaulino]

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

[ariard]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

Note, here we're checking the second-stage HTLC transactions, and we're filtering out the input/output pair to be claimed based on the spent commitment transaction id. Post-anchor output, second-stage HTLC transactions are signed with SIGHASH_SINGLE enabling aggregation of HTLC claims from the same commitment transaction, but there is no constraint restraining the aggregation of HTLC claims across commitment transactions. Even if for one ChannelMonitor there is a single commitment_txid that can confirms.

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

This is a correct, the claim of HTLCs is a per-ChannelMonitor responsibility, and I don't think there is a way open by malleability where the aggregated HTLC claims crafted by our counterparty could blind our parsing logic. Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

[wpaulino]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

Since the ChannelMonitor is already on a per-channel basis, the comment really means "all from the same commitment for this channel". I agree it could definitely be re-worded better though.

Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

Doesn't seem like there's much we can do other than document it? Even then, I would imagine all LDK users want to use our ChainMonitor as is. Rather than re-implementing it for their own needs, they just need to implement the trait dependencies on ChainMonitor.

[ariard]

Doesn't seem like there's much we can do other than document it?

More documentation of our assumptions is always better. I think with the extended flexibility offered by our interfaces we have far less visibility on how it could be re-implemented by users rather than a monolithic Lightning node directly consuming from standard Core interfaces. This is raising the bar for funds safety, as implicit assumptions of our low-level code could be silently broken. Beyond, we might have in the future to re-implement a ChainMonitor for "monitor-replica" support, a bit more sophisticated than the standard one.

[wpaulino]

This PR continues the work laid out in #1689 and introduces a new BumpTransactionEvent variant: HTLCResolution. Similarly, this event is to be consumed by users and indicates that a channel's commitment transaction with unresolved HTLCs has confirmed onchain, requiring HTLC transactions to be broadcast with additional inputs and/or outputs attached to satisfy feerate demands at the time of broadcast.

[wpaulino]

Rebased on latest to address an import conflict.

[codecov-commenter]

Codecov Report

Base: 90.57% // Head: 90.50% // Decreases project coverage by -0.07%⚠️

Coverage data is based on head (6a99960) compared to base (36e6023).
Patch coverage: 86.98% of modified lines in pull request are covered.

❗ Current head 6a99960 differs from pull request most recent head ec1f334. Consider uploading reports for the commit ec1f334 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1825 +/- ##
==========================================
- Coverage 90.57% 90.50% -0.08% 
==========================================
Files 91 91 Lines 48556 48731 +175 Branches 48556 48731 +175 ==========================================
+ Hits 43982 44102 +120 - Misses 4574 4629 +55 

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	83.14% <ø> (ø)
lightning/src/util/enforcing_trait_impls.rs	82.92% <ø> (ø)
lightning/src/util/events.rs	25.45% <ø> (ø)
lightning/src/chain/onchaintx.rs	92.73% <66.66%> (-2.62%)	⬇️
lightning/src/chain/package.rs	91.76% <68.57%> (-1.09%)	⬇️
lightning/src/ln/chan_utils.rs	93.61% <94.59%> (-0.02%)	⬇️
lightning/src/chain/channelmonitor.rs	91.04% <98.63%> (+0.11%)	⬆️
lightning/src/ln/onion_utils.rs	93.56% <0.00%> (-1.37%)	⬇️
lightning/src/ln/functional_tests.rs	96.65% <0.00%> (-0.35%)	⬇️
... and 5 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.

[wpaulino]

Pushed a new update addressing some remaining comments and a missing change noted by @ariard in #1860.

[TheBlueMatt]

Feel free to squash.

[wpaulino]

Squashed and rebased on latest to resolve a conflict.

[wpaulino]

> git diff -U1 a6eff8a f62e96f

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index b70121ba..cb31783d 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2651,13 +2651,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) where L::Target: Logger {
- macro_rules! ignore_error {
- ( $thing : expr ) => {
- match $thing {
- Ok(a) => a,
- Err(_) => return (Vec::new(), None)
- }
- };
- }
-
let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); };
- let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
+ let per_commitment_key = match SecretKey::from_slice(&secret) {
+ Ok(key) => key,
+ Err(_) => return (Vec::new(), None)
+ };
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
@@ -2667,6 +2661,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
let mut outputs_to_watch = None;
+ // Previously, we would only claim HTLCs from revoked HTLC transactions if they had 1 input
+ // with a witness of 5 elements and 1 output. This wasn't enough for anchor outputs, as the
+ // counterparty can now aggregate multiple HTLCs into a single transaction thanks to
+ // `SIGHASH_SINGLE` remote signatures, leading us to not claim any HTLCs upon seeing a
+ // confirmed revoked HTLC transaction (for more details, see
+ // https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-April/003561.html).
+ //
+ // We make sure we're not vulnerable to this case by checking all inputs of the transaction,
+ // and claim those which spend the commitment transaction, have a witness of 5 elements, and
+ // have a corresponding output at the same index within the transaction.
for (idx, input) in tx.input.iter().enumerate() {
- // HTLC transactions always spend an output on the commitment transaction with a witness
- // of 5 elements. The HTLC input will always have a corresponding output at the same
- // index within the transaction.
if input.previous_output.txid == *commitment_txid && input.witness.len() == 5 && tx.output.get(idx).is_some() {

[wpaulino]

@TheBlueMatt I addressed your remaining comments. Will open a new PR to address the serialization changes discussed in #1825 (comment).

[TheBlueMatt]

I left a handful of comments, ranging from mildly annoyed at what we have to trivial nits, but only one (the question about the 0th output) actually needs a response for this to land, I think.

[TheBlueMatt]

For both this and sign_holder_anchor_input I'm really not a fan of passing in a full tx-to-sign. I'm not sure what to do about it, though, we don't really want to constrain the tx contents really. Luckily segwit commits to the amount, but I could see someone forgetting to check the fee, for example. I guess we're really relying on the other input being signed as "the" security control here?

[wpaulino]

I'm not following -- we need to pass in the full transaction since it includes the additional fee inputs/outputs and our local signature is SIGHASH_ALL.

[TheBlueMatt]

I guess I was thinking more that we could pass the set of non-HTLC inputs, the output script and fee and then have the siger recreate the transaction. This avoids having to calculate the fee, check the locktime, etc. In general the principle for the signing interfaces should be (but isn't always) that we pass the user the things they need to construct the object to sign (and a utility method to do so) and make them do the construction. That way everything they need to validate is shoved in their face and they can't avoid looking at it (and hopefully remember to check it). Given our other HTLC-sign transactions already have this mistake it's fine for now, but in the future we should refactor all those methods to do the HTLC tx construction on the signer's end.

[wpaulino]

Updated docs and rebased on top of #1887 since it gets rid of some Results on HTLCDescriptor methods.

[ariard]

See last comment.

[ariard]

Here I think we can debug_assert!(seff.onchain_tx_handler.opt_anchors()).

[wpaulino]

We can probably replace most #[cfg(anchors)] with this assertion so will leave it for a follow-up.

[ariard]

Note if you would like to add a commit to recall this enum field FundingUnilateralResolution. A ChannelClose in Lightning parlance is really ambiguous as it designates also the cooperative case. This cooperative case itself could be a target for fee-bumping in the future (e.g splicing support). A small bikeshedding as we're around.

[TheBlueMatt]

If we want to rename it, let's do it in a followup. This PR has been delayed enough and isn't even touching this code.

[ariard]

There is no current indication on the level of external fee-bumping reserves a user is supposed to keep. For HTLC resolution, we would have to encompass the worst-case of holder's max_accepted_htlcs + counterparty's max_accepted_htlcs * 706 WU (for post-anchor HTLC-Success) * worst-historical mempool feerate. And this is only per-channel. Anyway, I don't know where we would like to start document those requirements here, in #1860 or even after ?

[TheBlueMatt]

Let's do it later.

[TheBlueMatt]

Needs rebase, think we can land this today or tomorrow?

[TheBlueMatt]

Looks like CI is sad? Feel free to squash the fixups.

[wpaulino]

There was a stray else causing the failures. Test suite passes locally now so we should be good.

[ariard]

On my side, I'll try to review back tomorrow. Checking the full transaction processing flow from ChainMonitor.

[arik-so]

I really like the comments explaining the potential vulnerability that might arise with anchor HTLC transactions.

[ariard]

See second-last comment.

[ariard]

I think this is assumption is broken, and per se our code will still miss the detection and claim of revoked second-stage HTLC transactions.

With SIGHASH_SINGLE | SIGHASH_ANYONECANPAY, the second-stage HTLC transaction claims can be aggregated across commitment transactions, as there is no commitment to the spend txid in the shared transaction fields (e.g the parent txid could be in the nLocktime field). Our logic iterate on all the inputs, and at the first one with a confirmed commitment txid in counterparty_commitment_txn_on_chain, we'll call in check_spend_counterparty_htlc() L3028. There, we iterate against all inputs and if the input outpoint txid is different from the passed commitment_txid, we skip it further processing. Once we're out of check_spend_counterparty_htlc(), we end up here and break from the for control flow, am I correct ? At the very least I don't think we have a test coverage for this case.

[TheBlueMatt]

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

[wpaulino]

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

[ariard]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

Note, here we're checking the second-stage HTLC transactions, and we're filtering out the input/output pair to be claimed based on the spent commitment transaction id. Post-anchor output, second-stage HTLC transactions are signed with SIGHASH_SINGLE enabling aggregation of HTLC claims from the same commitment transaction, but there is no constraint restraining the aggregation of HTLC claims across commitment transactions. Even if for one ChannelMonitor there is a single commitment_txid that can confirms.

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

This is a correct, the claim of HTLCs is a per-ChannelMonitor responsibility, and I don't think there is a way open by malleability where the aggregated HTLC claims crafted by our counterparty could blind our parsing logic. Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

[wpaulino]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

Since the ChannelMonitor is already on a per-channel basis, the comment really means "all from the same commitment for this channel". I agree it could definitely be re-worded better though.

Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

Doesn't seem like there's much we can do other than document it? Even then, I would imagine all LDK users want to use our ChainMonitor as is. Rather than re-implementing it for their own needs, they just need to implement the trait dependencies on ChainMonitor.

[ariard]

Doesn't seem like there's much we can do other than document it?

More documentation of our assumptions is always better. I think with the extended flexibility offered by our interfaces we have far less visibility on how it could be re-implemented by users rather than a monolithic Lightning node directly consuming from standard Core interfaces. This is raising the bar for funds safety, as implicit assumptions of our low-level code could be silently broken. Beyond, we might have in the future to re-implement a ChainMonitor for "monitor-replica" support, a bit more sophisticated than the standard one.

[wpaulino]

This PR continues the work laid out in #1689 and introduces a new BumpTransactionEvent variant: HTLCResolution. Similarly, this event is to be consumed by users and indicates that a channel's commitment transaction with unresolved HTLCs has confirmed onchain, requiring HTLC transactions to be broadcast with additional inputs and/or outputs attached to satisfy feerate demands at the time of broadcast.

[wpaulino]

Rebased on latest to address an import conflict.

[codecov-commenter]

Codecov Report

Base: 90.57% // Head: 90.50% // Decreases project coverage by -0.07%⚠️

Coverage data is based on head (6a99960) compared to base (36e6023).
Patch coverage: 86.98% of modified lines in pull request are covered.

❗ Current head 6a99960 differs from pull request most recent head ec1f334. Consider uploading reports for the commit ec1f334 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## main #1825 +/- ##
==========================================
- Coverage 90.57% 90.50% -0.08% 
==========================================
Files 91 91 Lines 48556 48731 +175 Branches 48556 48731 +175 ==========================================
+ Hits 43982 44102 +120 - Misses 4574 4629 +55 

Impacted Files	Coverage Δ
lightning/src/chain/keysinterface.rs	83.14% <ø> (ø)
lightning/src/util/enforcing_trait_impls.rs	82.92% <ø> (ø)
lightning/src/util/events.rs	25.45% <ø> (ø)
lightning/src/chain/onchaintx.rs	92.73% <66.66%> (-2.62%)	⬇️
lightning/src/chain/package.rs	91.76% <68.57%> (-1.09%)	⬇️
lightning/src/ln/chan_utils.rs	93.61% <94.59%> (-0.02%)	⬇️
lightning/src/chain/channelmonitor.rs	91.04% <98.63%> (+0.11%)	⬆️
lightning/src/ln/onion_utils.rs	93.56% <0.00%> (-1.37%)	⬇️
lightning/src/ln/functional_tests.rs	96.65% <0.00%> (-0.35%)	⬇️
... and 5 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.

[wpaulino]

Pushed a new update addressing some remaining comments and a missing change noted by @ariard in #1860.

[TheBlueMatt]

Feel free to squash.

[wpaulino]

Squashed and rebased on latest to resolve a conflict.

[wpaulino]

> git diff -U1 a6eff8a f62e96f

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index b70121ba..cb31783d 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -2651,13 +2651,7 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
) -> (Vec<PackageTemplate>, Option<TransactionOutputs>) where L::Target: Logger {
- macro_rules! ignore_error {
- ( $thing : expr ) => {
- match $thing {
- Ok(a) => a,
- Err(_) => return (Vec::new(), None)
- }
- };
- }
-
let secret = if let Some(secret) = self.get_secret(commitment_number) { secret } else { return (Vec::new(), None); };
- let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret));
+ let per_commitment_key = match SecretKey::from_slice(&secret) {
+ Ok(key) => key,
+ Err(_) => return (Vec::new(), None)
+ };
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
@@ -2667,6 +2661,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
let mut outputs_to_watch = None;
+ // Previously, we would only claim HTLCs from revoked HTLC transactions if they had 1 input
+ // with a witness of 5 elements and 1 output. This wasn't enough for anchor outputs, as the
+ // counterparty can now aggregate multiple HTLCs into a single transaction thanks to
+ // `SIGHASH_SINGLE` remote signatures, leading us to not claim any HTLCs upon seeing a
+ // confirmed revoked HTLC transaction (for more details, see
+ // https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-April/003561.html).
+ //
+ // We make sure we're not vulnerable to this case by checking all inputs of the transaction,
+ // and claim those which spend the commitment transaction, have a witness of 5 elements, and
+ // have a corresponding output at the same index within the transaction.
for (idx, input) in tx.input.iter().enumerate() {
- // HTLC transactions always spend an output on the commitment transaction with a witness
- // of 5 elements. The HTLC input will always have a corresponding output at the same
- // index within the transaction.
if input.previous_output.txid == *commitment_txid && input.witness.len() == 5 && tx.output.get(idx).is_some() {

[wpaulino]

@TheBlueMatt I addressed your remaining comments. Will open a new PR to address the serialization changes discussed in #1825 (comment).

[TheBlueMatt]

I left a handful of comments, ranging from mildly annoyed at what we have to trivial nits, but only one (the question about the 0th output) actually needs a response for this to land, I think.

[TheBlueMatt]

For both this and sign_holder_anchor_input I'm really not a fan of passing in a full tx-to-sign. I'm not sure what to do about it, though, we don't really want to constrain the tx contents really. Luckily segwit commits to the amount, but I could see someone forgetting to check the fee, for example. I guess we're really relying on the other input being signed as "the" security control here?

[wpaulino]

I'm not following -- we need to pass in the full transaction since it includes the additional fee inputs/outputs and our local signature is SIGHASH_ALL.

[TheBlueMatt]

I guess I was thinking more that we could pass the set of non-HTLC inputs, the output script and fee and then have the siger recreate the transaction. This avoids having to calculate the fee, check the locktime, etc. In general the principle for the signing interfaces should be (but isn't always) that we pass the user the things they need to construct the object to sign (and a utility method to do so) and make them do the construction. That way everything they need to validate is shoved in their face and they can't avoid looking at it (and hopefully remember to check it). Given our other HTLC-sign transactions already have this mistake it's fine for now, but in the future we should refactor all those methods to do the HTLC tx construction on the signer's end.

[wpaulino]

Updated docs and rebased on top of #1887 since it gets rid of some Results on HTLCDescriptor methods.

[ariard]

See last comment.

[ariard]

Here I think we can debug_assert!(seff.onchain_tx_handler.opt_anchors()).

[wpaulino]

We can probably replace most #[cfg(anchors)] with this assertion so will leave it for a follow-up.

[ariard]

Note if you would like to add a commit to recall this enum field FundingUnilateralResolution. A ChannelClose in Lightning parlance is really ambiguous as it designates also the cooperative case. This cooperative case itself could be a target for fee-bumping in the future (e.g splicing support). A small bikeshedding as we're around.

[TheBlueMatt]

If we want to rename it, let's do it in a followup. This PR has been delayed enough and isn't even touching this code.

[ariard]

There is no current indication on the level of external fee-bumping reserves a user is supposed to keep. For HTLC resolution, we would have to encompass the worst-case of holder's max_accepted_htlcs + counterparty's max_accepted_htlcs * 706 WU (for post-anchor HTLC-Success) * worst-historical mempool feerate. And this is only per-channel. Anyway, I don't know where we would like to start document those requirements here, in #1860 or even after ?

[TheBlueMatt]

Let's do it later.

[TheBlueMatt]

Needs rebase, think we can land this today or tomorrow?

[TheBlueMatt]

Looks like CI is sad? Feel free to squash the fixups.

[wpaulino]

There was a stray else causing the failures. Test suite passes locally now so we should be good.

[ariard]

On my side, I'll try to review back tomorrow. Checking the full transaction processing flow from ChainMonitor.

[arik-so]

I really like the comments explaining the potential vulnerability that might arise with anchor HTLC transactions.

[ariard]

See second-last comment.

[ariard]

I think this is assumption is broken, and per se our code will still miss the detection and claim of revoked second-stage HTLC transactions.

With SIGHASH_SINGLE | SIGHASH_ANYONECANPAY, the second-stage HTLC transaction claims can be aggregated across commitment transactions, as there is no commitment to the spend txid in the shared transaction fields (e.g the parent txid could be in the nLocktime field). Our logic iterate on all the inputs, and at the first one with a confirmed commitment txid in counterparty_commitment_txn_on_chain, we'll call in check_spend_counterparty_htlc() L3028. There, we iterate against all inputs and if the input outpoint txid is different from the passed commitment_txid, we skip it further processing. Once we're out of check_spend_counterparty_htlc(), we end up here and break from the for control flow, am I correct ? At the very least I don't think we have a test coverage for this case.

[TheBlueMatt]

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

[wpaulino]

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

[ariard]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if commitment_txid is set based on counterparty_commitment_txn_on_chain its definitely the one and only remote commitment transaction that is spendable. Thus, the loop in check_spend_counterparty_htlc looks correct to me.

Note, here we're checking the second-stage HTLC transactions, and we're filtering out the input/output pair to be claimed based on the spent commitment transaction id. Post-anchor output, second-stage HTLC transactions are signed with SIGHASH_SINGLE enabling aggregation of HTLC claims from the same commitment transaction, but there is no constraint restraining the aggregation of HTLC claims across commitment transactions. Even if for one ChannelMonitor there is a single commitment_txid that can confirms.

Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each ChannelMonitor to claim the HTLCs only for their channel.

This is a correct, the claim of HTLCs is a per-ChannelMonitor responsibility, and I don't think there is a way open by malleability where the aggregated HTLC claims crafted by our counterparty could blind our parsing logic. Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

[wpaulino]

Okay after looking more, the code is correct, even if the comment is wrong or confusing on the (all from the same commitment) as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.

Since the ChannelMonitor is already on a per-channel basis, the comment really means "all from the same commitment for this channel". I agree it could definitely be re-worded better though.

Note, it makes the logic dependent on the implementation of Chain::Confirm::transaction_confirmed. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.

Doesn't seem like there's much we can do other than document it? Even then, I would imagine all LDK users want to use our ChainMonitor as is. Rather than re-implementing it for their own needs, they just need to implement the trait dependencies on ChainMonitor.

[ariard]

Doesn't seem like there's much we can do other than document it?

More documentation of our assumptions is always better. I think with the extended flexibility offered by our interfaces we have far less visibility on how it could be re-implemented by users rather than a monolithic Lightning node directly consuming from standard Core interfaces. This is raising the bar for funds safety, as implicit assumptions of our low-level code could be silently broken. Beyond, we might have in the future to re-implement a ChainMonitor for "monitor-replica" support, a bit more sophisticated than the standard one.