Handle sizeable push msat (fix #195) + handle two first per_commitment_point + keys interface tests #230

ariard · 2018-10-31T03:03:22Z

Thanks to have pointed to me #195, I think I spotted a case which wasn't covered by SpendableOuput event generation, to_local output on local commitment tx in broadcast_by_state.
Should fix #195, at funding_created, we sign local_commitment_tx and give it to both channel_manager and channel_monitor.

ariard · 2018-11-01T03:41:24Z

Rebased, add solution for #228 and new test which covered both the latter and also one of spendable_output event generation cases.

yuntai · 2018-11-01T04:16:53Z

src/ln/channel.rs

 		secp_call!(self.secp_ctx.verify(&local_sighash, &sig, &self.their_funding_pubkey.unwrap()), "Invalid funding_created signature from peer", self.channel_id());

+		// ...and we sign it, allowing us ot broadcast the tx if we wish


nit: s/ot/to

Fixed, and also some others nits

ariard · 2018-11-02T02:36:27Z

Rebased, implemented the generation of SpendableOutput event for to_remote_output on remote commitment tx, in both branches of check_spend_remote_transaction.

I modified witness_script of DynamicOutput as Option, because in this case Script isn't a P2WSH but the secret key is still generate based on per_commitment_point.
And for so, added payment_base_key in private KeyStorage

Still some tests lacking before to consider the PR complete.

TheBlueMatt

Please limit your commits to having a short (no more than 80ish chars) title, followed by an empty line, then text that is also line-broken to 80ish chars.

TheBlueMatt · 2018-11-03T01:28:46Z

src/ln/channel.rs

@@ -1496,6 +1498,7 @@ 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)));


Hmm, I think this split may be simpler by just moving the their_next_revocation_point tracking into provide_latest_remote_commitment_tx_info. We only ever use the revocation point in case we have remote_tx_info available corresponding to it, so it should be way simpler to just move it there.

Not sure for this one... firstly we get per_commitment_point from remote at time we don't/didn't have generate remote commitment tx (accept_channel/new_from_req/funding_locked) and secondly we may use revocation point to generate pubkeys without per_commitment_option (htlcs_outputs) in case of sizeable push_msat?

Hmm? You mean you intend to add some use for their_cur_revocation_points beyond what its used for now, right now its only ever used in conjunction with remote_claimable_outpoints data.

Yes exactly, see deacb3f, in case of to_remote ouput you don't need remote_claimable_outpoints but you still need per_commitment_point to derive your remoteprivatekey. If I get derivation scheme right, but please review it thoroughly, that's easy to mess this part

src/ln/channelmonitor.rs

ariard · 2018-11-03T03:04:28Z

(side-note reviewing shutdown let me thought that maybe I should add a check_shutdown_tx in block_connected to generate spendable output for shutdown pubkey)

ariard · 2018-11-04T02:30:09Z

Rebased with corrections at deacb3f, noted for commits formatting.

ariard · 2018-11-05T03:04:34Z

Added new tests, still more coming, for code hygiene will prune redundant test branches with macro and some others nits.

ariard · 2018-11-06T03:27:40Z

Note : 7e07763 was just a quick solution to get test added in 9bebe2b functional, will try to clean out relative TODO in insert_combine

ariard · 2018-11-07T02:06:38Z

PR good to a second review, tests should cover almost all keys interface spendable events !

ariard · 2018-11-07T02:08:12Z

src/ln/channelmonitor.rs

+					self.key_storage = other.key_storage;
+				}
+			}
+		}
 		// TODO: We should use current_remote_commitment_number and the commitment number out of


What is the right merge strategy on this one ? If current_remote_commitment_number < our_commitment_number, we keep other data parts right ?

TheBlueMatt

This looks really good, sorry for the review delay!

TheBlueMatt · 2018-11-08T00:32:12Z

src/ln/channelmanager.rs

@@ -7490,4 +7495,413 @@ mod tests {
 			assert_eq!(msg.channel_id, channel_id);
 		} else { panic!("Unexpected result"); }
 	}
+
+	macro_rules! check_dynamic_output {
+		($event: expr, $event_idx: expr, $output_idx: expr, $value: expr) => {


Looks like $event_idx is always 0 and $event always just has a vec of one or two of the same event. May be cleaner (and more restrictive on behavior) if you just pass in the Node and it can check that the only event(s) are identical SpendableOutputs events (would also simplify the callsites).

Done at 892690b

TheBlueMatt · 2018-11-08T00:54:06Z

src/ln/channelmanager.rs

+		let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+		nodes[1].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![node_txn[0].clone()] }, 0);
+		let events = nodes[1].chan_monitor.simple_monitor.get_and_clear_pending_events();
+		let spend_tx = check_dynamic_output!(events, 0, 0, node_txn[0].output[1].value);


Oops, SpendableOutputDescriptor needs the value so that you can sign for it without the TxOut.

Yes, I arrived at the same conclusion, my bad, currently rebasing for it

src/chain/keysinterface.rs

TheBlueMatt · 2018-11-08T22:40:48Z

src/ln/channelmonitor.rs

+	/// back two descriptors, so even a false positive one for remote output to avoid watchtower mode
+	/// learning channel balance.
+	fn check_spend_closing_transaction(&self, tx: &Transaction, _height: u32) -> Vec<SpendableOutputDescriptor> {
+		if tx.input[0].sequence == 0xFFFFFFFF && tx.input[0].witness.last().unwrap().len() == 71 {


Hmm, wont this match, like, everything? I think we should just say get_shutdown_pubkey() and get_destination_script()-returned scripts won't be provided out as SpendableOutputDescriptors.

I think it's great to relieve users from any chain watching of LN kind of txs ? If not seems to me they have to implement some LN stuff to know it and it lessens keys interface out-of-box value. The other kind of tx which can spend the funding outpoint is commitment tx and the nSequence of their first input is set to an obfuscated commitment number so can't match I would say, but yes I can add heuristic rules by security.

I mean they should already have logic for watching an output to a script they generated, the only reason to tell them about outputs is when there is other data mixed in.

Aha yes make sense well if so I believe we can get ride of StaticOutputDescriptor (and its tests) ?

Hmmmmm...that may be the case. Indeed, I guess if we can't provide all of them we might as well provide none of them and at least for this case providing an event for it seems...dubious? I suppose we could generate them completely by comparing this output to addresses we've seen, obviously so that only non-watchtowers can see them.

In fact, checking against bolt, matching is sure (closing tx is the only spending_tx from funding with nSequence set to 0xFFFFFFFF). Don't know if you see my IRC comment but I think my point on offline wallet was irrelevant, detecting this case of output it's still a job handable by ChainWatchInterface if it got destination_script (and if user-provided one want to use a different format descriptor than our, we shouldn't force it to handle our crafted StaticOutputDescriptor). So yes I think that StaticOutputDescriptor can go, the only to reason to keep it could be a performance one (maybe easier to us to filter it, but you know better than me).
For privacy, a rogue watchtower still have the funding_txo and can spot when it spent no ?

Ahh, there's the confusion. So I forgot about the funding input check in block_connected. You should only call check_spend_closing_transaction when the funding_txo is set, though. Still, false-positives in output events really sucks, maybe we can generate these events in Channel in closing_signed handling instead?

So on the idea you're ok to keep StaticOutputDescriptor ?

So implemented with closing_signed as of 6fc8391, should avoid to false-positives due to non-definitive closing_signed message exchanged.

src/ln/channelmanager.rs

TheBlueMatt · 2018-11-14T00:15:23Z

src/chain/keysinterface.rs

+		/// localkey = payment_basepoint_secret + SHA256(per_commitment_point || payment_basepoint
+		key: SecretKey,
+		/// witness redeemScript encumbering output. If None script is a P2WPKH to build from key.
+		witness_script: Option<Script>,


At the risk of piling on late in review, I think I'd kinda prefer if this were just a new enum entry? Its already an enum so its not like we're making handling that much more complicated on the user end, and it rather drastically changes the way you have to sign the transaction. What do you think?

Will do, an enum should let us handle other type of script_pubkey latter.

Ohoh, I meant a new top-level enum entry in SpendableOutputDescriptor. Also to_self_delay only needs to be in the P2WSH/RevokeableOutput entry.

Goal to claim sizeable push_msat and in event of local commitment tx being broadcast without htlcs

Aims to cover both claiming of sizeable_push_msat and spendable output generation for to_local output

We needed it to be able to track remote_per_commitment_point after channel opening and funds locking

ariard · 2018-11-16T00:33:10Z

Rebased, added AddressType to describe DynamicOutput as comment suggested.

Aims to covered both keysinterace preimage tx case and detection of second remote commitment tx Split DynamicDescriptor between *P2WSH and *P2WKH

Aims to send back closing output descriptor to user wallet

Contrary to sizeable push_msat on local commitment tx, the output go to a P2WPKH

Cover both HTLC-Timeout/Success cases

Based on commitment_number

Cover both local HTLC-Timeout/Success case

TheBlueMatt · 2018-11-20T20:51:21Z

Gonna merge this cause its waited long enough, but I have one bugfix and one cleanup to add on top that I'll PR in a sec.

Two post-#230 fixups

ariard force-pushed the handle_sizeable_push_msat branch from fb533c2 to 01c9168 Compare November 1, 2018 03:39

ariard changed the title ~~Handle sizeable push msat (fix #195)~~ Handle sizeable push msat (fix #195) + handle two first per_commitment_point + keys interface tests Nov 1, 2018

yuntai reviewed Nov 1, 2018

View reviewed changes

ariard force-pushed the handle_sizeable_push_msat branch 3 times, most recently from d3ed15d to 92090bd Compare November 2, 2018 02:32

TheBlueMatt reviewed Nov 3, 2018

View reviewed changes

ariard force-pushed the handle_sizeable_push_msat branch from 92090bd to deacb3f Compare November 4, 2018 02:28

ariard force-pushed the handle_sizeable_push_msat branch from 9bebe2b to 0ebdc9d Compare November 7, 2018 02:00

ariard commented Nov 7, 2018

View reviewed changes

TheBlueMatt reviewed Nov 8, 2018

View reviewed changes

ariard force-pushed the handle_sizeable_push_msat branch 2 times, most recently from 81a1c03 to 892690b Compare November 11, 2018 21:53

TheBlueMatt reviewed Nov 14, 2018

View reviewed changes

ariard force-pushed the handle_sizeable_push_msat branch from 892690b to 6fc8391 Compare November 15, 2018 03:28

Antoine Riard added 5 commits November 15, 2018 19:07

Implement spendable output to_local output on local commitment tx

04e6be9

Track local_commitment_tx at funding_created

8c638ff

Goal to claim sizeable push_msat and in event of local commitment tx being broadcast without htlcs

Add TxOut in DynamicOutput

f88f826

Add test_claim_sizeable_push_msat

f053169

Aims to cover both claiming of sizeable_push_msat and spendable output generation for to_local output

Split provide_their_next_commitment_point from provide_secret

1b33064

We needed it to be able to track remote_per_commitment_point after channel opening and funds locking

ariard force-pushed the handle_sizeable_push_msat branch from 6fc8391 to 1485a85 Compare November 16, 2018 00:32

ariard force-pushed the handle_sizeable_push_msat branch from 1485a85 to 1092e95 Compare November 17, 2018 02:09

Antoine Riard added 8 commits November 19, 2018 20:39

Add test_static_spendable_outputs_preimage_tx

3518f1f

Aims to covered both keysinterace preimage tx case and detection of second remote commitment tx Split DynamicDescriptor between *P2WSH and *P2WKH

Add check_spend_closing_transaction ChannelMonitor

1255885

Aims to send back closing output descriptor to user wallet

Add test_claim_on_remote_sizeable_push_msat

e22220d

Contrary to sizeable push_msat on local commitment tx, the output go to a P2WPKH

Add test_static_spendable_outputs_justice_tx_revoked_commitment_tx

73415a8

Add test_static_spendable_outputs_justice_tx_revoked_htlc*

b6b5dec

Cover both HTLC-Timeout/Success cases

Add key_storage selection in ChannelMonitor insert_combine

394b11c

Based on commitment_number

Add test_dynamic_spendable_outputs_local_htlc_*

890d176

Cover both local HTLC-Timeout/Success case

Add test_static_output_closing_tx

3a7b40e

ariard force-pushed the handle_sizeable_push_msat branch from 1092e95 to 3a7b40e Compare November 20, 2018 01:53

TheBlueMatt mentioned this pull request Nov 20, 2018

Pin rust-lightning dependency to specific revision rust-bitcoin/murmel#14

Closed

TheBlueMatt merged commit 7efaf2e into lightningdevkit:master Nov 20, 2018

TheBlueMatt added this to the 0.0.7 milestone Nov 20, 2018

TheBlueMatt added a commit that referenced this pull request Nov 21, 2018

Merge pull request #255 from TheBlueMatt/2018-11-230-ext

93c8760

Two post-#230 fixups

		secp_call!(self.secp_ctx.verify(&local_sighash, &sig, &self.their_funding_pubkey.unwrap()), "Invalid funding_created signature from peer", self.channel_id());

		// ...and we sign it, allowing us ot broadcast the tx if we wish

Handle sizeable push msat (fix #195) + handle two first per_commitment_point + keys interface tests #230

Handle sizeable push msat (fix #195) + handle two first per_commitment_point + keys interface tests #230

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ariard commented Nov 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Nov 2, 2018

Uh oh!

TheBlueMatt left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ariard commented Nov 3, 2018

Uh oh!

ariard commented Nov 4, 2018

Uh oh!

ariard commented Nov 5, 2018

Uh oh!

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

Uh oh!

Uh oh!

ariard commented Nov 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

ariard commented Nov 16, 2018

Uh oh!

ariard commented Oct 31, 2018 •

edited

Loading

ariard commented Nov 6, 2018 •

edited

Loading