Skip to content

Creating fresh destination scripts for every channel in KeysManager #2174

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

Closed
wants to merge 1 commit into from
Closed

Creating fresh destination scripts for every channel in KeysManager #2174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ impl SignerProvider for KeyProvider {
})
}

fn get_destination_script(&self) -> Script {
fn get_destination_script(&self, _channel_keys_id : [u8; 32]) -> Script {
let secp_ctx = Secp256k1::signing_only();
let channel_monitor_claim_key = SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, self.node_secret[31]]).unwrap();
let our_channel_monitor_claim_key_hash = WPubkeyHash::hash(&PublicKey::from_secret_key(&secp_ctx, &channel_monitor_claim_key).serialize());
Expand Down
2 changes: 1 addition & 1 deletion fuzz/src/full_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ impl SignerProvider for KeyProvider {
))
}

fn get_destination_script(&self) -> Script {
fn get_destination_script(&self, _channel_keys_id : [u8; 32]) -> Script {
let secp_ctx = Secp256k1::signing_only();
let channel_monitor_claim_key = SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap();
let our_channel_monitor_claim_key_hash = WPubkeyHash::hash(&PublicKey::from_secret_key(&secp_ctx, &channel_monitor_claim_key).serialize());
Expand Down
2 changes: 1 addition & 1 deletion fuzz/src/onion_message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ impl SignerProvider for KeyProvider {

fn read_chan_signer(&self, _data: &[u8]) -> Result<EnforcingSigner, DecodeError> { unreachable!() }

fn get_destination_script(&self) -> Script { unreachable!() }
fn get_destination_script(&self, _channel_keys_id : [u8; 32]) -> Script { unreachable!() }

fn get_shutdown_scriptpubkey(&self) -> ShutdownScript { unreachable!() }
}
Expand Down
2 changes: 2 additions & 0 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3683,6 +3683,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
spendable_output = Some(SpendableOutputDescriptor::StaticOutput {
outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
output: outp.clone(),
channel_keys_id: Some(self.channel_keys_id),
});
break;
}
Expand Down Expand Up @@ -3713,6 +3714,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
spendable_output = Some(SpendableOutputDescriptor::StaticOutput {
outpoint: OutPoint { txid: tx.txid(), index: i as u16 },
output: outp.clone(),
channel_keys_id: Some(self.channel_keys_id),
});
break;
}
Expand Down
76 changes: 53 additions & 23 deletions lightning/src/chain/keysinterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@ pub enum SpendableOutputDescriptor {
outpoint: OutPoint,
/// The output which is referenced by the given outpoint.
output: TxOut,
/// Arbitrary identification information returned by a call to
/// `Sign::channel_keys_id()`. This may be useful in re-deriving keys used in
/// the channel to spend the output.
channel_keys_id: Option<[u8; 32]>
},
/// An output to a P2WSH script which can be spent with a single signature after an `OP_CSV`
/// delay.
Expand Down Expand Up @@ -208,6 +212,7 @@ impl_writeable_tlv_based_enum!(SpendableOutputDescriptor,
(0, StaticOutput) => {
(0, outpoint, required),
(2, output, required),
(3, channel_keys_id, option),
},
;
(1, DelayedPaymentOutput),
Expand Down Expand Up @@ -545,7 +550,7 @@ pub trait SignerProvider {
///
/// This method should return a different value each time it is called, to avoid linking
/// on-chain funds across channels as controlled to the same user.
fn get_destination_script(&self) -> Script;
fn get_destination_script(&self, channel_keys_id: [u8; 32]) -> Script;

/// Get a script pubkey which we will send funds to when closing a channel.
///
Expand Down Expand Up @@ -711,7 +716,7 @@ impl InMemorySigner {
if spend_tx.input[input_idx].previous_output != descriptor.outpoint.into_bitcoin_outpoint() { return Err(()); }

let remotepubkey = self.pubkeys().payment_point;
let witness_script = bitcoin::Address::p2pkh(&::bitcoin::PublicKey{compressed: true, inner: remotepubkey}, Network::Testnet).script_pubkey();
let witness_script = bitcoin::Address::p2pkh(&bitcoin::PublicKey{compressed: true, inner: remotepubkey}, Network::Testnet).script_pubkey();
let sighash = hash_to_message!(&sighash::SighashCache::new(spend_tx).segwit_signature_hash(input_idx, &witness_script, descriptor.output.value, EcdsaSighashType::All).unwrap()[..]);
let remotesig = sign_with_aux_rand(secp_ctx, &sighash, &self.payment_key, &self);
let payment_script = bitcoin::Address::p2wpkh(&::bitcoin::PublicKey{compressed: true, inner: remotepubkey}, Network::Bitcoin).unwrap().script_pubkey();
Expand Down Expand Up @@ -1014,9 +1019,10 @@ pub struct KeysManager {
node_id: PublicKey,
inbound_payment_key: KeyMaterial,
destination_script: Script,
shutdown_pubkey: PublicKey,
shutdown_pubkey: ExtendedPubKey,
channel_master_key: ExtendedPrivKey,
channel_child_index: AtomicUsize,
disallow_downgrades: bool,

rand_bytes_unique_start: [u8; 32],
rand_bytes_index: AtomicCounter,
Expand Down Expand Up @@ -1061,7 +1067,7 @@ impl KeysManager {
Err(_) => panic!("Your RNG is busted"),
};
let shutdown_pubkey = match master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(2).unwrap()) {
Ok(shutdown_key) => ExtendedPubKey::from_priv(&secp_ctx, &shutdown_key).public_key,
Ok(shutdown_key) => ExtendedPubKey::from_priv(&secp_ctx, &shutdown_key),
Err(_) => panic!("Your RNG is busted"),
};
let channel_master_key = master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(3).unwrap()).expect("Your RNG is busted");
Expand All @@ -1087,6 +1093,7 @@ impl KeysManager {

channel_master_key,
channel_child_index: AtomicUsize::new(0),
disallow_downgrades: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be a parameter/configurable.


rand_bytes_unique_start,
rand_bytes_index: AtomicCounter::new(),
Expand Down Expand Up @@ -1202,7 +1209,7 @@ impl KeysManager {
input_value += descriptor.output.value;
if !output_set.insert(descriptor.outpoint) { return Err(()); }
},
SpendableOutputDescriptor::StaticOutput { ref outpoint, ref output } => {
SpendableOutputDescriptor::StaticOutput { ref outpoint, ref output, .. } => {
input.push(TxIn {
previous_output: outpoint.into_bitcoin_outpoint(),
script_sig: Script::new(),
Expand Down Expand Up @@ -1245,17 +1252,22 @@ impl KeysManager {
}
spend_tx.input[input_idx].witness = Witness::from_vec(keys_cache.as_ref().unwrap().0.sign_dynamic_p2wsh_input(&spend_tx, input_idx, &descriptor, &secp_ctx)?);
},
SpendableOutputDescriptor::StaticOutput { ref output, .. } => {
SpendableOutputDescriptor::StaticOutput { ref output, channel_keys_id, .. } => {
let destination_script = channel_keys_id.map_or( None,|s| Some(self.get_destination_script(s)));
let derivation_idx = if output.script_pubkey == self.destination_script {
1
} else {
} else if Some(&output.script_pubkey) == destination_script.as_ref() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced the logic here is right, the destination script in the output should always be what get_destination_script gives us, so we need to use the derivation idx as calculated there, not just always use the channel_keys_id one here.

u32::from_be_bytes(channel_keys_id.unwrap()[0..4].try_into().unwrap())
}
else {
2
};
let secret = {

let secret = {
// Note that when we aren't serializing the key, network doesn't matter
match ExtendedPrivKey::new_master(Network::Testnet, &self.seed) {
Ok(master_key) => {
match master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx(derivation_idx).expect("key space exhausted")) {
match master_key.ckd_priv(&secp_ctx, ChildNumber::from_hardened_idx((derivation_idx) % (1 << 31)).expect("key space exhausted")) {
Ok(key) => key,
Err(_) => panic!("Your RNG is busted"),
}
Expand All @@ -1265,7 +1277,7 @@ impl KeysManager {
};
let pubkey = ExtendedPubKey::from_priv(&secp_ctx, &secret).to_pub();
if derivation_idx == 2 {
assert_eq!(pubkey.inner, self.shutdown_pubkey);
assert_eq!(pubkey.inner, self.shutdown_pubkey.public_key);
}
let witness_script = bitcoin::Address::p2pkh(&pubkey, Network::Testnet).script_pubkey();
let payment_script = bitcoin::Address::p2wpkh(&pubkey, Network::Testnet).expect("uncompressed key found").script_pubkey();
Expand Down Expand Up @@ -1302,6 +1314,10 @@ impl EntropySource for KeysManager {
}

impl NodeSigner for KeysManager {
fn get_inbound_payment_key_material(&self) -> KeyMaterial {
self.inbound_payment_key.clone()
}

fn get_node_id(&self, recipient: Recipient) -> Result<PublicKey, ()> {
match recipient {
Recipient::Node => Ok(self.node_id.clone()),
Expand All @@ -1320,10 +1336,6 @@ impl NodeSigner for KeysManager {
Ok(SharedSecret::new(other_key, &node_secret))
}

fn get_inbound_payment_key_material(&self) -> KeyMaterial {
self.inbound_payment_key.clone()
}

fn sign_invoice(&self, hrp_bytes: &[u8], invoice_data: &[u5], recipient: Recipient) -> Result<RecoverableSignature, ()> {
let preimage = construct_invoice_preimage(&hrp_bytes, &invoice_data);
let secret = match recipient {
Expand Down Expand Up @@ -1366,12 +1378,30 @@ impl SignerProvider for KeysManager {
InMemorySigner::read(&mut io::Cursor::new(reader), self)
}

fn get_destination_script(&self) -> Script {
self.destination_script.clone()
fn get_destination_script(&self, channel_keys_id: [u8; 32]) -> Script {
let derivation_idx = if self.disallow_downgrades {
u32::from_be_bytes(channel_keys_id[0..4].try_into().unwrap())
} else {
3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Destination key was 1 on current git, so 3 is a totally different key.

};
match ExtendedPrivKey::new_master(Network::Testnet, &self.seed) {
Ok(master_key) => {
match master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx((derivation_idx) % (1 << 31)).expect("key space exhausted")) {
Ok(destination_key) => {
let wpubkey_hash = WPubkeyHash::hash(&ExtendedPubKey::from_priv(&self.secp_ctx, &destination_key).to_pub().to_bytes());
Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0)
.push_slice(&wpubkey_hash.into_inner())
.into_script()
},
Err(_) => panic!("Your RNG is busted"),
}
}
Err(_) => panic!("Your RNG is busted")
}
}

fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
ShutdownScript::new_p2wpkh_from_pubkey(self.shutdown_pubkey.clone())
ShutdownScript::new_p2wpkh_from_pubkey(self.shutdown_pubkey.public_key.clone())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also make this different per channel too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will deriving from the existing shutdown_pubkey at some fixed index and creating a shutdown script work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd think we just use the same derivation logic (effectively merging destination and shutdown scripts).

}
}

Expand Down Expand Up @@ -1410,6 +1440,10 @@ impl EntropySource for PhantomKeysManager {
}

impl NodeSigner for PhantomKeysManager {
fn get_inbound_payment_key_material(&self) -> KeyMaterial {
self.inbound_payment_key.clone()
}

fn get_node_id(&self, recipient: Recipient) -> Result<PublicKey, ()> {
match recipient {
Recipient::Node => self.inner.get_node_id(Recipient::Node),
Expand All @@ -1428,10 +1462,6 @@ impl NodeSigner for PhantomKeysManager {
Ok(SharedSecret::new(other_key, &node_secret))
}

fn get_inbound_payment_key_material(&self) -> KeyMaterial {
self.inbound_payment_key.clone()
}

fn sign_invoice(&self, hrp_bytes: &[u8], invoice_data: &[u5], recipient: Recipient) -> Result<RecoverableSignature, ()> {
let preimage = construct_invoice_preimage(&hrp_bytes, &invoice_data);
let secret = match recipient {
Expand Down Expand Up @@ -1461,8 +1491,8 @@ impl SignerProvider for PhantomKeysManager {
self.inner.read_chan_signer(reader)
}

fn get_destination_script(&self) -> Script {
self.inner.get_destination_script()
fn get_destination_script(&self, channel_keys_id: [u8; 32]) -> Script {
self.inner.get_destination_script(channel_keys_id)
}

fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
Expand Down
8 changes: 5 additions & 3 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {

let mut secp_ctx = Secp256k1::new();
secp_ctx.seeded_randomize(&entropy_source.get_secure_random_bytes());
let channel_keys_id = holder_signer.channel_keys_id();

let shutdown_scriptpubkey = if config.channel_handshake_config.commit_upfront_shutdown_pubkey {
Some(signer_provider.get_shutdown_scriptpubkey())
Expand Down Expand Up @@ -1021,7 +1022,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {

holder_signer,
shutdown_scriptpubkey,
destination_script: signer_provider.get_destination_script(),
destination_script: signer_provider.get_destination_script(channel_keys_id),

cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
Expand Down Expand Up @@ -1344,6 +1345,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {

let mut secp_ctx = Secp256k1::new();
secp_ctx.seeded_randomize(&entropy_source.get_secure_random_bytes());
let channel_keys_id = holder_signer.channel_keys_id();

let chan = Channel {
user_id,
Expand All @@ -1368,7 +1370,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {

holder_signer,
shutdown_scriptpubkey,
destination_script: signer_provider.get_destination_script(),
destination_script: signer_provider.get_destination_script(channel_keys_id),

cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
Expand Down Expand Up @@ -7087,7 +7089,7 @@ mod tests {

fn read_chan_signer(&self, _data: &[u8]) -> Result<Self::Signer, DecodeError> { panic!(); }

fn get_destination_script(&self) -> Script {
fn get_destination_script(&self, _channel_keys_id: [u8; 32]) -> Script {
let secp_ctx = Secp256k1::signing_only();
let channel_monitor_claim_key = SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap();
let channel_monitor_claim_key_hash = WPubkeyHash::hash(&PublicKey::from_secret_key(&secp_ctx, &channel_monitor_claim_key).serialize());
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/util/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ impl SignerProvider for OnlyReadsKeysInterface {
))
}

fn get_destination_script(&self) -> Script { unreachable!(); }
fn get_destination_script(&self, _channel_keys_id: [u8; 32]) -> Script { unreachable!(); }
fn get_shutdown_scriptpubkey(&self) -> ShutdownScript { unreachable!(); }
}

Expand Down Expand Up @@ -809,7 +809,7 @@ impl SignerProvider for TestKeysInterface {
))
}

fn get_destination_script(&self) -> Script { self.backing.get_destination_script() }
fn get_destination_script(&self, channel_keys_id: [u8; 32]) -> Script { self.backing.get_destination_script(channel_keys_id) }

fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
match &mut *self.expectations.lock().unwrap() {
Expand Down