Add outpoint index in watch_outputs to fix tracking

[ariard]

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]

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]

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]

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]

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]

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]

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

[ariard]

@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.

[ariard]

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

[codecov]

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;
}
}

[ariard]

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]

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?

[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;
}
}

[ariard]

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]

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).

[ariard]

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 =

[ariard]

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.

[TheBlueMatt]

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

[TheBlueMatt]

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

[TheBlueMatt]

One comment, otherwise ACK.

[ariard]

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]

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]

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]

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]

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]

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]

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

[ariard]

@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.

[ariard]

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

[codecov]

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;
}
}

[ariard]

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]

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?

[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;
}
}

[ariard]

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]

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).

[ariard]

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 =

[ariard]

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.

[TheBlueMatt]

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

[TheBlueMatt]

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

[TheBlueMatt]

One comment, otherwise ACK.

[ariard]

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]

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]

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]

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]

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]

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]

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

[ariard]

@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.

[ariard]

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

[codecov]

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;
}
}

[ariard]

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]

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?

[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;
}
}

[ariard]

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]

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).

[ariard]

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 =

[ariard]

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.

[TheBlueMatt]

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

[TheBlueMatt]

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

[TheBlueMatt]

One comment, otherwise ACK.

[ariard]

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]

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]

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]

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]

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]

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]

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

[ariard]

@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.

[ariard]

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

[codecov]

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;
}
}

[ariard]

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]

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?

[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;
}
}

[ariard]

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]

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).

[ariard]

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 =

[ariard]

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.

[TheBlueMatt]

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

[TheBlueMatt]

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

[TheBlueMatt]

One comment, otherwise ACK.

[ariard]

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]

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]

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]

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]

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]

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]

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

[ariard]

@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.

[ariard]

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

[codecov]

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;
}
}

[ariard]

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]

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?

[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;
}
}

[ariard]

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]

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).

[ariard]

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 =

[ariard]

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.

[TheBlueMatt]

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

[TheBlueMatt]

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

[TheBlueMatt]

One comment, otherwise ACK.

[ariard]

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]

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]

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]

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]

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]

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]

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

[ariard]

@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.

[ariard]

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

[codecov]

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;
}
}

[ariard]

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]

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?

[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;
}
}

[ariard]

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]

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).

[ariard]

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 =

[ariard]

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.

[TheBlueMatt]

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

[TheBlueMatt]

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

[TheBlueMatt]

One comment, otherwise ACK.

[ariard]

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]

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]

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]

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]

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]

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]

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

[ariard]

@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.

[ariard]

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

[codecov]

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;
}
}

[ariard]

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]

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?

[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;
}
}

[ariard]

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]

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).

[ariard]

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 =

[ariard]

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.

[TheBlueMatt]

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

[TheBlueMatt]

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

[TheBlueMatt]

One comment, otherwise ACK.

[ariard]

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]

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]

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]

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]

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]

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]

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

[ariard]

@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.

[ariard]

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

[codecov]

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;
}
}

[ariard]

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]

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?

[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;
}
}

[ariard]

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]

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).

[ariard]

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 =

[ariard]

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.

[TheBlueMatt]

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

[TheBlueMatt]

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

[TheBlueMatt]

One comment, otherwise ACK.