Skip to content

Two post-#230 fixups #255

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
Nov 21, 2018
Merged

Two post-#230 fixups #255

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
90b0ed9
Provide commitment point to monitor with the remote txn update
TheBlueMatt Nov 19, 2018
3af20fd
Check P2WPKH script against expected before gen'ing an output event
TheBlueMatt Nov 20, 2018
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
10 changes: 3 additions & 7 deletions src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,6 @@ impl Channel {
&chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), BREAKDOWN_TIMEOUT,
keys_provider.get_destination_script(), logger.clone());
channel_monitor.set_their_base_keys(&msg.htlc_basepoint, &msg.delayed_payment_basepoint);
channel_monitor.provide_their_next_revocation_point(Some((INITIAL_COMMITMENT_NUMBER, msg.first_per_commitment_point)));
channel_monitor.set_their_to_self_delay(msg.to_self_delay);

let mut chan = Channel {
Expand Down Expand Up @@ -1358,7 +1357,6 @@ impl Channel {
}

self.channel_monitor.set_their_base_keys(&msg.htlc_basepoint, &msg.delayed_payment_basepoint);
self.channel_monitor.provide_their_next_revocation_point(Some((INITIAL_COMMITMENT_NUMBER, msg.first_per_commitment_point)));

self.their_dust_limit_satoshis = msg.dust_limit_satoshis;
self.their_max_htlc_value_in_flight_msat = cmp::min(msg.max_htlc_value_in_flight_msat, self.channel_value_satoshis * 1000);
Expand Down Expand Up @@ -1434,7 +1432,7 @@ impl Channel {

// Now that we're past error-generating stuff, update our local state:

self.channel_monitor.provide_latest_remote_commitment_tx_info(&remote_initial_commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number);
self.channel_monitor.provide_latest_remote_commitment_tx_info(&remote_initial_commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap());
self.last_local_commitment_txn = vec![local_initial_commitment_tx.clone()];
self.channel_monitor.provide_latest_local_commitment_tx_info(local_initial_commitment_tx, local_keys, self.feerate_per_kw, Vec::new());
self.channel_state = ChannelState::FundingSent as u32;
Expand Down Expand Up @@ -1506,7 +1504,6 @@ impl Channel {
return Err(ChannelError::Close("Peer sent a funding_locked at a strange time"));
}

self.channel_monitor.provide_their_next_revocation_point(Some((INITIAL_COMMITMENT_NUMBER - 1 , msg.next_per_commitment_point)));
self.their_prev_commitment_point = self.their_cur_commitment_point;
self.their_cur_commitment_point = Some(msg.next_per_commitment_point);
Ok(())
Expand Down Expand Up @@ -1901,7 +1898,6 @@ impl Channel {
}
self.channel_monitor.provide_secret(self.cur_remote_commitment_transaction_number + 1, msg.per_commitment_secret)
.map_err(|e| ChannelError::Close(e.0))?;
self.channel_monitor.provide_their_next_revocation_point(Some((self.cur_remote_commitment_transaction_number - 1, msg.next_per_commitment_point)));

// Update state now that we've passed all the can-fail calls...
// (note that we may still fail to generate the new commitment_signed message, but that's
Expand Down Expand Up @@ -3002,7 +2998,7 @@ impl Channel {
let temporary_channel_id = self.channel_id;

// Now that we're past error-generating stuff, update our local state:
self.channel_monitor.provide_latest_remote_commitment_tx_info(&commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number);
self.channel_monitor.provide_latest_remote_commitment_tx_info(&commitment_tx, Vec::new(), self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap());
self.channel_state = ChannelState::FundingCreated as u32;
self.channel_id = funding_txo.to_channel_id();
self.cur_remote_commitment_transaction_number -= 1;
Expand Down Expand Up @@ -3214,7 +3210,7 @@ impl Channel {
match self.send_commitment_no_state_update() {
Ok((res, remote_commitment_tx)) => {
// Update state now that we've passed all the can-fail calls...
self.channel_monitor.provide_latest_remote_commitment_tx_info(&remote_commitment_tx.0, remote_commitment_tx.1, self.cur_remote_commitment_transaction_number);
self.channel_monitor.provide_latest_remote_commitment_tx_info(&remote_commitment_tx.0, remote_commitment_tx.1, self.cur_remote_commitment_transaction_number, self.their_cur_commitment_point.unwrap());
self.channel_state |= ChannelState::AwaitingRemoteRevoke as u32;
Ok((res, self.channel_monitor.clone()))
},
Expand Down
105 changes: 46 additions & 59 deletions src/ln/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,37 +461,11 @@ impl ChannelMonitor {
Ok(())
}

/// Tracks the next revocation point which may be required to claim HTLC outputs which we know
/// the preimage of in case the remote end force-closes using their latest state. When called at
/// channel opening revocation point is the CURRENT one used for first commitment tx. Needed in case of sizeable push_msat.
pub(super) fn provide_their_next_revocation_point(&mut self, their_next_revocation_point: Option<(u64, PublicKey)>) {
if let Some(new_revocation_point) = their_next_revocation_point {
match self.their_cur_revocation_points {
Some(old_points) => {
if old_points.0 == new_revocation_point.0 + 1 {
self.their_cur_revocation_points = Some((old_points.0, old_points.1, Some(new_revocation_point.1)));
} else if old_points.0 == new_revocation_point.0 + 2 {
if let Some(old_second_point) = old_points.2 {
self.their_cur_revocation_points = Some((old_points.0 - 1, old_second_point, Some(new_revocation_point.1)));
} else {
self.their_cur_revocation_points = Some((new_revocation_point.0, new_revocation_point.1, None));
}
} else {
self.their_cur_revocation_points = Some((new_revocation_point.0, new_revocation_point.1, None));
}
},
None => {
self.their_cur_revocation_points = Some((new_revocation_point.0, new_revocation_point.1, None));
}
}
}
}

/// Informs this monitor of the latest remote (ie non-broadcastable) commitment transaction.
/// The monitor watches for it to be broadcasted and then uses the HTLC information (and
/// possibly future revocation/preimage information) to claim outputs where possible.
/// We cache also the mapping hash:commitment number to lighten pruning of old preimages by watchtowers.
pub(super) fn provide_latest_remote_commitment_tx_info(&mut self, unsigned_commitment_tx: &Transaction, htlc_outputs: Vec<HTLCOutputInCommitment>, commitment_number: u64) {
pub(super) fn provide_latest_remote_commitment_tx_info(&mut self, unsigned_commitment_tx: &Transaction, htlc_outputs: Vec<HTLCOutputInCommitment>, commitment_number: u64, their_revocation_point: PublicKey) {
// TODO: Encrypt the htlc_outputs data with the single-hash of the commitment transaction
// so that a remote monitor doesn't learn anything unless there is a malicious close.
// (only maybe, sadly we cant do the same for local info, as we need to be aware of
Expand All @@ -501,6 +475,25 @@ impl ChannelMonitor {
}
self.remote_claimable_outpoints.insert(unsigned_commitment_tx.txid(), htlc_outputs);
self.current_remote_commitment_number = commitment_number;
//TODO: Merge this into the other per-remote-transaction output storage stuff
match self.their_cur_revocation_points {
Some(old_points) => {
if old_points.0 == commitment_number + 1 {
self.their_cur_revocation_points = Some((old_points.0, old_points.1, Some(their_revocation_point)));
} else if old_points.0 == commitment_number + 2 {
if let Some(old_second_point) = old_points.2 {
self.their_cur_revocation_points = Some((old_points.0 - 1, old_second_point, Some(their_revocation_point)));
} else {
self.their_cur_revocation_points = Some((commitment_number, their_revocation_point, None));
}
} else {
self.their_cur_revocation_points = Some((commitment_number, their_revocation_point, None));
}
},
None => {
self.their_cur_revocation_points = Some((commitment_number, their_revocation_point, None));
}
}
}

/// Informs this monitor of the latest local (ie broadcastable) commitment transaction. The
Expand Down Expand Up @@ -896,16 +889,18 @@ impl ChannelMonitor {
if commitment_number >= self.get_min_seen_secret() {
let secret = self.get_secret(commitment_number).unwrap();
let per_commitment_key = ignore_error!(SecretKey::from_slice(&self.secp_ctx, &secret));
let (revocation_pubkey, b_htlc_key) = match self.key_storage {
KeyStorage::PrivMode { ref revocation_base_key, ref htlc_base_key, .. } => {
let (revocation_pubkey, b_htlc_key, local_payment_key) = match self.key_storage {
KeyStorage::PrivMode { ref revocation_base_key, ref htlc_base_key, ref payment_base_key, .. } => {
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
(ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &PublicKey::from_secret_key(&self.secp_ctx, &revocation_base_key))),
ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &per_commitment_point, &PublicKey::from_secret_key(&self.secp_ctx, &htlc_base_key))))
ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &per_commitment_point, &PublicKey::from_secret_key(&self.secp_ctx, &htlc_base_key))),
Some(ignore_error!(chan_utils::derive_private_key(&self.secp_ctx, &per_commitment_point, &payment_base_key))))
},
KeyStorage::SigsMode { ref revocation_base_key, ref htlc_base_key, .. } => {
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
(ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &revocation_base_key)),
ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &per_commitment_point, &htlc_base_key)))
ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &per_commitment_point, &htlc_base_key)),
None)
},
};
let delayed_key = ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &self.their_delayed_payment_base_key.unwrap()));
Expand All @@ -917,6 +912,13 @@ impl ChannelMonitor {
let revokeable_redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.our_to_self_delay, &delayed_key);
let revokeable_p2wsh = revokeable_redeemscript.to_v0_p2wsh();

let local_payment_p2wpkh = if let Some(payment_key) = local_payment_key {
// Note that the Network here is ignored as we immediately drop the address for the
// script_pubkey version.
let payment_hash160 = Hash160::from_data(&PublicKey::from_secret_key(&self.secp_ctx, &payment_key).serialize());
Some(Builder::new().push_opcode(opcodes::All::OP_PUSHBYTES_0).push_slice(&payment_hash160[..]).into_script())
} else { None };

let mut total_value = 0;
let mut values = Vec::new();
let mut inputs = Vec::new();
Expand All @@ -936,23 +938,12 @@ impl ChannelMonitor {
htlc_idxs.push(None);
values.push(outp.value);
total_value += outp.value;
} else if outp.script_pubkey.is_v0_p2wpkh() {
Copy link

Choose a reason for hiding this comment

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

Not sure for the bug fixes, the only P2WPKH from a remote revoked tx is ours following bolt 3, how the sequence/lock-time mess can false-positives the tx detection ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically at this point in check_spend_remote_transaction we aren't even sure if the transaction spends our funding transaction or if its just some random transaction that had the right bits set in its input-0-nSequence and lock time to make it match our commitment number xor check. I guess in the non-watchtower case it should be the case that self.funding_txo.is_some() which would mean we do know for sure, but to keep things consistent its easier to just not assume that here.

Copy link

Choose a reason for hiding this comment

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

Ah yes you're right, in non-watchtower were we don't have funding_txo set (and can't assume we face commitment tx) this could generate falses-positives

match self.key_storage {
KeyStorage::PrivMode { ref payment_base_key, .. } => {
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key);
if let Ok(local_key) = chan_utils::derive_private_key(&self.secp_ctx, &per_commitment_point, &payment_base_key) {
spendable_outputs.push(SpendableOutputDescriptor::DynamicOutputP2WPKH {
outpoint: BitcoinOutPoint { txid: commitment_txid, vout: idx as u32 },
key: local_key,
output: outp.clone(),
});
}
}
KeyStorage::SigsMode { .. } => {
//TODO: we need to ensure an offline client will generate the event when it
// cames back online after only the watchtower saw the transaction
}
}
} else if Some(&outp.script_pubkey) == local_payment_p2wpkh.as_ref() {
spendable_outputs.push(SpendableOutputDescriptor::DynamicOutputP2WPKH {
outpoint: BitcoinOutPoint { txid: commitment_txid, vout: idx as u32 },
key: local_payment_key.unwrap(),
output: outp.clone(),
});
}
}

Expand Down Expand Up @@ -1090,7 +1081,6 @@ impl ChannelMonitor {
Some(their_htlc_base_key) => ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, revocation_point, &their_htlc_base_key)),
};


for (idx, outp) in tx.output.iter().enumerate() {
if outp.script_pubkey.is_v0_p2wpkh() {
match self.key_storage {
Expand All @@ -1102,11 +1092,8 @@ impl ChannelMonitor {
output: outp.clone(),
});
}
}
KeyStorage::SigsMode { .. } => {
//TODO: we need to ensure an offline client will generate the event when it
// cames back online after only the watchtower saw the transaction
}
},
KeyStorage::SigsMode { .. } => {}
}
break; // Only to_remote ouput is claimable
}
Expand Down Expand Up @@ -2161,10 +2148,10 @@ mod tests {
let logger = Arc::new(TestLogger::new());
let dummy_sig = Signature::from_der(&secp_ctx, &hex::decode("3045022100fa86fa9a36a8cd6a7bb8f06a541787d51371d067951a9461d5404de6b928782e02201c8b7c334c10aed8976a3a465be9a28abff4cb23acbf00022295b378ce1fa3cd").unwrap()[..]).unwrap();

let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&secp_ctx, &[42; 32]).unwrap());
macro_rules! dummy_keys {
() => {
{
let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&secp_ctx, &[42; 32]).unwrap());
TxCreationKeys {
per_commitment_point: dummy_key.clone(),
revocation_key: dummy_key.clone(),
Expand Down Expand Up @@ -2233,10 +2220,10 @@ mod tests {
monitor.set_their_to_self_delay(10);

monitor.provide_latest_local_commitment_tx_info(dummy_tx.clone(), dummy_keys!(), 0, preimages_to_local_htlcs!(preimages[0..10]));
monitor.provide_latest_remote_commitment_tx_info(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655);
monitor.provide_latest_remote_commitment_tx_info(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[15..20]), 281474976710654);
monitor.provide_latest_remote_commitment_tx_info(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[17..20]), 281474976710653);
monitor.provide_latest_remote_commitment_tx_info(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[18..20]), 281474976710652);
monitor.provide_latest_remote_commitment_tx_info(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[5..15]), 281474976710655, dummy_key);
monitor.provide_latest_remote_commitment_tx_info(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[15..20]), 281474976710654, dummy_key);
monitor.provide_latest_remote_commitment_tx_info(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[17..20]), 281474976710653, dummy_key);
monitor.provide_latest_remote_commitment_tx_info(&dummy_tx, preimages_slice_to_htlc_outputs!(preimages[18..20]), 281474976710652, dummy_key);
for &(ref preimage, ref hash) in preimages.iter() {
monitor.provide_payment_preimage(hash, preimage);
}
Expand Down