Skip to content

Remove some duplicata of broadcast txn from ChannelMonitor #461

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 2 commits into from
Feb 12, 2020
Merged

Remove some duplicata of broadcast txn from ChannelMonitor #461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c595a7
Explicit pass index at HTLC spending transaction signing
Jan 27, 2020
494219e
Remove duplicata of broadcast txn from ChannelMonitor
Jan 23, 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
19 changes: 9 additions & 10 deletions lightning/src/ln/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1771,11 +1771,11 @@ impl ChannelMonitor {
let mut inputs_info = Vec::new();

macro_rules! sign_input {
($sighash_parts: expr, $input: expr, $amount: expr, $preimage: expr) => {
($sighash_parts: expr, $input: expr, $amount: expr, $preimage: expr, $idx: expr) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're gonna stop relying on the stupid hack of shoving the idx in the sequence field (which is a good cleanup), can you do it in its own commit and fully remove it instead of only partially?

Copy link
Author

Choose a reason for hiding this comment

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

That was a quick fix, given than in #462, the macro and a bunch of codes around is going away. Split in its own commit anyway (bundled there because of the very basic dedup parser in test_utils relying on nSequence)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but its easier to review if you just do it all the way...this patch seems to pass tests:

diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs
index d55db634..8a393be9 100644
--- a/lightning/src/ln/channelmonitor.rs
+++ b/lightning/src/ln/channelmonitor.rs
@@ -1805,17 +1805,16 @@ impl ChannelMonitor {
                                                        }
                                                        if let Some(payment_preimage) = self.payment_preimages.get(&htlc.payment_hash) {
                                                                if htlc.offered {
-                                                                       let mut input = TxIn {
+                                                                       let input = TxIn {
                                                                                previous_output: BitcoinOutPoint {
                                                                                        txid: commitment_txid,
                                                                                        vout: transaction_output_index,
                                                                                },
                                                                                script_sig: Script::new(),
-                                                                               sequence: idx as u32, // reset to 0xfffffffd before sign_input
+                                                                               sequence: 0xff_ff_ff_fd,
                                                                                witness: Vec::new(),
                                                                        };
                                                                        if htlc.cltv_expiry > height + CLTV_SHARED_CLAIM_BUFFER {
-                                                                               input.sequence = 0xff_ff_ff_fd;
                                                                                inputs.push(input);
                                                                                inputs_desc.push(if htlc.offered { InputDescriptors::OfferedHTLC } else { InputDescriptors::ReceivedHTLC });
                                                                                inputs_info.push((payment_preimage, tx.output[transaction_output_index as usize].value, htlc.cltv_expiry, idx));
@@ -1834,8 +1833,6 @@ impl ChannelMonitor {
                                                                                let height_timer = Self::get_height_timer(height, htlc.cltv_expiry);
                                                                                let mut used_feerate;
                                                                                if subtract_high_prio_fee!(self, fee_estimator, single_htlc_tx.output[0].value, predicted_weight, used_feerate) {
-                                                                                       let idx = single_htlc_tx.input[0].sequence;
-                                                                                       single_htlc_tx.input[0].sequence = 0xff_ff_ff_fd;
                                                                                        let sighash_parts = bip143::SighashComponents::new(&single_htlc_tx);
                                                                                        let (redeemscript, htlc_key) = sign_input!(sighash_parts, single_htlc_tx.input[0], htlc.amount_msat / 1000, payment_preimage.0.to_vec(), idx);
                                                                                        assert!(predicted_weight >= single_htlc_tx.get_weight());
@@ -1868,7 +1865,7 @@ impl ChannelMonitor {
                                                                                vout: transaction_output_index,
                                                                        },
                                                                        script_sig: Script::new(),
-                                                                       sequence: idx as u32,
+                                                                       sequence: 0xff_ff_ff_fd,
                                                                        witness: Vec::new(),
                                                                };
                                                                let mut timeout_tx = Transaction {
@@ -1884,8 +1881,6 @@ impl ChannelMonitor {
                                                                let height_timer = Self::get_height_timer(height, htlc.cltv_expiry);
                                                                let mut used_feerate;
                                                                if subtract_high_prio_fee!(self, fee_estimator, timeout_tx.output[0].value, predicted_weight, used_feerate) {
-                                                                       let idx = timeout_tx.input[0].sequence;
-                                                                       timeout_tx.input[0].sequence = 0xff_ff_ff_fd;
                                                                        let sighash_parts = bip143::SighashComponents::new(&timeout_tx);
                                                                        let (redeemscript, htlc_key) = sign_input!(sighash_parts, timeout_tx.input[0], htlc.amount_msat / 1000, vec![0], idx);
                                                                        assert!(predicted_weight >= timeout_tx.get_weight());

{
let (sig, redeemscript, htlc_key) = match self.key_storage {
Storage::Local { ref htlc_base_key, .. } => {
let htlc = &per_commitment_option.unwrap()[$input.sequence as usize].0;
let htlc = &per_commitment_option.unwrap()[$idx as usize].0;
let redeemscript = chan_utils::get_htlc_redeemscript_with_explicit_keys(htlc, &a_htlc_key, &b_htlc_key, &revocation_pubkey);
let sighash = hash_to_message!(&$sighash_parts.sighash_all(&$input, &redeemscript, $amount)[..]);
let htlc_key = ignore_error!(chan_utils::derive_private_key(&self.secp_ctx, revocation_point, &htlc_base_key));
Expand Down Expand Up @@ -1804,19 +1804,19 @@ impl ChannelMonitor {
}
if let Some(payment_preimage) = self.payment_preimages.get(&htlc.payment_hash) {
if htlc.offered {
let input = TxIn {
let mut input = TxIn {
previous_output: BitcoinOutPoint {
txid: commitment_txid,
vout: transaction_output_index,
},
script_sig: Script::new(),
sequence: idx as u32, // reset to 0xfffffffd in sign_input
sequence: 0xff_ff_ff_fd,
witness: Vec::new(),
};
if htlc.cltv_expiry > height + CLTV_SHARED_CLAIM_BUFFER {
inputs.push(input);
inputs_desc.push(if htlc.offered { InputDescriptors::OfferedHTLC } else { InputDescriptors::ReceivedHTLC });
inputs_info.push((payment_preimage, tx.output[transaction_output_index as usize].value, htlc.cltv_expiry));
inputs_info.push((payment_preimage, tx.output[transaction_output_index as usize].value, htlc.cltv_expiry, idx));
total_value += tx.output[transaction_output_index as usize].value;
} else {
let mut single_htlc_tx = Transaction {
Expand All @@ -1833,7 +1833,7 @@ impl ChannelMonitor {
let mut used_feerate;
if subtract_high_prio_fee!(self, fee_estimator, single_htlc_tx.output[0].value, predicted_weight, used_feerate) {
let sighash_parts = bip143::SighashComponents::new(&single_htlc_tx);
let (redeemscript, htlc_key) = sign_input!(sighash_parts, single_htlc_tx.input[0], htlc.amount_msat / 1000, payment_preimage.0.to_vec());
let (redeemscript, htlc_key) = sign_input!(sighash_parts, single_htlc_tx.input[0], htlc.amount_msat / 1000, payment_preimage.0.to_vec(), idx);
assert!(predicted_weight >= single_htlc_tx.get_weight());
spendable_outputs.push(SpendableOutputDescriptor::StaticOutput {
outpoint: BitcoinOutPoint { txid: single_htlc_tx.txid(), vout: 0 },
Expand Down Expand Up @@ -1864,7 +1864,7 @@ impl ChannelMonitor {
vout: transaction_output_index,
},
script_sig: Script::new(),
sequence: idx as u32,
sequence: 0xff_ff_ff_fd,
witness: Vec::new(),
};
let mut timeout_tx = Transaction {
Expand All @@ -1881,7 +1881,7 @@ impl ChannelMonitor {
let mut used_feerate;
if subtract_high_prio_fee!(self, fee_estimator, timeout_tx.output[0].value, predicted_weight, used_feerate) {
let sighash_parts = bip143::SighashComponents::new(&timeout_tx);
let (redeemscript, htlc_key) = sign_input!(sighash_parts, timeout_tx.input[0], htlc.amount_msat / 1000, vec![0]);
let (redeemscript, htlc_key) = sign_input!(sighash_parts, timeout_tx.input[0], htlc.amount_msat / 1000, vec![0], idx);
assert!(predicted_weight >= timeout_tx.get_weight());
//TODO: track SpendableOutputDescriptor
log_trace!(self, "Outpoint {}:{} is being being claimed, if it doesn't succeed, a bumped claiming txn is going to be broadcast at height {}", timeout_tx.input[0].previous_output.txid, timeout_tx.input[0].previous_output.vout, height_timer);
Expand Down Expand Up @@ -1933,7 +1933,7 @@ impl ChannelMonitor {
let height_timer = Self::get_height_timer(height, soonest_timelock);
let spend_txid = spend_tx.txid();
for (input, info) in spend_tx.input.iter_mut().zip(inputs_info.iter()) {
let (redeemscript, htlc_key) = sign_input!(sighash_parts, input, info.1, (info.0).0.to_vec());
let (redeemscript, htlc_key) = sign_input!(sighash_parts, input, info.1, (info.0).0.to_vec(), info.3);
log_trace!(self, "Outpoint {}:{} is being being claimed, if it doesn't succeed, a bumped claiming txn is going to be broadcast at height {}", input.previous_output.txid, input.previous_output.vout, height_timer);
per_input_material.insert(input.previous_output, InputMaterial::RemoteHTLC { script: redeemscript, key: htlc_key, preimage: Some(*(info.0)), amount: info.1, locktime: 0});
match self.claimable_outpoints.entry(input.previous_output) {
Expand Down Expand Up @@ -2871,7 +2871,6 @@ impl ChannelMonitor {
for per_outp_material in cached_claim_datas.per_input_material.values() {
match per_outp_material {
&InputMaterial::Revoked { ref script, ref is_htlc, ref amount, .. } => {
log_trace!(self, "Is HLTC ? {}", is_htlc);
inputs_witnesses_weight += Self::get_witnesses_weight(if !is_htlc { &[InputDescriptors::RevokedOutput] } else if HTLCType::scriptlen_to_htlctype(script.len()) == Some(HTLCType::OfferedHTLC) { &[InputDescriptors::RevokedOfferedHTLC] } else if HTLCType::scriptlen_to_htlctype(script.len()) == Some(HTLCType::AcceptedHTLC) { &[InputDescriptors::RevokedReceivedHTLC] } else { unreachable!() });
amt += *amount;
},
Expand Down
3 changes: 2 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 @@ -35,6 +35,7 @@ use std::cell::RefCell;
use std::rc::Rc;
use std::sync::{Arc, Mutex};
use std::mem;
use std::collections::HashSet;

pub const CHAN_CONFIRM_DEPTH: u32 = 100;
pub fn confirm_transaction<'a, 'b: 'a>(notifier: &'a chaininterface::BlockNotifierRef<'b>, chain: &chaininterface::ChainWatchInterfaceUtil, tx: &Transaction, chan_id: u32) {
Expand Down Expand Up @@ -857,7 +858,7 @@ pub fn create_node_cfgs(node_count: usize) -> Vec<NodeCfg> {
let logger = Arc::new(test_utils::TestLogger::with_id(format!("node {}", i)));
let fee_estimator = Arc::new(test_utils::TestFeeEstimator { sat_per_kw: 253 });
let chain_monitor = Arc::new(chaininterface::ChainWatchInterfaceUtil::new(Network::Testnet, logger.clone() as Arc<Logger>));
let tx_broadcaster = Arc::new(test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new())});
let tx_broadcaster = Arc::new(test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), broadcasted_txn: Mutex::new(HashSet::new())});
let mut seed = [0; 32];
rng.fill_bytes(&mut seed);
let keys_manager = Arc::new(test_utils::TestKeysInterface::new(&seed, Network::Testnet, logger.clone() as Arc<Logger>));
Expand Down
Loading