Add KeysInterface trait to handle keys management, first part: when C… #214

ariard · 2018-10-19T02:49:06Z

…hannelMonitor

detects a spendable outputs onchain it will notify back the user wallet
with needed data to spend it with register_spendable_outputs

ariard · 2018-10-19T02:57:05Z

Early work, just to agree on the general KeysInterface thing. I don't follow the event model we talked previously on irc as it doesn't fit well with ChannelMonitor, I've more thought to something like ChainWatchInterface.
I think we have two cases of keys, the ones static, as found in justice tx or preimage tx witness, and the others dynamic, generated from per_commitment_point. For the last ones, we send back the local per_commitment_secret from which the user wallet can derive the priv_key, we assume it keeps knowledge of the basepoint_privatekey.

(note I commented an assert in test_utils, because of hanging of ChannelMonintor serialization, which could be changed with ongoing #61, before I'm done here )

yuntai · 2018-10-19T17:38:02Z

src/ln/channelmonitor.rs

+			writer.write_all(&[0;32])?;
+
+		}
+


you don't want to persist secret? also, seems it's better to re-sign local_commitment_tx can when it's loaded onto the memory and strip off signature when it's stored into hard disk.

If I get derivation scheme right you can't do nothing with a per_commitment_point, you still need basepoint and basepoint_secret? ChannelMonitor handling current per_commitment_point is to avoid KeysManager or whatsever having to remember the per_commitment_point for each new commitment, without knowing if it's gonna be useful.
Hard disk storage thing is under revision, so here it's temporary.

Definitely do want to persist, but you should only do it in case of for_local_storage.

TheBlueMatt · 2018-10-19T18:42:15Z

Thanks for the quick work on this one! The nice thing about Events is that they are inherintly asynchronous and we don't have to worry about lock inversions between rust-lightning bits and client bits. Making a callback for something that is asynchronous just feels like asking for more complexity than we need there. What about just making SimpleManyChannelMonitor an EventsProvider?

ariard · 2018-10-23T02:10:09Z

Rebased, with EventsProvider model, with two Interfaces: ln --> wallet and wallet --> ln, still WIP.

TheBlueMatt

High-level interface looks good!

src/util/events.rs

src/ln/msgs.rs

TheBlueMatt · 2018-10-23T18:37:00Z

src/ln/channelmonitor.rs


+					if let Some(ref per_commitment_point) = *per_commitment_point {
+						spendable_outputs.push((Some(per_commitment_point.clone()), redeemscript, BitcoinOutPoint { txid: htlc_success_tx.txid(), vout: 0 }));


The script here is a bit confused - sometimes its the redeemscript sometimes its the script itself. Also, a script isn't really the best way to tell a user how to spend an output - I think an enum (that might contain data, eg a redeemscript) may be more useful.

Hmm something like output script descriptor seems to be the right solution ? But it's still not into rust-bitcoin..

Indeed, though I dont think the descriptor stuff is always sufficiently expressive for custom scripts. A simple enum with clear docs that explain what you have to put in the witness stack for the spend to succeed should be sufficient.

src/ln/channelmonitor.rs

TheBlueMatt · 2018-10-23T18:41:22Z

src/ln/channelmonitor.rs

@@ -226,6 +246,9 @@ pub struct ChannelMonitor {
 	prev_local_signed_commitment_tx: Option<LocalSignedTx>,
 	current_local_signed_commitment_tx: Option<LocalSignedTx>,

+	prev_latest_per_commitment_point: Option<SecretKey>,


I dont think we actually need these right now? They are only used for the local htlc tx case, but I think we actually want the delayed_payment_key there, which we derive.

The user wallet has to have access to local_delay key. We can derive this one from delayed_payment_basepoint and local per_commitment_point, but actually ChannelMonitor doesn't have any of them ? So I think we need them and also add delayed_payment_basepoint in KeyStorage ? If so it's nice for the user wallet because it hasn't to be aware of LN derivation scheme, it just gets the right key

Do we need more than the local_delayedkey anywhere? Maybe only store that?

Nop right only need local_delayedkey for dynamic outputs, we store it only in PrivMode ? How a user will be able to spend if watchtower's channel monitor only send him back local_delayedpubkey ?

ariard · 2018-10-24T01:59:29Z

Hmm still working on it, should we provide destination_script as a P2WPKH with a key derive from seed or let the user provide us destination_script (multisig, ...) ? Seems 2nd is better.

For shutdown_scriptpubkey, following BOLT 2, there is 4 forms, we pick one by default for user ?

TheBlueMatt · 2018-10-24T19:10:37Z

I think ideally for shutdown_scriptpubkey and destination_script we'd derive something from a user-provided secret (eg just do an HD derivation from a user-provided HD master key). If we double-derive on accident and reuse them we'll just have slightly less privacy but that's OK cause we already exposed a persistent identifier via our node_id.

ariard · 2018-10-25T02:08:30Z

Okay rebased with addition of CustomOutputScriptDescriptor. Still lack some integrations of KeysManager in other places and maybe tests to be finished.

Still open : #214 (comment) ? which conditions writer/reader of per_commitment_point and some TODO in ChannelMonitor

fuzz/fuzz_targets/full_stack_target.rs

src/ln/channelmonitor.rs

TheBlueMatt · 2018-10-25T18:57:17Z

src/ln/channelmonitor.rs

@@ -490,6 +519,13 @@ impl ChannelMonitor {
 		self.payment_preimages.insert(payment_hash.clone(), payment_preimage.clone());
 	}

+	/// Provides latest local per_commitment_point secret. Useful to derive private key for protocol exit points based
+	/// on local commitment tx.
+	pub(super) fn provide_latest_per_commitment_point(&mut self, per_commitment_point: SecretKey) {


This is currently never called, but I think you should be able to add the info to provide_latest_local_commitment_tx_info and move the fields into LocalTx, no?

TheBlueMatt · 2018-10-25T18:58:16Z

src/ln/channelmonitor.rs

+			writer.write_all(&[0;32])?;
+
+		}
+


Definitely do want to persist, but you should only do it in case of for_local_storage.

src/ln/keysinterface.rs

TheBlueMatt · 2018-10-25T19:12:23Z

src/ln/keysinterface.rs

+pub enum CustomOutputScriptDescriptor {
+	/// Outpoint commits to a P2PWKH, should be spend by the following witness :
+	/// <signature> <pubkey> OP_CHECKSIG
+	StaticOutput {


Shouldn't this include the publickey so that the user knows which pubkey to use?

Yes updated with "With pubkey being bip32 /1' from HMAC-Sha512 of user-provided seed as master private key" ?

Well KeysInterface is just an interface so would have to specify it as "get_destination_script() return value", but I was thinking more because a user might find it easier to see the pubkey here as it would allow them to claim the output without having to go look up the output on-chain - they could just create a transaction from only the data in the event.

TheBlueMatt · 2018-10-25T19:15:20Z

src/ln/channelmonitor.rs

@@ -1446,6 +1518,9 @@ impl<R: ::std::io::Read> Readable<R> for ChannelMonitor {
 			_ => return Err(DecodeError::InvalidValue),
 		};

+		let prev_latest_per_commitment_point = Some(unwrap_obj!(SecretKey::from_slice(&secp_ctx, read_bytes!(32))));


Can't unwrap_obj! if it was optional at write-time.

src/ln/keysinterface.rs

TheBlueMatt · 2018-10-25T19:18:09Z

src/util/test_utils.rs

@@ -55,7 +55,7 @@ impl channelmonitor::ManyChannelMonitor for TestChannelMonitor {
 		// to a watchtower and disk...
 		let mut w = VecWriter(Vec::new());
 		monitor.write_for_disk(&mut w).unwrap();
-		assert!(channelmonitor::ChannelMonitor::read(&mut ::std::io::Cursor::new(&w.0)).unwrap() == monitor);
+		//assert!(channelmonitor::ChannelMonitor::read(&mut ::std::io::Cursor::new(&w.0)).unwrap() == monitor);


I dont think you need this anymore with the current code (at least if you remove the unwrap_obj that I flagged)?

… ChannelMonitor detects a spendable outputs onchain it will send back an Event to user wallet with needed data to be at disposal Add CustomOutputScriptDescriptor to ease user wallet spending of onchain lightning outputs Extend KeyStorage with delayed_payment_base_key and per_commitment_point to derive local_delayed private key

componenents, responsible to get seed from the former and derive appropriate keyring materials Move ChannelKeys into keysinterface for generate a set of it from master_seed

Drop channel_close_key from ChannelKeys

ChannelManager/Channel Drop channel_monitor_claim_key from ChannelKeys

TheBlueMatt · 2018-10-26T14:37:45Z

fuzz/fuzz_targets/full_stack_target.rs

-	let router = Arc::new(Router::new(PublicKey::from_secret_key(&secp_ctx, &our_network_key), watch.clone(), Arc::clone(&logger)));
+	let mut seed = [0; 32];
+	let mut rng = thread_rng();
+	rng.fill_bytes(&mut seed);


fuzz targets must be fully deterministic - if you need to read bytes, read them from the input buffer, though ideally you'd keep consistency with the existing input data.

TheBlueMatt · 2018-10-26T17:56:50Z

Will take as #225.

yuntai reviewed Oct 19, 2018

View reviewed changes

TheBlueMatt added this to the 0.0.6 milestone Oct 20, 2018

ariard force-pushed the 81-key-management branch from 83d8947 to 96567d8 Compare October 23, 2018 02:09

ariard force-pushed the 81-key-management branch from 96567d8 to 4c53b79 Compare October 23, 2018 15:43

TheBlueMatt reviewed Oct 23, 2018

View reviewed changes

ariard force-pushed the 81-key-management branch 5 times, most recently from a1ea971 to 7b4d1f2 Compare October 25, 2018 02:04

TheBlueMatt reviewed Oct 25, 2018

View reviewed changes

ariard force-pushed the 81-key-management branch 2 times, most recently from 177956e to 8e7ec6d Compare October 26, 2018 02:55

ariard force-pushed the 81-key-management branch from 8e7ec6d to 981a9ed Compare October 26, 2018 03:30

Antoine Riard added 3 commits October 26, 2018 12:16

Add a KeysInterface to bridge between user wallet and others lightning

5cc6a77

componenents, responsible to get seed from the former and derive appropriate keyring materials Move ChannelKeys into keysinterface for generate a set of it from master_seed

Implement KeysInterface for KeysManager util

ea98741

Integrate KeysInterface to ChannelManager with Arc

3a8c01a

ariard force-pushed the 81-key-management branch from 981a9ed to 3a8c01a Compare October 26, 2018 13:05

Antoine Riard added 2 commits October 26, 2018 13:31

Integrate shutdown_pubkey from KeysInterface in ChannelManager/Channel

252f22f

Drop channel_close_key from ChannelKeys

Integrate destination_script from KeysInterface in

c814790

ChannelManager/Channel Drop channel_monitor_claim_key from ChannelKeys

TheBlueMatt reviewed Oct 26, 2018

View reviewed changes

TheBlueMatt mentioned this pull request Oct 26, 2018

Keys Interface Simplification #225

Merged

TheBlueMatt closed this Oct 26, 2018


		if let Some(ref per_commitment_point) = *per_commitment_point {
		spendable_outputs.push((Some(per_commitment_point.clone()), redeemscript, BitcoinOutPoint { txid: htlc_success_tx.txid(), vout: 0 }));

Add KeysInterface trait to handle keys management, first part: when C… #214

Add KeysInterface trait to handle keys management, first part: when C… #214

Uh oh!

Conversation

ariard commented Oct 19, 2018

Uh oh!

ariard commented Oct 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 19, 2018

Uh oh!

ariard commented Oct 23, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Oct 24, 2018

Uh oh!

TheBlueMatt commented Oct 24, 2018

Uh oh!

ariard commented Oct 25, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 26, 2018

Uh oh!

Uh oh!

ariard Oct 26, 2018 •

edited

Loading