Do not test any block-contents-rescan cases #715

TheBlueMatt · 2020-09-18T22:31:32Z

This is in preparation for #649 removing support for block rescan wholesale, fixing the tests which rely on it and are overly-specific. It doesn't really address the fact that the tests are over-specific (though does address a few cases where we're confirming double-spends in blocks in tests), but should at least make it so that #649 can make progress.

We'd previously largely not turned on derive(Debug) on any of our structs, but not for good reason. Especially for Events objects, Debug can be a very useful for users to quickly print what they received from us without having to write out a large match.

Add a few comments to make it clear whats going on a bit more and don't test/confirm a bogus transaction.

TheBlueMatt · 2020-09-18T22:36:26Z

Note that this does not update any docs (though that is absolutely necessary for release), its presumed that 649 will rewrite most of the docs anyway, so no need to update them here.

codecov · 2020-09-18T23:11:22Z

Codecov Report

Merging #715 into main will increase coverage by 0.08%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##             main     #715      +/-   ##
==========================================
+ Coverage   91.91%   92.00%   +0.08%     
==========================================
  Files          36       36              
  Lines       20161    20200      +39     
==========================================
+ Hits        18531    18585      +54     
+ Misses       1630     1615      -15

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`27.58% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.13% <98.24%> (+0.18%)`	⬆️
lightning/src/chain/chaininterface.rs	`93.29% <100.00%> (+0.19%)`	⬆️
lightning/src/chain/keysinterface.rs	`96.20% <100.00%> (ø)`
lightning/src/ln/channelmonitor.rs	`95.51% <0.00%> (+0.51%)`	⬆️

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 640ae63...38dacf1. Read the comment docs.

lightning/src/ln/functional_tests.rs

Add a few comments to make it clear whats going on a bit more and don't test/confirm a double-spend transaction.

ariard

I'm leaning about pushing this PR further and remove bogus transactions generation. We may have a helper running before OnchainTxHandler::block_connected sanitizing disjunctive outpoints, like sanitize_outpoints(txn_matched, claimable_outpoints)

lightning/src/ln/functional_tests.rs

ariard · 2020-09-21T14:25:48Z

lightning/src/ln/functional_tests.rs

+		assert_eq!(node_txn[0].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output);
+		assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output);
+
+		// node_txn[3] is the local commitment tx broadcast just because (and somewhat in case of


"Just because of remote force-closing". But I think this local broadcast isn't really worthy as this transaction is double-spending a already confirmed and will never propagate.

I think that future work should remove this behavior and we should only broadcast holder commitment when a) a off-chain condition is detected, or b) holder request, or c) HTLC to claim on-chain. But not in reaction to confirmation of counterparty commitment.

Right, sure, there's definitely room to improve things, but at least for the local-commitment copy we'd need to store it somewhere for broadcast-on-reorg, which we don't currently do.

ariard · 2020-09-21T14:49:20Z

lightning/src/chain/chaininterface.rs

@@ -377,10 +377,19 @@ impl ChainWatchInterface for ChainWatchInterfaceUtil {

 	fn filter_block(&self, block: &Block) -> Vec<usize> {
 		let mut matched_index = Vec::new();
+		let mut match_set = HashSet::new();


I don't know how long this function will live but in the meanwhile IMO it's clearer if we call this variable descendants_matched as it underscores better that those transactions are captured in reason of their parents succeeding does_match_tx_unguarded.

It would depend on the context. That naming would apply to inserting into the set, but with membership checks "ancestor" rather than "descendant" would be an accurate naming.

Lets not bikeshed a name that will be removed asap.

ariard · 2020-09-21T14:50:54Z

lightning/src/chain/chaininterface.rs

-				if self.does_match_tx_unguarded(transaction, &watched) {
+				let mut matched = self.does_match_tx_unguarded(transaction, &watched);
+				for input in transaction.input.iter() {
+					if matched || match_set.contains(&input.previous_output.txid) {


If you want to break early from iteration on a already-matched transaction, scope code in a conditional if !matched {}, it would make code clearer.

I added a comment, hopefully that's clear, but its also nice to break when we hit the contains case too :)

lightning/src/ln/functional_tests.rs

ariard · 2020-09-21T15:27:21Z

lightning/src/ln/functional_tests.rs

-	assert_eq!(node_txn[0].input.len(), 2);
-	check_spends!(node_txn[0], revoked_local_txn[0]);
+	assert_eq!(node_txn[0].input.len(), 3);
+	check_spends!(node_txn[0], revoked_local_txn[0], revoked_htlc_txn[0]);


I observed the logs and here the new behavior:

detection of revoked commitment and revoked htlc-timeout by ChannelMonitor

aggregation of revoked outputs in one claim tx by OnchainTxHandler, broadcast of node_txn[0]

detection of revoked htlc-timeout which double-spend a registered claim by OnchainTxHandler, broadcast of node_txn[1]

feerate of node_txn[1] is wrongly bumped as we don't dissociate clearly the reschedule source either double-spend or internal block timer expiration

I think either ChannelMonitor or OnchainTxHandler should sanitize claimable outpoints and ensure there is no disjunctive ones.

Is there something you think should be changed here? I agree eventually we should be more careful about the txn we generate, but we should be pretty confident when we do that - I'd rather air on the side of over-broadcasting invalid crap than ever miss a claim, of course, and this PR is a bit more time-critical as its blocking 649.

There is currently an overpaid fee bug due to this behavior which should be fixed. But I'm also thinking we should rework our aggregation logic to avoid exploitation. Let's address this further.

TheBlueMatt · 2020-09-21T16:00:02Z

I'm leaning about pushing this PR further and remove bogus transactions generation.

Hmm, that's a lot of work (and, as you pointed out on IRC, in many cases not practical at all), and seems somewhat orthogonal to fixing tests. There is some generation that is obviously a double-spend, but for the most part we just generate a lot of transactions which are theoretically valid in a reorg case.

Add a few comments to make it clear whats going on a bit more and assert the basic structure of more transactions. Further, point out where transactions are double-spends and don't confirm double-spends.

In anticipation for removing support for users calling block_connected multiple times for the same block to include all relevant transactions in the next PR, this commit stops testing such cases. Specifically, users who filter blocks for relevant transactions before calling block_connected will need to filter by including any transactions which spend a previously-matched transaction in the same block (and we now do so in our own filtering logic, which is also used in our testing).

jkczyz

Overall LGTM

jkczyz · 2020-09-21T23:13:12Z

lightning/src/ln/functional_tests.rs

+		if node_txn[0].input[0].previous_output == revoked_htlc_txn[0].input[0].previous_output  {
+			assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output);
+		} else {
+			assert_eq!(node_txn[0].input[0].previous_output, revoked_htlc_txn[1].input[0].previous_output);
+			assert_eq!(node_txn[1].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output);
+		}


For my own understanding, is there some non-determinism that can't be eliminated from how transactions are broadcast? Given these are accumulated in a vector (at least in TestBroadcaster), could they be sorted in some manner prior to making these asserts?

Honestly, I didn't dig too far into it, just noted that it broke the test non-deterministically and fixed it up.

ariard · 2020-09-22T18:17:08Z

Code Review ACK 38dacf1

TheBlueMatt added 2 commits September 18, 2020 18:29

derive(Debug) on Events

e7678d4

We'd previously largely not turned on derive(Debug) on any of our structs, but not for good reason. Especially for Events objects, Debug can be a very useful for users to quickly print what they received from us without having to write out a large match.

Clean up static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx

07a0ad7

Add a few comments to make it clear whats going on a bit more and don't test/confirm a bogus transaction.

jkczyz self-requested a review September 18, 2020 22:32

jkczyz reviewed Sep 19, 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

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

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

Clean up static_spendable_outputs_justice_tx_revoked_htlc_success_tx

ebb8d2d

Add a few comments to make it clear whats going on a bit more and don't test/confirm a double-spend transaction.

TheBlueMatt force-pushed the 2020-09-no-rescan-tests branch from b8d0a7f to b63ab7d Compare September 20, 2020 21:27

ariard reviewed Sep 21, 2020

View reviewed changes

TheBlueMatt added 2 commits September 21, 2020 13:34

Clean up test_bump_penalty_txn_on_revoked_htlcs

9c1c432

Add a few comments to make it clear whats going on a bit more and assert the basic structure of more transactions. Further, point out where transactions are double-spends and don't confirm double-spends.

TheBlueMatt force-pushed the 2020-09-no-rescan-tests branch from b63ab7d to 38dacf1 Compare September 21, 2020 17:34

ariard added High Review Priority Moderate Review Difficulty labels Sep 21, 2020

jkczyz approved these changes Sep 21, 2020

View reviewed changes

TheBlueMatt merged commit 8640173 into lightningdevkit:main Sep 22, 2020

TheBlueMatt mentioned this pull request Sep 22, 2020

Refactor chain monitoring #649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not test any block-contents-rescan cases #715

Do not test any block-contents-rescan cases #715

TheBlueMatt commented Sep 18, 2020

TheBlueMatt commented Sep 18, 2020 •

edited

Loading

codecov bot commented Sep 18, 2020 •

edited

Loading

ariard left a comment

ariard Sep 21, 2020

TheBlueMatt Sep 21, 2020

ariard Sep 21, 2020

jkczyz Sep 21, 2020

TheBlueMatt Sep 22, 2020

ariard Sep 21, 2020

TheBlueMatt Sep 21, 2020

ariard Sep 21, 2020

TheBlueMatt Sep 21, 2020

ariard Sep 22, 2020

TheBlueMatt commented Sep 21, 2020

jkczyz left a comment

jkczyz Sep 21, 2020

TheBlueMatt Sep 22, 2020

ariard commented Sep 22, 2020

Do not test any block-contents-rescan cases #715

Do not test any block-contents-rescan cases #715

Conversation

TheBlueMatt commented Sep 18, 2020

TheBlueMatt commented Sep 18, 2020 • edited Loading

codecov bot commented Sep 18, 2020 • edited Loading

Codecov Report

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 21, 2020

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Sep 22, 2020

TheBlueMatt commented Sep 18, 2020 •

edited

Loading

codecov bot commented Sep 18, 2020 •

edited

Loading