Skip to content

Add assertions for in-order block [dis]connection in ChannelManager #831

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

Closed
wants to merge 1 commit into from
Closed

Add assertions for in-order block [dis]connection in ChannelManager #831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,8 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {

macro_rules! confirm_txn {
($node: expr) => { {
let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
let chain_hash = genesis_block(Network::Bitcoin).block_hash();
let mut header = BlockHeader { version: 0x20000000, prev_blockhash: chain_hash, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
let txdata: Vec<_> = channel_txn.iter().enumerate().map(|(i, tx)| (i + 1, tx)).collect();
$node.block_connected(&header, &txdata, 1);
for i in 2..100 {
Expand Down
14 changes: 7 additions & 7 deletions fuzz/src/full_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ struct MoneyLossDetector<'a> {
peers: &'a RefCell<[bool; 256]>,
funding_txn: Vec<Transaction>,
txids_confirmed: HashMap<Txid, usize>,
header_hashes: Vec<BlockHash>,
header_hashes: Vec<(BlockHash, u32)>,
height: usize,
max_height: usize,
blocks_connected: u32,
Expand All @@ -179,7 +179,7 @@ impl<'a> MoneyLossDetector<'a> {
peers,
funding_txn: Vec::new(),
txids_confirmed: HashMap::new(),
header_hashes: vec![Default::default()],
header_hashes: vec![(genesis_block(Network::Bitcoin).block_hash(), 0)],
height: 0,
max_height: 0,
blocks_connected: 0,
Expand All @@ -199,23 +199,23 @@ impl<'a> MoneyLossDetector<'a> {
}
}

let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height], merkle_root: Default::default(), time: self.blocks_connected, bits: 42, nonce: 42 };
self.height += 1;
self.blocks_connected += 1;
let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height].0, merkle_root: Default::default(), time: self.blocks_connected, bits: 42, nonce: 42 };
self.height += 1;
self.manager.block_connected(&header, &txdata, self.height as u32);
(*self.monitor).block_connected(&header, &txdata, self.height as u32);
if self.header_hashes.len() > self.height {
self.header_hashes[self.height] = header.block_hash();
self.header_hashes[self.height] = (header.block_hash(), self.blocks_connected);
} else {
assert_eq!(self.header_hashes.len(), self.height);
self.header_hashes.push(header.block_hash());
self.header_hashes.push((header.block_hash(), self.blocks_connected));
}
self.max_height = cmp::max(self.height, self.max_height);
}

fn disconnect_block(&mut self) {
if self.height > 0 && (self.max_height < 6 || self.height >= self.max_height - 6) {
let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height], merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height - 1].0, merkle_root: Default::default(), time: self.header_hashes[self.height].1, bits: 42, nonce: 42 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing! Was the time here the problem? Wondering why it was fine as a constant prior to the change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It has to match what was used in connect_block (as otherwise the hashes wont match), so we have to store it somehow. We both had an off-by-one in the prev_blockhash lookup and we didn't have a way to make time match.

self.manager.block_disconnected(&header);
self.monitor.block_disconnected(&header, self.height as u32);
self.height -= 1;
Expand Down
4 changes: 2 additions & 2 deletions lightning-persister/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,8 @@ mod tests {
let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
assert_eq!(node_txn.len(), 1);

let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
connect_block(&nodes[1], &Block { header, txdata: vec![node_txn[0].clone(), node_txn[0].clone()]}, 1);
let header = BlockHeader { version: 0x20000000, prev_blockhash: *nodes[0].last_block_hash.lock().unwrap(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
connect_block(&nodes[1], &Block { header, txdata: vec![node_txn[0].clone(), node_txn[0].clone()]}, CHAN_CONFIRM_DEPTH);
check_closed_broadcast!(nodes[1], false);
check_added_monitors!(nodes[1], 1);

Expand Down
12 changes: 12 additions & 0 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3260,6 +3260,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana

let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);


// This assertion should be enforced in tests, however we have a number of tests that
// were written before this requirement and do not meet it.
#[cfg(not(test))]
{
assert_eq!(*self.last_block_hash.read().unwrap(), header.prev_blockhash,
"Blocks must be connected in chain-order - the connected header must build on the last connected header");
assert_eq!(self.latest_block_height.load(Ordering::Acquire) as u64, height as u64 - 1,
"Blocks must be connected in chain-order - the connected header must build on the last connected header");
}
self.latest_block_height.store(height as usize, Ordering::Release);
*self.last_block_hash.write().unwrap() = block_hash;

Expand Down Expand Up @@ -3376,6 +3386,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
// See the docs for `ChannelManagerReadArgs` for more.
let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);

assert_eq!(*self.last_block_hash.read().unwrap(), header.block_hash(),
"Blocks must be disconnected in chain-order - the disconnected header must be the last connected header");
self.latest_block_height.fetch_sub(1, Ordering::AcqRel);
*self.last_block_hash.write().unwrap() = header.prev_blockhash;

Expand Down
6 changes: 5 additions & 1 deletion lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub fn confirm_transaction<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, tx: &Tran
let dummy_tx = Transaction { version: 0, lock_time: 0, input: Vec::new(), output: Vec::new() };
let dummy_tx_count = tx.version as usize;
let mut block = Block {
header: BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
header: BlockHeader { version: 0x20000000, prev_blockhash: genesis_block(Network::Testnet).header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
txdata: vec![dummy_tx; dummy_tx_count],
};
block.txdata.push(tx.clone());
Expand Down Expand Up @@ -85,12 +85,14 @@ pub fn connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: &Block,
node.chain_monitor.chain_monitor.block_connected(&block.header, &txdata, height);
node.node.block_connected(&block.header, &txdata, height);
node.node.test_process_background_events();
*node.last_block_hash.lock().unwrap() = block.header.block_hash();
}

pub fn disconnect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, header: &BlockHeader, height: u32) {
node.chain_monitor.chain_monitor.block_disconnected(header, height);
node.node.block_disconnected(header);
node.node.test_process_background_events();
*node.last_block_hash.lock().unwrap() = header.prev_blockhash;
}

pub struct TestChanMonCfg {
Expand Down Expand Up @@ -123,6 +125,7 @@ pub struct Node<'a, 'b: 'a, 'c: 'b> {
pub network_payment_count: Rc<RefCell<u8>>,
pub network_chan_count: Rc<RefCell<u32>>,
pub logger: &'c test_utils::TestLogger,
pub last_block_hash: Mutex<BlockHash>,
}

impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
Expand Down Expand Up @@ -1189,6 +1192,7 @@ pub fn create_network<'a, 'b: 'a, 'c: 'b>(node_count: usize, cfgs: &'b Vec<NodeC
keys_manager: &cfgs[i].keys_manager, node: &chan_mgrs[i], net_graph_msg_handler,
node_seed: cfgs[i].node_seed, network_chan_count: chan_count.clone(),
network_payment_count: payment_count.clone(), logger: cfgs[i].logger,
last_block_hash: Mutex::new(genesis_block(Network::Testnet).header.block_hash()),
})
}

Expand Down
20 changes: 11 additions & 9 deletions lightning/src/ln/reorg_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ use util::test_utils;
use util::ser::{ReadableArgs, Writeable};

use bitcoin::blockdata::block::{Block, BlockHeader};
use bitcoin::blockdata::constants::genesis_block;
use bitcoin::hash_types::BlockHash;
use bitcoin::network::constants::Network;

use std::collections::HashMap;
use std::mem;
Expand Down Expand Up @@ -208,7 +210,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool) {
mem::drop(channel_state);

let mut headers = Vec::new();
let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
let mut header = BlockHeader { version: 0x20000000, prev_blockhash: genesis_block(Network::Testnet).header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
headers.push(header.clone());
for _i in 2..100 {
header = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
Expand Down Expand Up @@ -319,7 +321,7 @@ fn test_set_outpoints_partial_claiming() {
check_spends!(remote_txn[2], remote_txn[0]);

// Connect blocks on node A to advance height towards TEST_FINAL_CLTV
let prev_header_100 = connect_blocks(&nodes[1], 100, 0, false, Default::default());
let block_hash_100 = connect_blocks(&nodes[1], 100, 0, false, genesis_block(Network::Testnet).header.block_hash());
// Provide node A with both preimage
nodes[0].node.claim_funds(payment_preimage_1, &None, 3_000_000);
nodes[0].node.claim_funds(payment_preimage_2, &None, 3_000_000);
Expand All @@ -328,8 +330,8 @@ fn test_set_outpoints_partial_claiming() {
nodes[0].node.get_and_clear_pending_msg_events();

// Connect blocks on node A commitment transaction
let header = BlockHeader { version: 0x20000000, prev_blockhash: prev_header_100, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
connect_block(&nodes[0], &Block { header, txdata: vec![remote_txn[0].clone()] }, 101);
let header_101 = BlockHeader { version: 0x20000000, prev_blockhash: block_hash_100, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
connect_block(&nodes[0], &Block { header: header_101, txdata: vec![remote_txn[0].clone()] }, 101);
check_closed_broadcast!(nodes[0], false);
check_added_monitors!(nodes[0], 1);
// Verify node A broadcast tx claiming both HTLCs
Expand Down Expand Up @@ -361,8 +363,8 @@ fn test_set_outpoints_partial_claiming() {
};

// Broadcast partial claim on node A, should regenerate a claiming tx with HTLC dropped
let header = BlockHeader { version: 0x20000000, prev_blockhash: header.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
connect_block(&nodes[0], &Block { header, txdata: vec![partial_claim_tx.clone()] }, 102);
let header_102 = BlockHeader { version: 0x20000000, prev_blockhash: header_101.block_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
connect_block(&nodes[0], &Block { header: header_102, txdata: vec![partial_claim_tx.clone()] }, 102);
{
let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
assert_eq!(node_txn.len(), 1);
Expand All @@ -373,7 +375,7 @@ fn test_set_outpoints_partial_claiming() {
nodes[0].node.get_and_clear_pending_msg_events();

// Disconnect last block on node A, should regenerate a claiming tx with HTLC dropped
disconnect_block(&nodes[0], &header, 102);
disconnect_block(&nodes[0], &header_102, 102);
{
let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
assert_eq!(node_txn.len(), 1);
Expand All @@ -383,8 +385,8 @@ fn test_set_outpoints_partial_claiming() {
}

//// Disconnect one more block and then reconnect multiple no transaction should be generated
disconnect_block(&nodes[0], &header, 101);
connect_blocks(&nodes[1], 15, 101, false, prev_header_100);
disconnect_block(&nodes[0], &header_101, 101);
connect_blocks(&nodes[1], 15, 101, false, block_hash_100);
{
let mut node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap();
assert_eq!(node_txn.len(), 0);
Expand Down