Avoid unwrap'ing channel_parameters in to_counterparty signing
#2634
Conversation
Previously, `StaticPaymentOutputDescriptor`s did not include `channel_parameters` for the signer. As a result, when going to spend old `StaticPaymentOutputDescriptor`s, `InMemorySigner::sign_counterparty_payment_input` may be called with `channel_parameters` set to `None`. This should be fine, but in fa2a2ef we started relying on it (indirectly via `channel_features`) for signing. This caused an `unwrap` when spending old output descriptors. This is fixed here by simply avoiding the unwrap and assuming old `StaticPaymentOutputDescriptor`s represent non-anchor channels.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2634 +/- ##
==========================================
+ Coverage 89.10% 89.51% +0.40%
==========================================
Files 112 112
Lines 86932 90838 +3906
Branches 86932 90838 +3906
==========================================
+ Hits 77465 81316 +3851
- Misses 7239 7293 +54
- Partials 2228 2229 +1
☔ View full report in Codecov by Sentry. |
lightning/src/sign/mod.rs
Outdated
| let supports_anchors_zero_fee_htlc_tx = self.channel_parameters.as_ref() | ||
| .map(|params| params.channel_type_features.supports_anchors_zero_fee_htlc_tx()) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Might be worth adding a helper. We do this in create_spendable_outputs_psbt, too, and it looks like we have an unchecked use in sign_counterparty_commitment. Though maybe that is never reached in practice?
There was a problem hiding this comment.
Yeah sign_counterparty_commitment shouldn't happen, we always provide the parameters there before signing commitment transactions.
There was a problem hiding this comment.
Turns out the one in create_spendable_outputs_psbt is not on self but rather on StaticPaymentOutputDescriptor. While we could DRY up that usage and StaticPaymentOutputDescriptor::max_witness_length, it's probably not worth it because we may refactor the former as per #2605 (comment).
In the previous commit we fixed a bug where we were spuriously (indirectly) `unwrap`ing the `channel_parameters` in `InMemorySigner`. Here we make such bugs much less likely in the future by having the utilities which do the `unwrap`ing internally return `Option`s instead. This makes the `unwrap`s clear at the callsite.
This further documents the parameter-fetching utilities in `InMemorySigner` to hopefully make them more robust against future spurious `unwrap`s.
Previously,
StaticPaymentOutputDescriptors did not includechannel_parametersfor the signer. As a result, when going to spend oldStaticPaymentOutputDescriptors,InMemorySigner::sign_counterparty_payment_inputmay be called withchannel_parametersset toNone. This should be fine, but in fa2a2ef we started relying on it (indirectly viachannel_features) for signing. This caused anunwrapwhen spending old output descriptors.This is fixed here by simply avoiding the unwrap and assuming old
StaticPaymentOutputDescriptors represent non-anchor channels.