-
Notifications
You must be signed in to change notification settings - Fork 350
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
Implement claiming of revoked HTLC transactions by ChannelMonitor #163
Implement claiming of revoked HTLC transactions by ChannelMonitor #163
Conversation
src/ln/channelmonitor.rs
Outdated
total_value += tx.output[0].value; | ||
} | ||
|
||
key_materials.push((redeemscript, per_commitment_key)); |
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.
Buffering key materials to create only one transaction even if inputs are from unrelated commitment_tx
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.
I'm not sure this is necessary - there cant be two inputs pointing to two different remote_commitment_txn_on_chain as the commitment txn would be double-spends.
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.
At first glance looks good, maybe replace the TODO comment with a comment describing what we're doing here, but definitely needs tests (All the claim stuff needs more tests, but at least adding new cases should get tested).
src/ln/channelmonitor.rs
Outdated
|
||
|
||
if tx.output[0].script_pubkey == revokeable_p2wsh { //HTLC transactions have one txin, one txout | ||
inputs.push(TxIn { |
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.
You should try to honor the CLTV_SHARED_CLAIM_BUFFER here - splitting out inputs into separate transactions if they expire too soon.
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.
Hmmmm in case of HTLC-Success/Timeout transactions aren't we more affected by CSV delay ? If so I think we want to follow the same delay buffer as for a CLTV but we maybe need to update comment of CLTV_SHARED_CLAIM_BUFFER. And here, if it's HTLC-Success/Timemout transactions, I think we can only have one input so no case of multi-inputs to separate ?
src/ln/channelmonitor.rs
Outdated
total_value += tx.output[0].value; | ||
} | ||
|
||
key_materials.push((redeemscript, per_commitment_key)); |
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.
I'm not sure this is necessary - there cant be two inputs pointing to two different remote_commitment_txn_on_chain as the commitment txn would be double-spends.
7d07498
to
f52f666
Compare
src/ln/channelmanager.rs
Outdated
let mut node_txn = node.tx_broadcaster.txn_broadcasted.lock().unwrap(); | ||
assert!(node_txn.len() >= if commitment_tx.is_some() { 0 } else { 1 } + if has_htlc_tx == HTLCType::NONE { 0 } else { 1 }); | ||
assert!(node_txn.len() >= if has_htlc_tx == HTLCType::NONE { 0 } else { 1 } + if has_penalty_tx == PenaltyType::NONE { 0 } else { 1 }); |
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.
I removed the commitment_tx ternary has it's wasn't making sense on revoked_tx case, but tell me on this and other changes of test_txn_broadcast, maybe more assert! needed
} | ||
|
||
(txn_to_broadcast, (commitment_txid, watch_outputs)) | ||
} | ||
|
||
/// Attempst to claim a remote HTLC-Success/HTLC-Timeout s outputs using the revocation key | ||
fn check_spend_remote_htlc(&self, tx: &Transaction, commitment_number: u64) -> Vec<Transaction> { |
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.
Was refactored mainly to avoid double-locking of remote_commitment_tx_on_chain as block_connected has only them to match HTLC transactions, so a part of old commit was moved there.
With set_their_delayed_payment_base_key as I think Channel passing it to monitor at channel opening is the only way to get it ? |
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.
Awesome! Nice work!
src/ln/channel.rs
Outdated
@@ -563,6 +563,7 @@ impl Channel { | |||
&chan_keys.htlc_base_key, | |||
BREAKDOWN_TIMEOUT, our_channel_monitor_claim_script); | |||
channel_monitor.set_their_htlc_base_key(&msg.htlc_basepoint); | |||
channel_monitor.set_their_delayed_payment_base_key(&msg.delayed_payment_basepoint); |
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.
Why not just merge this with set_their_htlc_base_key? They're always called together anyway...
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.
Good idea, done
src/ln/channelmonitor.rs
Outdated
|
||
let mut txn_to_broadcast = Vec::new(); | ||
|
||
let commitment_txid = tx.txid(); //TODO: This is gonna be a performance bottleneck for watchtowers! |
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.
Its not a commitment tx :p
src/ln/channelmonitor.rs
Outdated
} | ||
|
||
(txn_to_broadcast, (commitment_txid, watch_outputs)) | ||
} | ||
|
||
/// Attempst to claim a remote HTLC-Success/HTLC-Timeout s outputs using the revocation key | ||
fn check_spend_remote_htlc(&self, tx: &Transaction, commitment_number: u64) -> Vec<Transaction> { | ||
|
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.
nit: why the space here?
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.
a mine dumb copy-paste!
src/ln/channelmonitor.rs
Outdated
broadcaster.broadcast_transaction(tx); | ||
} else { | ||
let remote_commitment_txn_on_chain = self.remote_commitment_txn_on_chain.lock().unwrap(); | ||
for (txid, commitment_number) in remote_commitment_txn_on_chain.iter() { |
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.
remote_commitment_txn_on_chain is a HashMap, so you should be able to do a get instead of an iter() over the whole thing.
src/ln/channelmonitor.rs
Outdated
for tx in txn_matched { | ||
for txin in tx.input.iter() { | ||
if self.funding_txo.is_none() || (txin.previous_output.txid == self.funding_txo.as_ref().unwrap().0.txid && txin.previous_output.vout == self.funding_txo.as_ref().unwrap().0.index as u32) { | ||
let (mut txn, new_outputs) = self.check_spend_remote_transaction(tx, height); | ||
let (mut remote_txn, new_outputs) = self.check_spend_remote_transaction(tx, height); | ||
txn.append(&mut remote_txn); |
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.
Why append instead of just txn = remote_txn? Are you accidentally calling broadcast_transaction for txn from multiple ChannelMonitors multiple times? Maybe move txn into the per-tx loop?
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.
Modified, but you mean broadcasting in per-tx loop ? Does it change something for broadcaster?
src/ln/channelmanager.rs
Outdated
@@ -3108,9 +3108,11 @@ mod tests { | |||
|
|||
#[derive(PartialEq)] | |||
enum HTLCType { NONE, TIMEOUT, SUCCESS } | |||
fn test_txn_broadcast(node: &Node, chan: &(msgs::ChannelUpdate, msgs::ChannelUpdate, [u8; 32], Transaction), commitment_tx: Option<Transaction>, has_htlc_tx: HTLCType) -> Vec<Transaction> { | |||
#[derive(PartialEq)] | |||
enum PenaltyType { NONE, COMMIT, HTLC } |
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.
Is COMMIT PenaltyType unused? Looks like its somewhat duplicative with the revoked_tx option?
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.
Drop it for now but I think I'm gonna use in a next commit to test spending of remote outputs on revoked commitment tx
src/ln/channelmanager.rs
Outdated
@@ -3126,7 +3128,9 @@ mod tests { | |||
} | |||
} | |||
} | |||
assert_eq!(res.len(), 1); | |||
if revoked_tx.is_none() { |
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.
Why not just check against revoked is_none() and PenaltyType is HTLC so you check the call is doing something (and doesn't change existing logic).
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.
Modified, but you mean against revoked_is_some() right?
src/ln/channelmanager.rs
Outdated
@@ -3145,6 +3149,21 @@ mod tests { | |||
} | |||
assert_eq!(res.len(), 2); | |||
} | |||
|
|||
if has_penalty_tx == PenaltyType::HTLC { | |||
if let Some(revoked_tx) = revoked_tx { |
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.
Maybe just unwrap() to make sure the caller is sane? This is a test.....
Needed to build redeemscript on HTLC-Success/HTLC-Timeout tx from remote revoked commitment tx
Refactor check_spend_remote_transaction in part to check_spend_remote_htlc to avoid lock mess in block_connected. We need remote_commitment_txn_on_chain to match remote HTLC tx
f52f666
to
383bd90
Compare
Optimize some ChannelMonitor stuff after #163
Still working on others parts of #137. Implement claiming of revoked HTLC-Success/HTLC-timeout transactions in check_spend_remote_transaction that as I understand the TODO was all about ?
Will add/finish test in ChannelManager with others commits tomorrow.