Skip to content

A few minor nits on #462 #535

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 4 commits into from
Mar 5, 2020
Merged

A few minor nits on #462 #535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c3533d
Flatten Vec passed from channelmonitor to onchaintx block_connected
TheBlueMatt Mar 4, 2020
9de9288
Correct comment in onchaintx.rs
TheBlueMatt Mar 4, 2020
f554c59
Drop redundant .clone() in check_spends calls.
TheBlueMatt Mar 4, 2020
6abce81
Take multiple spent-txn to check_spends! in functional_tests
TheBlueMatt Mar 4, 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
4 changes: 2 additions & 2 deletions lightning/src/ln/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2010,7 +2010,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
watch_outputs.push(new_outputs);
}
}
claimable_outpoints.push(new_outpoints);
claimable_outpoints.append(&mut new_outpoints);
Copy link

Choose a reason for hiding this comment

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

It was done this way to avoid mixing spend between revoked commitment txn and HTLC-txn in same block. Our tracking stuff should be reliable enough in case of one of them being reorg-out but still, make harder to reason on it...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, right, but it doesn't do that on master, either. the behavior didn't change in this commit, your last refactor before merge changed that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And, also, I think we should aggregate such outputs - reorgs are rare-ish, and if we have to fall back to individual transactions to deal with them so be it, but in the common case it saves a good chunk on fees, so we should do it.

Copy link

Choose a reason for hiding this comment

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

No my last change was segregating them inside Vec<> and then iterating inside OnChainTxHandler::block_connected ? Let's move forward with this but add an issue somewhere we should add test-coverage for this case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They all get added to the same aggregated_claim map, though.

}
if !funding_txo.is_none() && claimable_outpoints.is_empty() {
if let Some(spendable_output) = self.check_spend_closing_transaction(&tx) {
Expand All @@ -2020,7 +2020,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> {
} else {
if let Some(&(commitment_number, _)) = self.remote_commitment_txn_on_chain.get(&prevout.txid) {
let mut new_outpoints = self.check_spend_remote_htlc(&tx, commitment_number, height);
claimable_outpoints.push(new_outpoints);
claimable_outpoints.append(&mut new_outpoints);
}
}
}
Expand Down
19 changes: 10 additions & 9 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,14 +403,15 @@ pub fn create_announced_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(nodes: &'a
}

macro_rules! check_spends {
($tx: expr, $spends_tx: expr) => {
($tx: expr, $($spends_txn: expr),*) => {
{
$tx.verify(|out_point| {
if out_point.txid == $spends_tx.txid() {
$spends_tx.output.get(out_point.vout as usize).cloned()
} else {
None
}
$(
if out_point.txid == $spends_txn.txid() {
return $spends_txn.output.get(out_point.vout as usize).cloned()
}
)*
None
}).unwrap();
}
}
Expand Down Expand Up @@ -1033,7 +1034,7 @@ pub fn test_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, chan: &(msgs::Cha
let mut res = Vec::with_capacity(2);
node_txn.retain(|tx| {
if tx.input.len() == 1 && tx.input[0].previous_output.txid == chan.3.txid() {
check_spends!(tx, chan.3.clone());
check_spends!(tx, chan.3);
if commitment_tx.is_none() {
res.push(tx.clone());
}
Expand All @@ -1049,7 +1050,7 @@ pub fn test_txn_broadcast<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, chan: &(msgs::Cha
if has_htlc_tx != HTLCType::NONE {
node_txn.retain(|tx| {
if tx.input.len() == 1 && tx.input[0].previous_output.txid == res[0].txid() {
check_spends!(tx, res[0].clone());
check_spends!(tx, res[0]);
if has_htlc_tx == HTLCType::TIMEOUT {
assert!(tx.lock_time != 0);
} else {
Expand Down Expand Up @@ -1098,7 +1099,7 @@ pub fn check_preimage_claim<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, prev_txn: &Vec<

for tx in prev_txn {
if node_txn[0].input[0].previous_output.txid == tx.txid() {
check_spends!(node_txn[0], tx.clone());
check_spends!(node_txn[0], tx);
assert!(node_txn[0].input[0].witness[2].len() > 106); // must spend an htlc output
assert_eq!(tx.input.len(), 1); // must spend a commitment tx

Expand Down
Loading