Add outpoint index in watch_outputs to fix tracking #653

ariard · 2020-07-01T18:15:10Z

Previously, outputs were monitored based on txid and an index yelled
from an enumeration over the returned selected outputs by monitoring
code. This is broken we don't have a guarantee that HTLC outputs are
ranking first after introduction of anchor outputs.

I think alternatively we can fix sorting in build_commitment_transaction to always order HTLCs first but sounds less robust to me.

TheBlueMatt · 2020-07-01T18:20:14Z

Can you elaborate on "don't have a guarantee that HTLC outputs are ranking first after introduction of anchor outputs."? Specifically, we should always know exactly what the list of outputs in a commitment transaction is, why can we not use that?

ariard · 2020-07-02T00:10:26Z

On "don't have a guarantee that HTLC outputs are ranking first after introduction of anchor outputs" it needs an amendment, I think we don't have previously guarantee that HTLC outputs were ranking first before to_local/to_remote as comparators are in order : value, script_pubkey, (timelocks), (hash). So this issue sounds to have been silently avoided by our test framework.

We always know the list but not their order and that matters to match by outpoint ?

TheBlueMatt · 2020-07-02T19:49:52Z

Right, but I don't see where the current code is making any assumptions about HTLC output ordering - watch_outputs seems to always be called with something like watch_outputs.append(&mut tx.output.clone()); which means enumerate() does the correct thing.

jkczyz · 2020-07-11T01:26:01Z

I ran across an issue today that looks to be resolved by this PR. Here we are pushing outputs to watch and later assume they are indexed by how they appear in the transaction.

@TheBlueMatt concurred that this fix is appropriate.

TheBlueMatt · 2020-07-11T21:50:47Z

Right, I think I realized this was actually right (and we get it wrong in a few places), but forgot to comment here. In any case, this really needs a robust test to ensure we never hit such an error in the future - our test chain monitoring code should refuse to match things that don't have the correct output index.

TheBlueMatt · 2020-09-27T01:24:30Z

Is this fixed in #649 or do we need to rebase this on top of it/ whats the status here?

ariard · 2020-09-29T17:22:01Z

@TheBlueMatt @jkczyz I'll rebase this on top of #649. Without I've test breakage on my anchor branch, but surely needs it own test coverage.

This test is a mutation to underscore the detetection logic bug we had before lightningdevkit#653. HTLC value routed is above the remaining balance, thus inverting HTLC and `to_remote` output. HTLC will come second and it wouldn't be seen by pre-lightningdevkit#653 detection as we were eneumerate()'ing on a watched outputs vector (Vec<TxOut>) thus implictly relying on outputs order detection for correct spending children filtering.

ariard · 2020-10-06T23:49:16Z

@TheBlueMatt @jkczyz Thanks for review finally updated at 80c0e8c, see commit messages for explaining the bug. Or IRC conv of 10/06/2020.

codecov · 2020-10-07T00:34:08Z

Codecov Report

Merging #653 into master will decrease coverage by 0.04%.
The diff coverage is 95.52%.

@@            Coverage Diff             @@
##           master     #653      +/-   ##
==========================================
- Coverage   91.39%   91.35%   -0.05%     
==========================================
  Files          37       37              
  Lines       21964    21974      +10     
==========================================
  Hits        20074    20074              
- Misses       1890     1900      +10

Impacted Files	Coverage Δ
lightning/src/chain/channelmonitor.rs	`95.52% <91.42%> (-0.20%)`	⬇️
lightning/src/chain/chainmonitor.rs	`97.10% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`96.98% <100.00%> (-0.13%)`	⬇️

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 df778b6...27ee115. Read the comment docs.

TheBlueMatt

I'm a little confused why something like this doesn't catch the bug, even on your new test:

@@ -1811,10 +1811,32 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
 
        /// Checks if a given transaction spends any watched outputs.
        fn spends_watched_output(&self, tx: &Transaction) -> bool {
+               #[cfg(test)]
+               {
+                       // If we see a transaction which we registered previously, make sure the registration
+                       // matches the actual transaction.
+                       if let Some(outputs) = self.get_outputs_to_watch().get(&tx.txid()) {
+                               for (idx, script_pubkey) in outputs.iter().enumerate() {
+                                       assert!(idx < tx.output.len());
+                                       assert_eq!(tx.output[idx].script_pubkey, *script_pubkey);
+                               }
+                       }
+               }
                for input in tx.input.iter() {
                        if let Some(outputs) = self.get_outputs_to_watch().get(&input.previous_output.txid) {
                                for (idx, _script_pubkey) in outputs.iter().enumerate() {
                                        if idx == input.previous_output.vout as usize {
+                                               #[cfg(test)]
+                                               {
+                                                       // If the expected script is a known type, check that the witness
+                                                       // appears to be spending the correct type (ie that the match would
+                                                       // actually succeed in BIP 158/159-style filters).
+                                                       if _script_pubkey.is_v0_p2wsh() {
+                                                               assert_eq!(&bitcoin::Address::p2wsh(&Script::from(input.witness.last().unwrap().clone()), bitcoin::Network::Bitcoin).script_pubkey(), _script_pubkey);
+                                                       } else if _script_pubkey.is_v0_p2wpkh() {
+                                                               assert_eq!(&bitcoin::Address::p2wpkh(&bitcoin::PublicKey::from_slice(&input.witness.last().unwrap()).unwrap(), bitcoin::Network::Bitcoin).unwrap().script_pubkey(), _script_pubkey);
+                                                       }
+                                               }
                                                return true;
                                        }
                                }

lightning/src/ln/functional_tests.rs

This test is a mutation to underscore the detetection logic bug we had before lightningdevkit#653. HTLC value routed is above the remaining balance, thus inverting HTLC and `to_remote` output. HTLC will come second and it wouldn't be seen by pre-lightningdevkit#653 detection as we were eneumerate()'ing on a watched outputs vector (Vec<TxOut>) thus implictly relying on outputs order detection for correct spending children filtering.

ariard · 2020-10-07T23:24:37Z

I'm a little confused why something like this doesn't catch the bug, even on your new test:

What did you observe ? I tested your diff on master with new test and effectively it's failing as the index as yelled by the iterator enumeration isn't the real index at which the output should be watched and filtered.

You just have watched_outputs.len() < commitment_tx.output.len()

TheBlueMatt · 2020-10-08T18:46:22Z

What did you observe ?

It looked to me like your new test was failing at the assertion at the end both with and without the above diff, not ever hitting the new assertions, did I do something wrong?

This test is a mutation to underscore the detetection logic bug we had before lightningdevkit#653. HTLC value routed is above the remaining balance, thus inverting HTLC and `to_remote` output. HTLC will come second and it wouldn't be seen by pre-lightningdevkit#653 detection as we were eneumerate()'ing on a watched outputs vector (Vec<TxOut>) thus implictly relying on outputs order detection for correct spending children filtering.

TheBlueMatt

I'd still like to keep the second part of the new assertions. While it doesn't hit here because we're spending something which didn't get registered, I could see us screwing up and registering a script wrong in the future without having the transaction in the current matched set.

                        if let Some(outputs) = self.get_outputs_to_watch().get(&input.previous_output.txid) {
                                for (idx, _script_pubkey) in outputs.iter().enumerate() {
                                        if idx == input.previous_output.vout as usize {
+                                               #[cfg(test)]
+                                               {
+                                                       // If the expected script is a known type, check that the witness
+                                                       // appears to be spending the correct type (ie that the match would
+                                                       // actually succeed in BIP 158/159-style filters).
+                                                       if _script_pubkey.is_v0_p2wsh() {
+                                                               assert_eq!(&bitcoin::Address::p2wsh(&Script::from(input.witness.last().unwrap().clone()), bitcoin::Network::Bitcoin).script_pubkey(), _script_pubkey);
+                                                       } else if _script_pubkey.is_v0_p2wpkh() {
+                                                               assert_eq!(&bitcoin::Address::p2wpkh(&bitcoin::PublicKey::from_slice(&input.witness.last().unwrap()).unwrap(), bitcoin::Network::Bitcoin).unwrap().script_pubkey(), _script_pubkey);
+                                                       } else { panic!(); }
+                                               }
                                                return true;
                                        }
                                }

lightning/src/chain/channelmonitor.rs

Previously, outputs were monitored based on txid and an index yelled from an enumeration over the returned selected outputs by monitoring code. This is always have been broken but was only discovered while introducing anchor outputs as those ones rank always first per BIP69. We didn't have test cases where a HTLC was bigger than a party balance on a holder commitment and thus not ranking first. Next commit introduce test coverage.

This test is a mutation to underscore the detetection logic bug we had before lightningdevkit#653. HTLC value routed is above the remaining balance, thus inverting HTLC and `to_remote` output. HTLC will come second and it wouldn't be seen by pre-lightningdevkit#653 detection as we were eneumerate()'ing on a watched outputs vector (Vec<TxOut>) thus implictly relying on outputs order detection for correct spending children filtering.

ariard · 2020-10-10T23:28:13Z

Updated at 324edf1

See modification of your supplementary diff and caveat comment to keep passing test_no_failure_dust_htlc_local_commitment, which is intentionally throwing junk in monitoring code to test robustness.

TheBlueMatt · 2020-10-11T03:03:22Z

lightning/src/chain/channelmonitor.rs

+					if *idx == input.previous_output.vout {
+						#[cfg(test)]
+						{
+							// If the witness is empty this transaction is a dummy one expressely


Can we just...drop that test and panic instead? What was the rationale behind connecting garbage that should only ever be an indication the user is being duped by a bogus chain source (which is explicitly not in our threat model, at least not yet).

Tested was added in #333, but can't find the rational. If I remember loosely, at some point we had bug in our dust HTLC canceling back logic at commitment transaction confirmation. Mutating with the following doesn't break the test so I presume it was an oversight as it doesn't actually cover anything. Removed.

Diff:

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 3af98121..927d9d70 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -1418,12 +1418,12 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> { } } } - if let Some(ref txid) = self.current_counterparty_commitment_txid { - check_htlc_fails!(txid, "current", 'current_loop); - } - if let Some(ref txid) = self.prev_counterparty_commitment_txid { - check_htlc_fails!(txid, "previous", 'prev_loop); - } + //if let Some(ref txid) = self.current_counterparty_commitment_txid { + // check_htlc_fails!(txid, "current", 'current_loop); + //} + //if let Some(ref txid) = self.prev_counterparty_commitment_txid { + // check_htlc_fails!(txid, "previous", 'prev_loop); + //} if let Some(revocation_points) = self.their_cur_revocation_points { let revocation_point_option =

I think this test was added in case of future changes of the monitoring code (check_spend_counterparty) which may have broken the no-dust-HTLC-canceling-back.

We remove test_no_failure_dust_htlc_local_commitment from our test framework as this test deliberately throwing junk transaction in our monitoring parsing code is hitting new assertions. This test was added in lightningdevkit#333, but it sounds as an oversight as the correctness intention of this test (i.e verifying lack of dust HTLCs canceling back in case of junk commitment transaction) doesn't currently break.

TheBlueMatt · 2020-10-15T00:42:31Z

lightning/src/chain/channelmonitor.rs

-			let output_scripts = txouts.iter().map(|o| o.script_pubkey.clone()).collect();
-			self.outputs_to_watch.insert(txid.clone(), output_scripts).is_none()
+			let idx_and_scripts = txouts.iter().map(|o| (o.0, o.1.script_pubkey.clone())).collect();
+			self.outputs_to_watch.insert(txid.clone(), idx_and_scripts).is_none()


Can we iterate the new watch txn to assert they're known types so that the panic!() two hunks down is definitely correct?

Actually, its all test-only, it doesnt matter.

TheBlueMatt

One comment, otherwise ACK.

TheBlueMatt added this to the 0.0.12 milestone Sep 27, 2020

TheBlueMatt mentioned this pull request Sep 29, 2020

Drop the redundant/broken ChannelMonitor::get_monitored_outpoints #722

Merged

ariard force-pushed the 2020-06-fix-outputs-tracking branch from e9a0ab2 to 80c0e8c Compare October 6, 2020 23:46

TheBlueMatt reviewed Oct 7, 2020

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

ariard force-pushed the 2020-06-fix-outputs-tracking branch from 80c0e8c to 0abec48 Compare October 7, 2020 23:11

ariard force-pushed the 2020-06-fix-outputs-tracking branch from 0abec48 to 524e842 Compare October 9, 2020 01:26

TheBlueMatt reviewed Oct 9, 2020

View reviewed changes

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

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

Antoine Riard added 2 commits October 10, 2020 18:51

ariard force-pushed the 2020-06-fix-outputs-tracking branch from 524e842 to 324edf1 Compare October 10, 2020 23:26

TheBlueMatt reviewed Oct 11, 2020

View reviewed changes

ariard force-pushed the 2020-06-fix-outputs-tracking branch from 324edf1 to 27ee115 Compare October 14, 2020 13:28

TheBlueMatt reviewed Oct 15, 2020

View reviewed changes

TheBlueMatt approved these changes Oct 15, 2020

View reviewed changes

TheBlueMatt merged commit 8a79877 into lightningdevkit:master Oct 15, 2020

Add outpoint index in watch_outputs to fix tracking #653

Add outpoint index in watch_outputs to fix tracking #653

Uh oh!

Conversation

ariard commented Jul 1, 2020

Uh oh!

TheBlueMatt commented Jul 1, 2020

Uh oh!

ariard commented Jul 2, 2020

Uh oh!

TheBlueMatt commented Jul 2, 2020

Uh oh!

jkczyz commented Jul 11, 2020

Uh oh!

TheBlueMatt commented Jul 11, 2020

Uh oh!

TheBlueMatt commented Sep 27, 2020

Uh oh!

ariard commented Sep 29, 2020

Uh oh!

ariard commented Oct 6, 2020

Uh oh!

codecov bot commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ariard commented Oct 7, 2020

Uh oh!

TheBlueMatt commented Oct 8, 2020

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ariard commented Oct 10, 2020

Uh oh!

TheBlueMatt Oct 11, 2020

Choose a reason for hiding this comment

Uh oh!

ariard Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

ariard Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Oct 7, 2020 •

edited

Loading