From 3af20fd507d0e0a62aadb855169e7b066e3f4b39 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 20 Nov 2018 15:09:47 -0500 Subject: [PATCH] Check P2WPKH script against expected before gen'ing an output event This fixes a bug in 3518f1f85d8a3daff451b3fe56cc7854b833e2bd where we may generate an output event for a P2WPKH output which is not ours if the transaction has a sequence/lock_time combination which false-positives our remote tx detection. Also note that the TODO is removed as this should already be covered without issue if the client properly replays the chain on restart. --- src/ln/channelmonitor.rs | 48 ++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/src/ln/channelmonitor.rs b/src/ln/channelmonitor.rs index 39eba2198..6ce4b22d5 100644 --- a/src/ln/channelmonitor.rs +++ b/src/ln/channelmonitor.rs @@ -889,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())); @@ -910,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(); @@ -929,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() { - 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(), + }); } } @@ -1083,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 { @@ -1095,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 } -- 2.39.5