Use sign_holder_htlc_transaction to sign non-anchors holder HTLCs by wpaulino · Pull Request #2667 · lightningdevkit/rust-lightning

wpaulino · 2023-10-16T19:34:48Z

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels due to a signature cache. This PR removed said cache completely and switches to signing holder HTLCs with the existing sign_holder_htlc_transaction signing method, previously only used for anchor channels.

ariard

Re-broadcasting at every block still means every competing replacement must at least commit feerate in mempool

We plan to use `EcdsaChannelSigner::sign_holder_htlc_transaction` to also sign holder HTLC transactions on non-anchor outputs channels. `HTLCDescriptor` was only used in an anchor outputs context, so a few things needed changing, mostly to handle the different scripts and feerate.

`OnchainTxHandler` will need to construct `HTLCDescriptor`s for holder HTLCs, but it did not have access to all of the derivation parameters that need to be provided.

codecov-commenter · 2023-10-20T18:06:50Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (b1d3aa8) 89.06% compared to head (a5fd95d) 88.72%.
Report is 62 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #2667 +/- ##
==========================================
- Coverage 89.06% 88.72% -0.34% 
==========================================
Files 112 112 Lines 87417 88365 +948 Branches 87417 88365 +948 ==========================================
+ Hits 77856 78405 +549 - Misses 7322 7718 +396 - Partials 2239 2242 +3

Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`84.91% <100.00%> (+0.01%)`	⬆️
lightning/src/events/bump_transaction.rs	`82.86% <ø> (+0.52%)`	⬆️
lightning/src/ln/chan_utils.rs	`91.02% <100.00%> (-1.19%)`	⬇️
lightning/src/ln/channel.rs	`88.38% <ø> (+0.01%)`	⬆️
lightning/src/util/test_channel_signer.rs	`85.18% <92.30%> (-0.90%)`	⬇️
lightning/src/chain/onchaintx.rs	`80.13% <92.30%> (-1.72%)`	⬇️
lightning/src/ln/monitor_tests.rs	`98.01% <95.69%> (-0.54%)`	⬇️
lightning/src/sign/mod.rs	`74.86% <80.68%> (+0.07%)`	⬆️

... and 36 files with indirect coverage changes

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

jkczyz · 2023-10-20T20:43:40Z

Looks good after a couple minor comments. Much more mechanical than I first thought.

ariard · 2023-10-20T20:46:48Z

won’t have time to review further, though test coverage sounds good from previous look.

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels. We can easily do this by reusing the existing `EcdsaChannelSigner::sign_holder_htlc_transaction` method and circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs` caches, which will be removed in a later commit anyway.

Since we want our holder HTLC signatures to be randomly generated and not reused, our existing caches are useless now, so we opt to remove them.

`sign_holder_commitment_and_htlcs` never really made sense. Unlike `sign_counterparty_commitment`, the signatures for holder HTLC transactions may be required much later than the commitment transaction's. While it was nice for us to only reach the signer once to obtain all holder signatures, it's not really ideal anymore as we want our signatures to be random and not reused. We no longer return all holder HTLC signatures and instead defer to obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.

Now that `HTLCDescriptor` is no longer specific to anchors, it doesn't make sense for it to live in the `bump_transaction` module anymore.

0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke

wpaulino · 2023-10-16T19:34:48Z

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels due to a signature cache. This PR removed said cache completely and switches to signing holder HTLCs with the existing sign_holder_htlc_transaction signing method, previously only used for anchor channels.

ariard

Re-broadcasting at every block still means every competing replacement must at least commit feerate in mempool

We plan to use `EcdsaChannelSigner::sign_holder_htlc_transaction` to also sign holder HTLC transactions on non-anchor outputs channels. `HTLCDescriptor` was only used in an anchor outputs context, so a few things needed changing, mostly to handle the different scripts and feerate.

`OnchainTxHandler` will need to construct `HTLCDescriptor`s for holder HTLCs, but it did not have access to all of the derivation parameters that need to be provided.

codecov-commenter · 2023-10-20T18:06:50Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (b1d3aa8) 89.06% compared to head (a5fd95d) 88.72%.
Report is 62 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #2667 +/- ##
==========================================
- Coverage 89.06% 88.72% -0.34% 
==========================================
Files 112 112 Lines 87417 88365 +948 Branches 87417 88365 +948 ==========================================
+ Hits 77856 78405 +549 - Misses 7322 7718 +396 - Partials 2239 2242 +3

Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`84.91% <100.00%> (+0.01%)`	⬆️
lightning/src/events/bump_transaction.rs	`82.86% <ø> (+0.52%)`	⬆️
lightning/src/ln/chan_utils.rs	`91.02% <100.00%> (-1.19%)`	⬇️
lightning/src/ln/channel.rs	`88.38% <ø> (+0.01%)`	⬆️
lightning/src/util/test_channel_signer.rs	`85.18% <92.30%> (-0.90%)`	⬇️
lightning/src/chain/onchaintx.rs	`80.13% <92.30%> (-1.72%)`	⬇️
lightning/src/ln/monitor_tests.rs	`98.01% <95.69%> (-0.54%)`	⬇️
lightning/src/sign/mod.rs	`74.86% <80.68%> (+0.07%)`	⬆️

... and 36 files with indirect coverage changes

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

jkczyz · 2023-10-20T20:43:40Z

Looks good after a couple minor comments. Much more mechanical than I first thought.

ariard · 2023-10-20T20:46:48Z

won’t have time to review further, though test coverage sounds good from previous look.

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels. We can easily do this by reusing the existing `EcdsaChannelSigner::sign_holder_htlc_transaction` method and circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs` caches, which will be removed in a later commit anyway.

Since we want our holder HTLC signatures to be randomly generated and not reused, our existing caches are useless now, so we opt to remove them.

`sign_holder_commitment_and_htlcs` never really made sense. Unlike `sign_counterparty_commitment`, the signatures for holder HTLC transactions may be required much later than the commitment transaction's. While it was nice for us to only reach the signer once to obtain all holder signatures, it's not really ideal anymore as we want our signatures to be random and not reused. We no longer return all holder HTLC signatures and instead defer to obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.

Now that `HTLCDescriptor` is no longer specific to anchors, it doesn't make sense for it to live in the `bump_transaction` module anymore.

0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke

wpaulino · 2023-10-16T19:34:48Z

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels due to a signature cache. This PR removed said cache completely and switches to signing holder HTLCs with the existing sign_holder_htlc_transaction signing method, previously only used for anchor channels.

ariard

Re-broadcasting at every block still means every competing replacement must at least commit feerate in mempool

We plan to use `EcdsaChannelSigner::sign_holder_htlc_transaction` to also sign holder HTLC transactions on non-anchor outputs channels. `HTLCDescriptor` was only used in an anchor outputs context, so a few things needed changing, mostly to handle the different scripts and feerate.

`OnchainTxHandler` will need to construct `HTLCDescriptor`s for holder HTLCs, but it did not have access to all of the derivation parameters that need to be provided.

codecov-commenter · 2023-10-20T18:06:50Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (b1d3aa8) 89.06% compared to head (a5fd95d) 88.72%.
Report is 62 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #2667 +/- ##
==========================================
- Coverage 89.06% 88.72% -0.34% 
==========================================
Files 112 112 Lines 87417 88365 +948 Branches 87417 88365 +948 ==========================================
+ Hits 77856 78405 +549 - Misses 7322 7718 +396 - Partials 2239 2242 +3

Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`84.91% <100.00%> (+0.01%)`	⬆️
lightning/src/events/bump_transaction.rs	`82.86% <ø> (+0.52%)`	⬆️
lightning/src/ln/chan_utils.rs	`91.02% <100.00%> (-1.19%)`	⬇️
lightning/src/ln/channel.rs	`88.38% <ø> (+0.01%)`	⬆️
lightning/src/util/test_channel_signer.rs	`85.18% <92.30%> (-0.90%)`	⬇️
lightning/src/chain/onchaintx.rs	`80.13% <92.30%> (-1.72%)`	⬇️
lightning/src/ln/monitor_tests.rs	`98.01% <95.69%> (-0.54%)`	⬇️
lightning/src/sign/mod.rs	`74.86% <80.68%> (+0.07%)`	⬆️

... and 36 files with indirect coverage changes

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

jkczyz · 2023-10-20T20:43:40Z

Looks good after a couple minor comments. Much more mechanical than I first thought.

ariard · 2023-10-20T20:46:48Z

won’t have time to review further, though test coverage sounds good from previous look.

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels. We can easily do this by reusing the existing `EcdsaChannelSigner::sign_holder_htlc_transaction` method and circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs` caches, which will be removed in a later commit anyway.

Since we want our holder HTLC signatures to be randomly generated and not reused, our existing caches are useless now, so we opt to remove them.

`sign_holder_commitment_and_htlcs` never really made sense. Unlike `sign_counterparty_commitment`, the signatures for holder HTLC transactions may be required much later than the commitment transaction's. While it was nice for us to only reach the signer once to obtain all holder signatures, it's not really ideal anymore as we want our signatures to be random and not reused. We no longer return all holder HTLC signatures and instead defer to obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.

Now that `HTLCDescriptor` is no longer specific to anchors, it doesn't make sense for it to live in the `bump_transaction` module anymore.

0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke

wpaulino · 2023-10-16T19:34:48Z

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels due to a signature cache. This PR removed said cache completely and switches to signing holder HTLCs with the existing sign_holder_htlc_transaction signing method, previously only used for anchor channels.

ariard

Re-broadcasting at every block still means every competing replacement must at least commit feerate in mempool

We plan to use `EcdsaChannelSigner::sign_holder_htlc_transaction` to also sign holder HTLC transactions on non-anchor outputs channels. `HTLCDescriptor` was only used in an anchor outputs context, so a few things needed changing, mostly to handle the different scripts and feerate.

`OnchainTxHandler` will need to construct `HTLCDescriptor`s for holder HTLCs, but it did not have access to all of the derivation parameters that need to be provided.

codecov-commenter · 2023-10-20T18:06:50Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (b1d3aa8) 89.06% compared to head (a5fd95d) 88.72%.
Report is 62 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #2667 +/- ##
==========================================
- Coverage 89.06% 88.72% -0.34% 
==========================================
Files 112 112 Lines 87417 88365 +948 Branches 87417 88365 +948 ==========================================
+ Hits 77856 78405 +549 - Misses 7322 7718 +396 - Partials 2239 2242 +3

Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`84.91% <100.00%> (+0.01%)`	⬆️
lightning/src/events/bump_transaction.rs	`82.86% <ø> (+0.52%)`	⬆️
lightning/src/ln/chan_utils.rs	`91.02% <100.00%> (-1.19%)`	⬇️
lightning/src/ln/channel.rs	`88.38% <ø> (+0.01%)`	⬆️
lightning/src/util/test_channel_signer.rs	`85.18% <92.30%> (-0.90%)`	⬇️
lightning/src/chain/onchaintx.rs	`80.13% <92.30%> (-1.72%)`	⬇️
lightning/src/ln/monitor_tests.rs	`98.01% <95.69%> (-0.54%)`	⬇️
lightning/src/sign/mod.rs	`74.86% <80.68%> (+0.07%)`	⬆️

... and 36 files with indirect coverage changes

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

jkczyz · 2023-10-20T20:43:40Z

Looks good after a couple minor comments. Much more mechanical than I first thought.

ariard · 2023-10-20T20:46:48Z

won’t have time to review further, though test coverage sounds good from previous look.

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels. We can easily do this by reusing the existing `EcdsaChannelSigner::sign_holder_htlc_transaction` method and circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs` caches, which will be removed in a later commit anyway.

Since we want our holder HTLC signatures to be randomly generated and not reused, our existing caches are useless now, so we opt to remove them.

`sign_holder_commitment_and_htlcs` never really made sense. Unlike `sign_counterparty_commitment`, the signatures for holder HTLC transactions may be required much later than the commitment transaction's. While it was nice for us to only reach the signer once to obtain all holder signatures, it's not really ideal anymore as we want our signatures to be random and not reused. We no longer return all holder HTLC signatures and instead defer to obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.

Now that `HTLCDescriptor` is no longer specific to anchors, it doesn't make sense for it to live in the `bump_transaction` module anymore.

0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke

wpaulino · 2023-10-16T19:34:48Z

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels due to a signature cache. This PR removed said cache completely and switches to signing holder HTLCs with the existing sign_holder_htlc_transaction signing method, previously only used for anchor channels.

ariard

Re-broadcasting at every block still means every competing replacement must at least commit feerate in mempool

We plan to use `EcdsaChannelSigner::sign_holder_htlc_transaction` to also sign holder HTLC transactions on non-anchor outputs channels. `HTLCDescriptor` was only used in an anchor outputs context, so a few things needed changing, mostly to handle the different scripts and feerate.

`OnchainTxHandler` will need to construct `HTLCDescriptor`s for holder HTLCs, but it did not have access to all of the derivation parameters that need to be provided.

codecov-commenter · 2023-10-20T18:06:50Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (b1d3aa8) 89.06% compared to head (a5fd95d) 88.72%.
Report is 62 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #2667 +/- ##
==========================================
- Coverage 89.06% 88.72% -0.34% 
==========================================
Files 112 112 Lines 87417 88365 +948 Branches 87417 88365 +948 ==========================================
+ Hits 77856 78405 +549 - Misses 7322 7718 +396 - Partials 2239 2242 +3

Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`84.91% <100.00%> (+0.01%)`	⬆️
lightning/src/events/bump_transaction.rs	`82.86% <ø> (+0.52%)`	⬆️
lightning/src/ln/chan_utils.rs	`91.02% <100.00%> (-1.19%)`	⬇️
lightning/src/ln/channel.rs	`88.38% <ø> (+0.01%)`	⬆️
lightning/src/util/test_channel_signer.rs	`85.18% <92.30%> (-0.90%)`	⬇️
lightning/src/chain/onchaintx.rs	`80.13% <92.30%> (-1.72%)`	⬇️
lightning/src/ln/monitor_tests.rs	`98.01% <95.69%> (-0.54%)`	⬇️
lightning/src/sign/mod.rs	`74.86% <80.68%> (+0.07%)`	⬆️

... and 36 files with indirect coverage changes

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

jkczyz · 2023-10-20T20:43:40Z

Looks good after a couple minor comments. Much more mechanical than I first thought.

ariard · 2023-10-20T20:46:48Z

won’t have time to review further, though test coverage sounds good from previous look.

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels. We can easily do this by reusing the existing `EcdsaChannelSigner::sign_holder_htlc_transaction` method and circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs` caches, which will be removed in a later commit anyway.

Since we want our holder HTLC signatures to be randomly generated and not reused, our existing caches are useless now, so we opt to remove them.

`sign_holder_commitment_and_htlcs` never really made sense. Unlike `sign_counterparty_commitment`, the signatures for holder HTLC transactions may be required much later than the commitment transaction's. While it was nice for us to only reach the signer once to obtain all holder signatures, it's not really ideal anymore as we want our signatures to be random and not reused. We no longer return all holder HTLC signatures and instead defer to obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.

Now that `HTLCDescriptor` is no longer specific to anchors, it doesn't make sense for it to live in the `bump_transaction` module anymore.

0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke

wpaulino · 2023-10-16T19:34:48Z

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels due to a signature cache. This PR removed said cache completely and switches to signing holder HTLCs with the existing sign_holder_htlc_transaction signing method, previously only used for anchor channels.

ariard

Re-broadcasting at every block still means every competing replacement must at least commit feerate in mempool

We plan to use `EcdsaChannelSigner::sign_holder_htlc_transaction` to also sign holder HTLC transactions on non-anchor outputs channels. `HTLCDescriptor` was only used in an anchor outputs context, so a few things needed changing, mostly to handle the different scripts and feerate.

`OnchainTxHandler` will need to construct `HTLCDescriptor`s for holder HTLCs, but it did not have access to all of the derivation parameters that need to be provided.

codecov-commenter · 2023-10-20T18:06:50Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (b1d3aa8) 89.06% compared to head (a5fd95d) 88.72%.
Report is 62 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #2667 +/- ##
==========================================
- Coverage 89.06% 88.72% -0.34% 
==========================================
Files 112 112 Lines 87417 88365 +948 Branches 87417 88365 +948 ==========================================
+ Hits 77856 78405 +549 - Misses 7322 7718 +396 - Partials 2239 2242 +3

Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`84.91% <100.00%> (+0.01%)`	⬆️
lightning/src/events/bump_transaction.rs	`82.86% <ø> (+0.52%)`	⬆️
lightning/src/ln/chan_utils.rs	`91.02% <100.00%> (-1.19%)`	⬇️
lightning/src/ln/channel.rs	`88.38% <ø> (+0.01%)`	⬆️
lightning/src/util/test_channel_signer.rs	`85.18% <92.30%> (-0.90%)`	⬇️
lightning/src/chain/onchaintx.rs	`80.13% <92.30%> (-1.72%)`	⬇️
lightning/src/ln/monitor_tests.rs	`98.01% <95.69%> (-0.54%)`	⬇️
lightning/src/sign/mod.rs	`74.86% <80.68%> (+0.07%)`	⬆️

... and 36 files with indirect coverage changes

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

jkczyz · 2023-10-20T20:43:40Z

Looks good after a couple minor comments. Much more mechanical than I first thought.

ariard · 2023-10-20T20:46:48Z

won’t have time to review further, though test coverage sounds good from previous look.

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels. We can easily do this by reusing the existing `EcdsaChannelSigner::sign_holder_htlc_transaction` method and circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs` caches, which will be removed in a later commit anyway.

Since we want our holder HTLC signatures to be randomly generated and not reused, our existing caches are useless now, so we opt to remove them.

`sign_holder_commitment_and_htlcs` never really made sense. Unlike `sign_counterparty_commitment`, the signatures for holder HTLC transactions may be required much later than the commitment transaction's. While it was nice for us to only reach the signer once to obtain all holder signatures, it's not really ideal anymore as we want our signatures to be random and not reused. We no longer return all holder HTLC signatures and instead defer to obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.

Now that `HTLCDescriptor` is no longer specific to anchors, it doesn't make sense for it to live in the `bump_transaction` module anymore.

0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke

wpaulino · 2023-10-16T19:34:48Z

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels due to a signature cache. This PR removed said cache completely and switches to signing holder HTLCs with the existing sign_holder_htlc_transaction signing method, previously only used for anchor channels.

ariard

Re-broadcasting at every block still means every competing replacement must at least commit feerate in mempool

We plan to use `EcdsaChannelSigner::sign_holder_htlc_transaction` to also sign holder HTLC transactions on non-anchor outputs channels. `HTLCDescriptor` was only used in an anchor outputs context, so a few things needed changing, mostly to handle the different scripts and feerate.

`OnchainTxHandler` will need to construct `HTLCDescriptor`s for holder HTLCs, but it did not have access to all of the derivation parameters that need to be provided.

codecov-commenter · 2023-10-20T18:06:50Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (b1d3aa8) 89.06% compared to head (a5fd95d) 88.72%.
Report is 62 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #2667 +/- ##
==========================================
- Coverage 89.06% 88.72% -0.34% 
==========================================
Files 112 112 Lines 87417 88365 +948 Branches 87417 88365 +948 ==========================================
+ Hits 77856 78405 +549 - Misses 7322 7718 +396 - Partials 2239 2242 +3

Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`84.91% <100.00%> (+0.01%)`	⬆️
lightning/src/events/bump_transaction.rs	`82.86% <ø> (+0.52%)`	⬆️
lightning/src/ln/chan_utils.rs	`91.02% <100.00%> (-1.19%)`	⬇️
lightning/src/ln/channel.rs	`88.38% <ø> (+0.01%)`	⬆️
lightning/src/util/test_channel_signer.rs	`85.18% <92.30%> (-0.90%)`	⬇️
lightning/src/chain/onchaintx.rs	`80.13% <92.30%> (-1.72%)`	⬇️
lightning/src/ln/monitor_tests.rs	`98.01% <95.69%> (-0.54%)`	⬇️
lightning/src/sign/mod.rs	`74.86% <80.68%> (+0.07%)`	⬆️

... and 36 files with indirect coverage changes

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

jkczyz · 2023-10-20T20:43:40Z

Looks good after a couple minor comments. Much more mechanical than I first thought.

ariard · 2023-10-20T20:46:48Z

won’t have time to review further, though test coverage sounds good from previous look.

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels. We can easily do this by reusing the existing `EcdsaChannelSigner::sign_holder_htlc_transaction` method and circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs` caches, which will be removed in a later commit anyway.

Since we want our holder HTLC signatures to be randomly generated and not reused, our existing caches are useless now, so we opt to remove them.

`sign_holder_commitment_and_htlcs` never really made sense. Unlike `sign_counterparty_commitment`, the signatures for holder HTLC transactions may be required much later than the commitment transaction's. While it was nice for us to only reach the signer once to obtain all holder signatures, it's not really ideal anymore as we want our signatures to be random and not reused. We no longer return all holder HTLC signatures and instead defer to obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.

Now that `HTLCDescriptor` is no longer specific to anchors, it doesn't make sense for it to live in the `bump_transaction` module anymore.

0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke

wpaulino · 2023-10-16T19:34:48Z

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels due to a signature cache. This PR removed said cache completely and switches to signing holder HTLCs with the existing sign_holder_htlc_transaction signing method, previously only used for anchor channels.

ariard

Re-broadcasting at every block still means every competing replacement must at least commit feerate in mempool

We plan to use `EcdsaChannelSigner::sign_holder_htlc_transaction` to also sign holder HTLC transactions on non-anchor outputs channels. `HTLCDescriptor` was only used in an anchor outputs context, so a few things needed changing, mostly to handle the different scripts and feerate.

`OnchainTxHandler` will need to construct `HTLCDescriptor`s for holder HTLCs, but it did not have access to all of the derivation parameters that need to be provided.

codecov-commenter · 2023-10-20T18:06:50Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (b1d3aa8) 89.06% compared to head (a5fd95d) 88.72%.
Report is 62 commits behind head on main.

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

Additional details and impacted files

@@ Coverage Diff @@## main #2667 +/- ##
==========================================
- Coverage 89.06% 88.72% -0.34% 
==========================================
Files 112 112 Lines 87417 88365 +948 Branches 87417 88365 +948 ==========================================
+ Hits 77856 78405 +549 - Misses 7322 7718 +396 - Partials 2239 2242 +3

Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`84.91% <100.00%> (+0.01%)`	⬆️
lightning/src/events/bump_transaction.rs	`82.86% <ø> (+0.52%)`	⬆️
lightning/src/ln/chan_utils.rs	`91.02% <100.00%> (-1.19%)`	⬇️
lightning/src/ln/channel.rs	`88.38% <ø> (+0.01%)`	⬆️
lightning/src/util/test_channel_signer.rs	`85.18% <92.30%> (-0.90%)`	⬇️
lightning/src/chain/onchaintx.rs	`80.13% <92.30%> (-1.72%)`	⬇️
lightning/src/ln/monitor_tests.rs	`98.01% <95.69%> (-0.54%)`	⬇️
lightning/src/sign/mod.rs	`74.86% <80.68%> (+0.07%)`	⬆️

... and 36 files with indirect coverage changes

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

jkczyz · 2023-10-20T20:43:40Z

Looks good after a couple minor comments. Much more mechanical than I first thought.

ariard · 2023-10-20T20:46:48Z

won’t have time to review further, though test coverage sounds good from previous look.

We want to ensure we use fresh random signatures to prevent certain classes of transaction replacement attacks at the bitcoin P2P layer. This was already covered for commitment transactions and zero fee holder HTLC transactions, but was missing for holder HTLC transactions on non-anchors channels. We can easily do this by reusing the existing `EcdsaChannelSigner::sign_holder_htlc_transaction` method and circumventing the existing `holder_htlc_sigs/prev_holder_htlc_sigs` caches, which will be removed in a later commit anyway.

Since we want our holder HTLC signatures to be randomly generated and not reused, our existing caches are useless now, so we opt to remove them.

`sign_holder_commitment_and_htlcs` never really made sense. Unlike `sign_counterparty_commitment`, the signatures for holder HTLC transactions may be required much later than the commitment transaction's. While it was nice for us to only reach the signer once to obtain all holder signatures, it's not really ideal anymore as we want our signatures to be random and not reused. We no longer return all holder HTLC signatures and instead defer to obtaining them via `EcdsaChannelSigner::sign_holder_htlc_transaction`.

Now that `HTLCDescriptor` is no longer specific to anchors, it doesn't make sense for it to live in the `bump_transaction` module anymore.

0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke

wpaulino mentioned this pull request Oct 16, 2023
Support async get_per_commitment_point, release_commitment_secret #2653
Closed

ariard reviewed Oct 17, 2023
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs Outdated
Comment threadlightning/src/ln/monitor_tests.rs
Comment threadlightning/src/ln/monitor_tests.rs

wpaulino added this to the 0.0.118 milestone Oct 17, 2023

ariard reviewed Oct 18, 2023
View reviewed changes

Comment threadlightning/src/sign/mod.rs Outdated

TheBlueMatt assigned TheBlueMatt, arik-so and jkczyz Oct 18, 2023

wpaulino added 2 commits October 20, 2023 11:03

Provide missing derivation parameters to OnchainTxHandler
5958604
`OnchainTxHandler` will need to construct `HTLCDescriptor`s for holder HTLCs, but it did not have access to all of the derivation parameters that need to be provided.

wpaulino force-pushed the random-htlc-holder-sigs-non-anchors branch from 4f1d1f8 to dc1c232Compare October 20, 2023 18:06

TheBlueMatt previously approved these changes Oct 20, 2023
View reviewed changes

Comment threadlightning/src/chain/onchaintx.rs Outdated

jkczyz reviewed Oct 20, 2023
View reviewed changes

Comment threadlightning/src/chain/onchaintx.rs Outdated
Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/sign/mod.rs

wpaulino dismissed TheBlueMatt’s stale review via c171169October 20, 2023 21:48

wpaulino force-pushed the random-htlc-holder-sigs-non-anchors branch 2 times, most recently from c171169 to 225a860Compare October 20, 2023 21:55

jkczyz reviewed Oct 20, 2023
View reviewed changes

Comment threadlightning/src/sign/mod.rs

jkczyz reviewed Oct 20, 2023
View reviewed changes

Comment threadlightning/src/sign/mod.rs

wpaulino force-pushed the random-htlc-holder-sigs-non-anchors branch from 225a860 to a5fd95dCompare October 20, 2023 22:09

jkczyz previously approved these changes Oct 20, 2023
View reviewed changes

Comment threadlightning/src/chain/onchaintx.rs Outdated

wpaulino added 4 commits October 20, 2023 15:32

Remove caching of holder HTLC signatures
a9d9d26
Since we want our holder HTLC signatures to be randomly generated and not reused, our existing caches are useless now, so we opt to remove them.

Move HTLCDescriptor to sign module
b06a652
Now that `HTLCDescriptor` is no longer specific to anchors, it doesn't make sense for it to live in the `bump_transaction` module anymore.

wpaulino dismissed jkczyz’s stale review via b06a652October 20, 2023 22:32

wpaulino force-pushed the random-htlc-holder-sigs-non-anchors branch from a5fd95d to b06a652Compare October 20, 2023 22:32

jkczyz approved these changes Oct 20, 2023
View reviewed changes

TheBlueMatt approved these changes Oct 20, 2023
View reviewed changes

TheBlueMatt merged commit a1a2f2a into lightningdevkit:mainOct 20, 2023

wpaulino deleted the random-htlc-holder-sigs-non-anchors branch October 20, 2023 23:25