-
Notifications
You must be signed in to change notification settings - Fork 455
Fix chanmon_consistency fuzz test for splice RBF
#4536
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
5dfe303
c972bf2
642baa6
0e90b5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,24 +186,46 @@ impl BroadcasterInterface for TestBroadcaster { | |
| struct ChainState { | ||
| blocks: Vec<(Header, Vec<Transaction>)>, | ||
| confirmed_txids: HashSet<Txid>, | ||
| /// Unconfirmed transactions (e.g., splice txs). Conflicting RBF candidates may coexist; | ||
| /// `confirm_pending_txs` determines which one confirms. | ||
| pending_txs: Vec<Transaction>, | ||
| } | ||
|
|
||
| impl ChainState { | ||
| fn new() -> Self { | ||
| let genesis_hash = genesis_block(Network::Bitcoin).block_hash(); | ||
| let genesis_header = create_dummy_header(genesis_hash, 42); | ||
| Self { blocks: vec![(genesis_header, Vec::new())], confirmed_txids: HashSet::new() } | ||
| Self { | ||
| blocks: vec![(genesis_header, Vec::new())], | ||
| confirmed_txids: HashSet::new(), | ||
| pending_txs: Vec::new(), | ||
| } | ||
| } | ||
|
|
||
| fn tip_height(&self) -> u32 { | ||
| (self.blocks.len() - 1) as u32 | ||
| } | ||
|
|
||
| fn is_outpoint_spent(&self, outpoint: &bitcoin::OutPoint) -> bool { | ||
| // Only check the last 6 blocks (1 confirmation block + 5 post-confirmation) to avoid | ||
| // false positives from hash collisions in older blocks. Under fuzz hashing, txids have | ||
| // only 8 effective bits, so unrelated outpoints in old blocks frequently collide. | ||
| let start = self.blocks.len().saturating_sub(6); | ||
| self.blocks[start..].iter().any(|(_, txs)| { | ||
| txs.iter().any(|tx| { | ||
| tx.input.iter().any(|input| input.previous_output == *outpoint) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note that because hashes in fuzzing can collide, this implies some splices will never lock in - is that an issue for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... maybe we can mostly avoid some occurrences by only looking back six blocks?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may not need this hack anymore after #4565? |
||
| }) | ||
| }) | ||
| } | ||
|
|
||
| fn confirm_tx(&mut self, tx: Transaction) -> bool { | ||
| let txid = tx.compute_txid(); | ||
| if self.confirmed_txids.contains(&txid) { | ||
| return false; | ||
| } | ||
| if tx.input.iter().any(|input| self.is_outpoint_spent(&input.previous_output)) { | ||
| return false; | ||
| } | ||
| self.confirmed_txids.insert(txid); | ||
|
|
||
| let prev_hash = self.blocks.last().unwrap().0.block_hash(); | ||
|
|
@@ -218,6 +240,54 @@ impl ChainState { | |
| true | ||
| } | ||
|
|
||
| /// Add a transaction to the pending pool (mempool). Multiple conflicting transactions (RBF | ||
| /// candidates) may coexist; `confirm_pending_txs` selects which one to confirm. | ||
| fn add_pending_tx(&mut self, tx: Transaction) { | ||
| self.pending_txs.push(tx); | ||
| } | ||
|
|
||
| /// Confirm pending transactions in a single block, selecting deterministically among | ||
| /// conflicting RBF candidates. Sorting by txid ensures the winner is determined by fuzz input | ||
| /// content. Transactions that double-spend an already-confirmed outpoint are skipped. | ||
| fn confirm_pending_txs(&mut self) { | ||
| let mut txs = std::mem::take(&mut self.pending_txs); | ||
| txs.sort_by_key(|tx| tx.compute_txid()); | ||
|
|
||
| let mut confirmed = Vec::new(); | ||
| let mut spent_outpoints = Vec::new(); | ||
| for tx in txs { | ||
| let txid = tx.compute_txid(); | ||
| if self.confirmed_txids.contains(&txid) { | ||
| continue; | ||
| } | ||
| if tx.input.iter().any(|input| { | ||
| self.is_outpoint_spent(&input.previous_output) | ||
| || spent_outpoints.contains(&input.previous_output) | ||
| }) { | ||
| continue; | ||
| } | ||
| self.confirmed_txids.insert(txid); | ||
| for input in &tx.input { | ||
| spent_outpoints.push(input.previous_output); | ||
| } | ||
| confirmed.push(tx); | ||
| } | ||
|
|
||
| if confirmed.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| let prev_hash = self.blocks.last().unwrap().0.block_hash(); | ||
| let header = create_dummy_header(prev_hash, 42); | ||
| self.blocks.push((header, confirmed)); | ||
|
|
||
| for _ in 0..5 { | ||
| let prev_hash = self.blocks.last().unwrap().0.block_hash(); | ||
| let header = create_dummy_header(prev_hash, 42); | ||
| self.blocks.push((header, Vec::new())); | ||
| } | ||
| } | ||
|
|
||
| fn block_at(&self, height: u32) -> &(Header, Vec<Transaction>) { | ||
| &self.blocks[height as usize] | ||
| } | ||
|
|
@@ -856,11 +926,15 @@ fn send_mpp_hop_payment( | |
| fn assert_action_timeout_awaiting_response(action: &msgs::ErrorAction) { | ||
| // Since sending/receiving messages may be delayed, `timer_tick_occurred` may cause a node to | ||
| // disconnect their counterparty if they're expecting a timely response. | ||
| assert!(matches!( | ||
| assert!( | ||
| matches!( | ||
| action, | ||
| msgs::ErrorAction::DisconnectPeerWithWarning { msg } | ||
| if msg.data.contains("Disconnecting due to timeout awaiting response") | ||
| ), | ||
| "Expected timeout disconnect, got: {:?}", | ||
| action, | ||
| msgs::ErrorAction::DisconnectPeerWithWarning { msg } | ||
| if msg.data.contains("Disconnecting due to timeout awaiting response") | ||
| )); | ||
| ); | ||
| } | ||
|
|
||
| enum ChanType { | ||
|
|
@@ -2025,15 +2099,16 @@ pub fn do_test<Out: Output + MaybeSend + MaybeSync>(data: &[u8], out: Out) { | |
| assert!(txs.len() >= 1); | ||
| let splice_tx = txs.remove(0); | ||
| assert_eq!(new_funding_txo.txid, splice_tx.compute_txid()); | ||
| chain_state.confirm_tx(splice_tx); | ||
| chain_state.add_pending_tx(splice_tx); | ||
| }, | ||
| events::Event::SpliceFailed { .. } => {}, | ||
| events::Event::DiscardFunding { | ||
| funding_info: events::FundingInfo::Contribution { .. }, | ||
| funding_info: events::FundingInfo::Contribution { .. } | ||
| | events::FundingInfo::Tx { .. }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be better to always emit the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's part of #4498, right? I guess we should prioritize that, too. 😬
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed offline. There is an occurrence were we use |
||
| .. | ||
| } => {}, | ||
|
|
||
| _ => panic!("Unhandled event"), | ||
| _ => panic!("Unhandled event: {:?}", event), | ||
| } | ||
| } | ||
| while nodes[$node].needs_pending_htlc_processing() { | ||
|
|
@@ -2477,13 +2552,31 @@ pub fn do_test<Out: Output + MaybeSend + MaybeSync>(data: &[u8], out: Out) { | |
| }, | ||
|
|
||
| // Sync node by 1 block to cover confirmation of a transaction. | ||
| 0xa8 => sync_with_chain_state(&mut chain_state, &nodes[0], &mut node_height_a, Some(1)), | ||
| 0xa9 => sync_with_chain_state(&mut chain_state, &nodes[1], &mut node_height_b, Some(1)), | ||
| 0xaa => sync_with_chain_state(&mut chain_state, &nodes[2], &mut node_height_c, Some(1)), | ||
| 0xa8 => { | ||
| chain_state.confirm_pending_txs(); | ||
| sync_with_chain_state(&mut chain_state, &nodes[0], &mut node_height_a, Some(1)); | ||
| }, | ||
| 0xa9 => { | ||
| chain_state.confirm_pending_txs(); | ||
| sync_with_chain_state(&mut chain_state, &nodes[1], &mut node_height_b, Some(1)); | ||
| }, | ||
| 0xaa => { | ||
| chain_state.confirm_pending_txs(); | ||
| sync_with_chain_state(&mut chain_state, &nodes[2], &mut node_height_c, Some(1)); | ||
| }, | ||
| // Sync node to chain tip to cover confirmation of a transaction post-reorg-risk. | ||
| 0xab => sync_with_chain_state(&mut chain_state, &nodes[0], &mut node_height_a, None), | ||
| 0xac => sync_with_chain_state(&mut chain_state, &nodes[1], &mut node_height_b, None), | ||
| 0xad => sync_with_chain_state(&mut chain_state, &nodes[2], &mut node_height_c, None), | ||
| 0xab => { | ||
| chain_state.confirm_pending_txs(); | ||
| sync_with_chain_state(&mut chain_state, &nodes[0], &mut node_height_a, None); | ||
| }, | ||
| 0xac => { | ||
| chain_state.confirm_pending_txs(); | ||
| sync_with_chain_state(&mut chain_state, &nodes[1], &mut node_height_b, None); | ||
| }, | ||
| 0xad => { | ||
| chain_state.confirm_pending_txs(); | ||
| sync_with_chain_state(&mut chain_state, &nodes[2], &mut node_height_c, None); | ||
| }, | ||
|
|
||
| 0xb0 | 0xb1 | 0xb2 => { | ||
| // Restart node A, picking among the in-flight `ChannelMonitor`s to use based on | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not quite the same, but #4381 is also extending the realism level of the chain.