Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 22, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e7678d4
derive(Debug) on Events
TheBlueMatt Sep 18, 2020
07a0ad7
Clean up static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx
TheBlueMatt Sep 18, 2020
ebb8d2d
Clean up static_spendable_outputs_justice_tx_revoked_htlc_success_tx
TheBlueMatt Sep 18, 2020
9c1c432
Clean up test_bump_penalty_txn_on_revoked_htlcs
TheBlueMatt Sep 18, 2020
38dacf1
Do not test any block-contents-rescan cases
TheBlueMatt Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion lightning/src/chain/chaininterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,10 +377,22 @@ impl ChainWatchInterface for ChainWatchInterfaceUtil {

fn filter_block(&self, block: &Block) -> Vec<usize> {
let mut matched_index = Vec::new();
let mut matched_txids = HashSet::new();
{
let watched = self.watched.lock().unwrap();
for (index, transaction) in block.txdata.iter().enumerate() {
if self.does_match_tx_unguarded(transaction, &watched) {
// A tx matches the filter if it either matches the filter directly (via
// does_match_tx_unguarded) or if it is a descendant of another matched
// transaction within the same block, which we check for in the loop.
let mut matched = self.does_match_tx_unguarded(transaction, &watched);
for input in transaction.input.iter() {
if matched || matched_txids.contains(&input.previous_output.txid) {
matched = true;
break;
}
}
if matched {
matched_txids.insert(transaction.txid());
matched_index.push(index);
}
}
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/chain/keysinterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use ln::msgs::DecodeError;
/// spend on-chain. The information needed to do this is provided in this enum, including the
/// outpoint describing which txid and output index is available, the full output which exists at
/// that txid/index, and any keys or other information required to sign.
#[derive(Clone, PartialEq)]
#[derive(Clone, Debug, PartialEq)]
pub enum SpendableOutputDescriptor {
/// An output to a script which was provided via KeysInterface, thus you should already know
/// how to spend it. No keys are provided as rust-lightning was never given any keys - only the
Expand Down
126 changes: 101 additions & 25 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5037,24 +5037,34 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() {
check_added_monitors!(nodes[1], 1);

let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
assert_eq!(node_txn.len(), 4); // ChannelMonitor: justice tx on revoked commitment, justice tx on revoked HTLC-timeout, adjusted justice tx, ChannelManager: local commitment tx
assert_eq!(node_txn[0].input.len(), 2);
check_spends!(node_txn[0], revoked_local_txn[0]);
check_spends!(node_txn[1], chan_1.3);
assert_eq!(node_txn.len(), 3); // ChannelMonitor: bogus justice tx, justice tx on revoked outputs, ChannelManager: local commitment tx
// The first transaction generated is bogus - it spends both outputs of revoked_local_txn[0]
// including the one already spent by revoked_htlc_txn[0]. That's OK, we'll spend with valid
// transactions next...
assert_eq!(node_txn[0].input.len(), 3);
check_spends!(node_txn[0], revoked_local_txn[0], revoked_htlc_txn[0]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


assert_eq!(node_txn[1].input.len(), 2);
check_spends!(node_txn[1], revoked_local_txn[0], revoked_htlc_txn[0]);
if node_txn[1].input[1].previous_output.txid == revoked_htlc_txn[0].txid() {
assert_ne!(node_txn[1].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output);
} else {
assert_eq!(node_txn[1].input[0].previous_output.txid, revoked_htlc_txn[0].txid());
assert_ne!(node_txn[1].input[1].previous_output, revoked_htlc_txn[0].input[0].previous_output);
}

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

let header_1 = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
nodes[1].block_notifier.block_connected(&Block { header: header_1, txdata: vec![node_txn[0].clone(), node_txn[2].clone()] }, 1);
nodes[1].block_notifier.block_connected(&Block { header: header_1, txdata: vec![node_txn[1].clone()] }, 1);
connect_blocks(&nodes[1].block_notifier, ANTI_REORG_DELAY - 1, 1, true, header.block_hash());

// Check B's ChannelMonitor was able to generate the right spendable output descriptor
let spend_txn = check_spendable_outputs!(nodes[1], 1, node_cfgs[1].keys_manager, 100000);
assert_eq!(spend_txn.len(), 2);
check_spends!(spend_txn[0], node_txn[0]);
check_spends!(spend_txn[1], node_txn[2]);
assert_eq!(spend_txn.len(), 1);
assert_eq!(spend_txn[0].input.len(), 1);
check_spends!(spend_txn[0], node_txn[1]);
}

#[test]
Expand All @@ -5072,6 +5082,9 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() {
assert_eq!(revoked_local_txn[0].input.len(), 1);
assert_eq!(revoked_local_txn[0].input[0].previous_output.txid, chan_1.3.txid());

// The to-be-revoked commitment tx should have one HTLC and one to_remote output
assert_eq!(revoked_local_txn[0].output.len(), 2);

claim_payment(&nodes[0], &vec!(&nodes[1])[..], payment_preimage, 3_000_000);

let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
Expand All @@ -5086,28 +5099,50 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() {
assert_eq!(revoked_htlc_txn[0].input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);

// Check that the unspent (of two) outputs on revoked_local_txn[0] is a P2WPKH:
let unspent_local_txn_output = revoked_htlc_txn[0].input[0].previous_output.vout as usize ^ 1;
assert_eq!(revoked_local_txn[0].output[unspent_local_txn_output].script_pubkey.len(), 2 + 20); // P2WPKH

// A will generate justice tx from B's revoked commitment/HTLC tx
nodes[0].block_notifier.block_connected(&Block { header, txdata: vec![revoked_local_txn[0].clone(), revoked_htlc_txn[0].clone()] }, 1);
check_closed_broadcast!(nodes[0], false);
check_added_monitors!(nodes[0], 1);

let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
assert_eq!(node_txn.len(), 3); // ChannelMonitor: justice tx on revoked commitment, justice tx on revoked HTLC-success, ChannelManager: local commitment tx
assert_eq!(node_txn[2].input.len(), 1);
check_spends!(node_txn[2], revoked_htlc_txn[0]);

// The first transaction generated is bogus - it spends both outputs of revoked_local_txn[0]
// including the one already spent by revoked_htlc_txn[0]. That's OK, we'll spend with valid
// transactions next...
assert_eq!(node_txn[0].input.len(), 2);
check_spends!(node_txn[0], revoked_local_txn[0], revoked_htlc_txn[0]);
if node_txn[0].input[1].previous_output.txid == revoked_htlc_txn[0].txid() {
assert_eq!(node_txn[0].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output);
} else {
assert_eq!(node_txn[0].input[0].previous_output.txid, revoked_htlc_txn[0].txid());
assert_eq!(node_txn[0].input[1].previous_output, revoked_htlc_txn[0].input[0].previous_output);
}

assert_eq!(node_txn[1].input.len(), 1);
check_spends!(node_txn[1], revoked_htlc_txn[0]);

check_spends!(node_txn[2], chan_1.3);

let header_1 = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
nodes[0].block_notifier.block_connected(&Block { header: header_1, txdata: vec![node_txn[0].clone(), node_txn[2].clone()] }, 1);
nodes[0].block_notifier.block_connected(&Block { header: header_1, txdata: vec![node_txn[1].clone()] }, 1);
connect_blocks(&nodes[0].block_notifier, ANTI_REORG_DELAY - 1, 1, true, header.block_hash());

// Note that nodes[0]'s tx_broadcaster is still locked, so if we get here the channelmonitor
// didn't try to generate any new transactions.

// Check A's ChannelMonitor was able to generate the right spendable output descriptor
let spend_txn = check_spendable_outputs!(nodes[0], 1, node_cfgs[0].keys_manager, 100000);
assert_eq!(spend_txn.len(), 5); // Duplicated SpendableOutput due to block rescan after revoked htlc output tracking
assert_eq!(spend_txn.len(), 3); // Duplicated SpendableOutput due to block rescan after revoked htlc output tracking
assert_eq!(spend_txn[0], spend_txn[1]);
assert_eq!(spend_txn[0], spend_txn[2]);
assert_eq!(spend_txn[0].input.len(), 1);
check_spends!(spend_txn[0], revoked_local_txn[0]); // spending to_remote output from revoked local tx
check_spends!(spend_txn[3], node_txn[0]); // spending justice tx output from revoked local tx htlc received output
check_spends!(spend_txn[4], node_txn[2]); // spending justice tx output on htlc success tx
assert_ne!(spend_txn[0].input[0].previous_output, revoked_htlc_txn[0].input[0].previous_output);
check_spends!(spend_txn[2], node_txn[1]); // spending justice tx output on the htlc success tx
}

#[test]
Expand Down Expand Up @@ -7757,54 +7792,95 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);
assert_eq!(revoked_htlc_txn[1].input.len(), 1);
assert_eq!(revoked_htlc_txn[1].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
assert_eq!(revoked_htlc_txn[1].output.len(), 1);
check_spends!(revoked_htlc_txn[1], revoked_local_txn[0]);
} else if revoked_htlc_txn[1].input[0].witness.last().unwrap().len() == ACCEPTED_HTLC_SCRIPT_WEIGHT {
assert_eq!(revoked_htlc_txn[1].input.len(), 1);
check_spends!(revoked_htlc_txn[1], revoked_local_txn[0]);
assert_eq!(revoked_htlc_txn[0].input.len(), 1);
assert_eq!(revoked_htlc_txn[0].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
assert_eq!(revoked_htlc_txn[0].output.len(), 1);
check_spends!(revoked_htlc_txn[0], revoked_local_txn[0]);
}

// Broadcast set of revoked txn on A
let header_128 = connect_blocks(&nodes[0].block_notifier, 128, 0, true, header.block_hash());
let header_128 = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
nodes[0].block_notifier.block_connected(&Block { header: header_128, txdata: vec![revoked_local_txn[0].clone()] }, 128);
expect_pending_htlcs_forwardable_ignore!(nodes[0]);

let header_129 = BlockHeader { version: 0x20000000, prev_blockhash: header_128, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
nodes[0].block_notifier.block_connected(&Block { header: header_129, txdata: vec![revoked_local_txn[0].clone(), revoked_htlc_txn[0].clone(), revoked_htlc_txn[1].clone()] }, 129);
let header_129 = BlockHeader { version: 0x20000000, prev_blockhash: header_128.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
nodes[0].block_notifier.block_connected(&Block { header: header_129, txdata: vec![revoked_htlc_txn[0].clone(), revoked_htlc_txn[1].clone()] }, 129);
let first;
let feerate_1;
let penalty_txn;
{
let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
assert_eq!(node_txn.len(), 5); // 3 penalty txn on revoked commitment tx + A commitment tx + 1 penalty tnx on revoked HTLC txn
// Verify claim tx are spending revoked HTLC txn

// node_txn 0-2 each spend a separate revoked output from revoked_local_txn[0]
// Note that node_txn[0] and node_txn[1] are bogus - they double spend the revoked_htlc_txn
// which are included in the same block (they are broadcasted because we scan the
// transactions linearly and generate claims as we go, they likely should be removed in the
// future).
assert_eq!(node_txn[0].input.len(), 1);
check_spends!(node_txn[0], revoked_local_txn[0]);
assert_eq!(node_txn[1].input.len(), 1);
check_spends!(node_txn[1], revoked_local_txn[0]);
assert_eq!(node_txn[2].input.len(), 1);
check_spends!(node_txn[2], revoked_local_txn[0]);

// Each of the three justice transactions claim a separate (single) output of the three
// available, which we check here:
assert_ne!(node_txn[0].input[0].previous_output, node_txn[1].input[0].previous_output);
assert_ne!(node_txn[0].input[0].previous_output, node_txn[2].input[0].previous_output);
assert_ne!(node_txn[1].input[0].previous_output, node_txn[2].input[0].previous_output);

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// reorgs, though its not clear its ever worth broadcasting conflicting txn like this when
// a remote commitment tx has already been confirmed).
check_spends!(node_txn[3], chan.3);

// node_txn[4] spends the revoked outputs from the revoked_htlc_txn (which only have one
// output, checked above).
assert_eq!(node_txn[4].input.len(), 2);
assert_eq!(node_txn[4].output.len(), 1);
check_spends!(node_txn[4], revoked_htlc_txn[0], revoked_htlc_txn[1]);

first = node_txn[4].txid();
// Store both feerates for later comparison
let fee_1 = revoked_htlc_txn[0].output[0].value + revoked_htlc_txn[1].output[0].value - node_txn[4].output[0].value;
feerate_1 = fee_1 * 1000 / node_txn[4].get_weight() as u64;
penalty_txn = vec![node_txn[0].clone(), node_txn[1].clone(), node_txn[2].clone()];
penalty_txn = vec![node_txn[2].clone()];
node_txn.clear();
}

// Connect three more block to see if bumped penalty are issued for HTLC txn
// Connect one more block to see if bumped penalty are issued for HTLC txn
let header_130 = BlockHeader { version: 0x20000000, prev_blockhash: header_129.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
nodes[0].block_notifier.block_connected(&Block { header: header_130, txdata: penalty_txn }, 130);
let header_131 = BlockHeader { version: 0x20000000, prev_blockhash: header_130.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
nodes[0].block_notifier.block_connected(&Block { header: header_131, txdata: Vec::new() }, 131);
{
let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
assert_eq!(node_txn.len(), 2); // 2 bumped penalty txn on revoked commitment tx

check_spends!(node_txn[0], revoked_local_txn[0]);
check_spends!(node_txn[1], revoked_local_txn[0]);
// Note that these are both bogus - they spend outputs already claimed in block 129:
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);
}
Comment on lines +7872 to +7877
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


node_txn.clear();
};

// Few more blocks to confirm penalty txn
let header_135 = connect_blocks(&nodes[0].block_notifier, 5, 130, true, header_130.block_hash());
let header_135 = connect_blocks(&nodes[0].block_notifier, 4, 131, true, header_131.block_hash());
assert!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().is_empty());
let header_144 = connect_blocks(&nodes[0].block_notifier, 9, 135, true, header_135);
let node_txn = {
Expand Down
1 change: 1 addition & 0 deletions lightning/src/util/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use std::time::Duration;
/// Note that while Writeable and Readable are implemented for Event, you probably shouldn't use
/// them directly as they don't round-trip exactly (for example FundingGenerationReady is never
/// written as it makes no sense to respond to it after reconnecting to peers).
#[derive(Debug)]
pub enum Event {
/// Used to indicate that the client should generate a funding transaction with the given
/// parameters and then call ChannelManager::funding_transaction_generated.
Expand Down