Low level anchor transaction build support

[devrandom]

This pulls out changes to CommitmentTransaction from #725.

Note that the approach is slightly different from @ariard's original implementation. We can already tell whether an anchor needs inclusion by whether the party's output is non-zero or there are HTLCs. We just need to know the value of opt_anchors and the funding pubkeys.

This does not yet add anchor support in Channel.

[devrandom]

Another motivation for this is that Lightning Signer needs this for adding anchor support, since we depend on LDK for transaction construction.

[codecov]

Codecov Report

Merging #1055 (8275698) into main (4368b56) will increase coverage by 1.22%.
The diff coverage is 95.78%.

@@            Coverage Diff             @@
##             main    #1055      +/-   ##
==========================================
+ Coverage   90.82%   92.04%   +1.22%     
==========================================
  Files          65       65              
  Lines       32890    37896    +5006     
==========================================
+ Hits        29872    34883    +5011     
+ Misses       3018     3013       -5     

Impacted Files	Coverage Δ
lightning/src/util/ser.rs	91.68% <0.00%> (-0.52%)	⬇️
lightning/src/ln/chan_utils.rs	97.41% <97.53%> (-0.02%)	⬇️
lightning/src/ln/channel.rs	92.38% <100.00%> (+3.67%)	⬆️
lightning/src/ln/functional_tests.rs	98.84% <100.00%> (+1.35%)	⬆️
lightning/src/util/enforcing_trait_impls.rs	89.54% <0.00%> (-0.85%)	⬇️
lightning-invoice/src/de.rs	80.62% <0.00%> (-0.32%)	⬇️
lightning/src/ln/features.rs	99.60% <0.00%> (+0.16%)	⬆️
lightning/src/chain/keysinterface.rs	96.32% <0.00%> (+0.95%)	⬆️
lightning/src/routing/router.rs	97.05% <0.00%> (+1.11%)	⬆️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4368b56...8275698. Read the comment docs.

[ariard]

Concept ACK.

This is a far cleaner approach than #725, as the need-to-include-anchor detection is wrapped inside the CommitmentTransaction interface.

[TheBlueMatt]

Concept ACK. It would be nice at least have some kind of test or something for this code.

[devrandom]

It would be nice at least have some kind of test or something for this code.

Done. But this is the first unit test for CommitmentTransaction - not sure if we want to have something more extensive.

[TheBlueMatt]

Done. But this is the first unit test for CommitmentTransaction - not sure if we want to have something more extensive.

I think its fine enough, would be good to check the output scripts but it doesn't matter hugely and once we ship anchor we'll have more extensive tests of that anyway.

[ariard]

Otherwise good to me!

[ariard]

I think there is a missing test case when we have !htlcs_with_aux.is_empty() ?

diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index 20734f95..e226dc84 100644
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -1268,7 +1268,8 @@ mod tests {
        use super::CounterpartyCommitmentSecrets;
        use ::{hex, chain};
        use prelude::*;
-       use ln::chan_utils::{CommitmentTransaction, TxCreationKeys, ChannelTransactionParameters, CounterpartyChannelTransactionParameters};
+       use ln::PaymentHash;
+       use ln::chan_utils::{CommitmentTransaction, TxCreationKeys, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, HTLCOutputInCommitment};
        use bitcoin::secp256k1::{PublicKey, SecretKey, Secp256k1};
        use util::test_utils;
        use chain::keysinterface::{KeysInterface, BaseSign};
@@ -1298,6 +1299,14 @@ mod tests {
                        funding_outpoint: Some(chain::transaction::OutPoint { txid: Default::default(), index: 0 })
                };
 
+               let payment_hash = PaymentHash([42; 32]);
+               let htlc_info = HTLCOutputInCommitment {
+                       offered: false,
+                       amount_msat: 1,
+                       cltv_expiry: 100,
+                       payment_hash,
+                       transaction_output_index: Some(3),
+               };
                let mut htlcs_with_aux: Vec<(_, ())> = Vec::new();
 
                let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
@@ -1337,6 +1346,15 @@ mod tests {
                        &mut htlcs_with_aux, &channel_parameters.as_holder_broadcastable()
                );
                assert_eq!(tx.built.transaction.output.len(), 2);
+               let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
+                       0, 3000, 0,
+                       true,
+                       holder_pubkeys.funding_pubkey,
+                       counterparty_pubkeys.funding_pubkey,
+                       keys.clone(), 1,
+                       &mut vec![(htlc_info, ())], &channel_parameters.as_holder_broadcastable()
+               );
+               assert_eq!(tx.built.transaction.output.len(), 4);
        }
 
        #[test]

[devrandom]

Good point. Slightly modified the test to generate a non-dust HTLC output (in case we start sanity checking that in the future) and pass None for the HTLC tx index (it should be populated by the commitment tx constructor, but we don't check if already set).

[ariard]

Code Review ACK 8275698