Rename a's keys as local's keys and b's keys as remote's keys #633

ariard · 2020-05-29T00:50:06Z

Downstreaming this while working on anchor, follow-up of #613. This is an attempt to make keys variable names less confusing based on script semantic as an invariant.

Variables should be named according to the script semantic which is
an invariant with regards to generating a local or remote commitment
transaction.

I.e a local_delayed_payment key will always offer to the owner
of the commitment transaction to be paid back after a given delay.

TheBlueMatt · 2020-05-29T00:55:41Z

lightning/src/ln/chan_utils.rs

@@ -374,7 +374,7 @@ impl_writeable!(HTLCOutputInCommitment, 1 + 8 + 4 + 32 + 5, {
 });

 #[inline]
-pub(crate) fn get_htlc_redeemscript_with_explicit_keys(htlc: &HTLCOutputInCommitment, a_htlc_key: &PublicKey, b_htlc_key: &PublicKey, revocation_key: &PublicKey) -> Script {
+pub(crate) fn get_htlc_redeemscript_with_explicit_keys(htlc: &HTLCOutputInCommitment, local_htlc_key: &PublicKey, remote_htlc_key: &PublicKey, revocation_key: &PublicKey) -> Script {


This isnt always the remote/local key though, no? We also call this with the keys swapped to get redeemscripts for their transactions. We probably need to standardize on some kind of nomenclature cause remote/local is often confusing.

From the script layout as described in the BOLT3, how local_htlcpubkey is instanced depends if we are building a transaction for _us _ or for our counterparty.

I think when we generate a transaction, including its scripts, there is no such thing aslocal or remote it's always a local commitment transaction. Now when we sign such transaction and what we sign depends if its a local transaction or remote one.

So, as a nomenclature, I propose:

for all our helpers in chan_utils bind to local/remote to match BOLT3

in channel/channelmonitor label transaction as local_tx or remote_tx (no changes expected)

for holding our or counterparty elements such as their_keys in Channel bind to owner/counterparty to avoid confusion when we instance our helpers.

We may have to move helpers from Channel to chan_utils to make it cleaner.

The naming is definitely contextual. So I would agree that we should not settle on always one way of describing the relationship.

I would like to see "counterparty" used when generally referring to the other side (i.e., when the relationship can be flipped). I don't have a strong opinion on "owner" being the opposite of "counterparty" just yet. It may make sense to not have any prefix in that case. I'd need to see a concrete example.

I'm generally not in favor of using possessive pronouns like "our" and "their" in code. I'd prefer a more descriptive name for the relationship being modeled.

@jkczyz fyi, even the BOLT isn't clear on this lightning/bolts#638

The naming is definitely contextual. So I would agree that we should not settle on always one way of describing the relationship.

I agree, do you see another context that the ones I'm describing, namely chan_utils, Channel, channel/channelmonitor or if those contexts make sense ? Maybe 2) and 3) are the same.

My thinking was that if chan_utils is suppose to be agnostic to what is local and what is remote (at least in some places), then the "local" and "remote" naming may not make sense in those particular contexts.

So for instance, AFAICT, TxCreationKeys should not use the words "local" and "remote" since it is used in both contexts:

https://github.com/rust-bitcoin/rust-lightning/blob/c09c9fb9a00ebaf60cd44755e7a14df1dbb7c7a7/lightning/src/ln/channel.rs#L1102

https://github.com/rust-bitcoin/rust-lightning/blob/c09c9fb9a00ebaf60cd44755e7a14df1dbb7c7a7/lightning/src/ln/channel.rs#L1116

Additionally, there is a lot of redundancy in some of the naming that isn't necessary in the given context. e.g., initial_local_commitment_tx.local_keys.local_htlc_key.

Right there's kind of two different things we'll want two words for:

Something is our's/our counterparty's (I think I like ours/counterpartys or so?)

Something is for the "owner of the transaction" (ie the party that can broadcast it), which is what local/remote mean here.

Its true the BOLTs are a mess in this regard, but thats precisely why we should try to do better.

As a concrete proposal, I think ours/counterpartys and local/remote (which is whats there already in TxCreationKeys) is reasonable, but am open to other ideas.

For ours and counterpartys, I would prefer no prefix and counterparty.

Hmm, guess I never responded to this. I'm really not a fan of no prefix - it seems ripe for confusion. "Yep, code reads right, it says to_self_delay" when its really the wrong "to_self_delay".

codecov · 2020-05-29T01:39:21Z

Codecov Report

Merging #633 into master will decrease coverage by 3.68%.
The diff coverage is 79.31%.

@@            Coverage Diff             @@
##           master     #633      +/-   ##
==========================================
- Coverage   78.40%   74.72%   -3.69%     
==========================================
  Files          55       55              
  Lines       27568    24477    -3091     
==========================================
- Hits        21616    18290    -3326     
- Misses       5952     6187     +235

Impacted Files	Coverage Δ
lightning-c-bindings/src/chain/keysinterface.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/ln/chan_utils.rs	`0.00% <0.00%> (ø)`
lightning-c-bindings/src/ln/channelmonitor.rs	`0.00% <0.00%> (ø)`
lightning/src/ln/channel.rs	`84.27% <ø> (-1.50%)`	⬇️
lightning/src/ln/channelmonitor.rs	`94.83% <ø> (-1.92%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`94.02% <ø> (-0.93%)`	⬇️
lightning/src/ln/channelmanager.rs	`79.80% <90.97%> (-5.72%)`	⬇️
lightning/src/chain/keysinterface.rs	`89.71% <93.61%> (-5.14%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`98.76% <94.11%> (-1.24%)`	⬇️
lightning/src/ln/chan_utils.rs	`90.84% <96.10%> (-5.13%)`	⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 253af8d...21d0a95. Read the comment docs.

ariard · 2020-06-09T00:55:44Z

Updated with a new commit re-labeling fields according to Matt's suggestion: #633 (comment). "Local"/"Remote" references aren't all dead in channel.rs but it kill a lot of them.

Lmk if you find this clearer and worthy of the change.

jkczyz

Updated with a new commit re-labeling fields according to Matt's suggestion: #633 (comment). "Local"/"Remote" references aren't all dead in channel.rs but it kill a lot of them.

Lmk if you find this clearer and worthy of the change.

I'm very close to settling on the naming. But I have a qualm with using "our" which I'll try to articulate better now that my understanding is clearer. I don't think we should ever use the possessive pronoun "our" in identifiers for the same reason we shouldn't use "their": more meaningful names should be used instead of these.

One case is when we need to differentiate roles (e.g., funder/fundee, sender/receiver, local/remote, etc). Having this symmetry in naming is good, IMHO.

The other case is when there are no such roles. I don't think symmetry in naming is needed here. In this instance, "our" refers to the node that this code is implementing and "their" is the node it is communicating or has a channel with.

For the latter case, "counterparty" adequately replaces "their" as it describes a relationship that is relative to the node. Further, the counterparty_ prefix implies that same entity without the prefix is "ours". So, using our_ is redundant and -- to put more plainly -- stating the obvious. So I'd prefer that it be left off entirely.

jkczyz · 2020-06-09T18:28:04Z

lightning/src/ln/chan_utils.rs

-			a_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &a_htlc_base)?,
-			b_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &b_htlc_base)?,
-			a_delayed_payment_key: derive_public_key(&secp_ctx, &per_commitment_point, &a_delayed_payment_base)?,
+			revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &remote_revocation_base)?,


Is remote_ needed on revocation_base? Or put differently, would we ever talk about a local_revocation_base?

We never talk about it, no more we talk about remote_delayedpubkey in script format. If we drop, by consistency we should also drop the local_ prefix for local_delayed_payment_base. Seems a shortcoming of the spec here.

Not updated, I would rather keep the prefix to mark origin of this pubkey with regards to script generation.

Not sure that I follow. What is a shortcoming of the spec? Trying to learn more than anything.

For e.g, if you take offered script with a revocationpubkey. It should have been called remote_revocationpubkey even if there is a contribution from local (per_commitment_point) it's used by remote to punish you. Keys names should be labeled according to their ownership.

jkczyz · 2020-06-09T19:20:46Z

lightning/src/ln/chan_utils.rs


-				let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
+				let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.local_htlc_key, &self.local_keys.remote_htlc_key, &self.local_keys.revocation_key);


Here is where I think there is a bit of redundancy. We have self which is of type LocalCommitmentTransaction with field local_keys which in turn has a local_htlc_key field.

Questions would be:

Do we need to prefix LocalCommitmentTransaction with "Local" if we don't have a RemoteCommitmentTransaction?

Do we need to prefix the field local_keys with "local" given the context?

My thinking is either the second or both could be eliminated without loss of meaning.

Dropped prefix local_ for keys.

jkczyz · 2020-06-09T23:51:12Z

lightning/src/ln/channel.rs

@@ -1893,13 +1893,13 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 		}

-		let their_funding_pubkey = self.their_pubkeys.as_ref().unwrap().funding_pubkey;
+		let funding_pubkey = self.counterparty_pubkeys.as_ref().unwrap().funding_pubkey;


I've seen similar occurrences like this. Why would there not be a prefix on this variable name when the context (i.e., self since this is inside a method) is not the counterparty? I'd expect missing prefixes to be where we'd normally use our_.

You're right, I've been confused by this first one. In fact the pubkey has a clear owner but redeemScript and txo is common. Corrected.

jkczyz

Thanks for making the change! Eventually, we may want to encapsulate these into separate structs with related elements (e.g., limits, balance, scripts, etc?) and expose behaviors rather than fields.

lightning/src/ln/channel.rs

jkczyz · 2020-06-11T00:46:38Z

lightning/src/ln/channel.rs

-		let local_initial_commitment_tx = self.build_commitment_transaction(self.cur_local_commitment_transaction_number, &local_keys, true, false, self.feerate_per_kw, logger).0;
-		let local_sighash = hash_to_message!(&bip143::SighashComponents::new(&local_initial_commitment_tx).sighash_all(&local_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]);
+		let keys = self.build_local_transaction_keys(self.cur_commtment_transaction_number)?;
+		let initial_commitment_tx = self.build_commitment_transaction(self.cur_commtment_transaction_number, &keys, true, false, self.feerate_per_kw, logger).0;


Should this still be "local" as per #633 (comment)? Not sure if I misinterpreted when "local" and "remote" should be used.

Other places throughout the file were similarly changed.

I interpret code here as "We build a commitment transaction. Owner is us" So no prefix.

Hmm, so I thought this would have fallen under the second bullet in the linked comment. Would we ever say local_commitment_tx and remote_commitment_tx in your interpretation? If so, in what context?

I think we should restrain pointed distinction (local/remote) to templates as used by our chan helpers (chan_utils.rs) and avoid leaking local/remote in Channel storage fields. Even it's not perfect for now.

jkczyz · 2020-06-11T00:46:49Z

lightning/src/ln/channel.rs

-		let remote_initial_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw, logger).0;
-		let remote_signature = self.local_keys.sign_remote_commitment(self.feerate_per_kw, &remote_initial_commitment_tx, &remote_keys, &Vec::new(), self.our_to_self_delay, &self.secp_ctx)
+		let counterparty_keys = self.build_remote_transaction_keys()?;
+		let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, self.feerate_per_kw, logger).0;


Likewise, but "remote".

Same reasoning. Builder has the notion of local/remote, its ouput has clearly an owner.

jkczyz · 2020-06-11T00:57:38Z

lightning/src/ln/channel.rs

-		let node_a_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
+		let node_a_counterparty_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let config = UserConfig::default();
-		let mut node_a_chan = Channel::<EnforcingChannelKeys>::new_outbound(&&feeest, &&keys_provider, node_a_node_id, 10000000, 100000, 42, &config).unwrap();
+		let mut node_a_chan = Channel::<EnforcingChannelKeys>::new_outbound(&&feeest, &&keys_provider, node_a_counterparty_node_id, 10000000, 100000, 42, &config).unwrap();

 		// Create Node B's channel by receiving Node A's open_channel message
 		let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.bitcoin_hash(), &&feeest);
-		let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
-		let mut node_b_chan = Channel::<EnforcingChannelKeys>::new_from_req(&&feeest, &&keys_provider, node_b_node_id, InitFeatures::known(), &open_channel_msg, 7, &config).unwrap();
+		let node_b_counterparty_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
+		let mut node_b_chan = Channel::<EnforcingChannelKeys>::new_from_req(&&feeest, &&keys_provider, node_b_counterparty_node_id, InitFeatures::known(), &open_channel_msg, 7, &config).unwrap();


Not sure I understand this change. Why "counterparty" here? Should we just call them node_a_id and node_b_id in the appropriate places?

Right I think here that's zeal. Corrected.

I believe the node ids aren't accurately named even with your change reverted. That is, node_a_chan should be created with node B's id. In practice, it doesn't matter since the ids aren't used after the Channel is created.

You right, updated comments to make it clearer.

jkczyz · 2020-06-11T01:02:28Z

lightning/src/ln/channelmanager.rs

-	fn internal_open_channel(&self, their_node_id: &PublicKey, their_features: InitFeatures, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {
+	fn internal_open_channel(&self, counterparty_node_id: &PublicKey, their_features: InitFeatures, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {


Personally, for all these methods I'd just call the parameter node_id and features. From the context, it should be clear that we aren't passing in our own node id and features.

I'm ~0 on this. It's quite obvious when you're used to, but for newcomers that's clearly helpful ?

Not sure what ~0 means. 😄 Indifferent?

My argument is these are (helpers for) implementing ChannelMessageHandler, so it should be clear. I'd be fine with leaving these unchanged and moving to node_id and features in a follow-up that included updating ChannelMessageHandler.

Yes indifferent, let's keep it for a follow-up.

ariard · 2020-06-11T08:01:43Z

Thanks @jkczyz for review, rebased and updated.

Thanks for making the change! Eventually, we may want to encapsulate these into separate structs with related elements (e.g., limits, balance, scripts, etc?) and expose behaviors rather than fields.

Yes, let's do this post-anchor output implementation ? I'm modifying few things here.
I generally agree on encapsulating related elements, also to split build_commitment_transactions right now it's melting balance computation and pure transaction generation.

TheBlueMatt · 2020-06-23T19:47:34Z

This needs rebase. Maybe instead of local_ and remote_ we could use broadcaster_ and broadcastee_ or something to describe whats really happening? local and remote are still sooo ripe for confusion.

ariard · 2020-08-18T23:10:54Z

@TheBlueMatt @jkczyz Sorry for latency, finally back working on this :)

This needs rebase. Maybe instead of local_ and remote_ we could use broadcaster_ and broadcastee_ or something to describe whats really happening? local and remote are still sooo ripe for confusion.

I agree that local_/remote_ should disappear, I'm thinking about owner_/contributor_ as when generating a transaction it must have a unique final owner while we might integrate contributions from another participant. These contributions can be either our key material or our counterparty's one according to message processing flow. IMO, better than broadcaster_/broadcastee_, we don't necessary broadcast commitment transactions.

Overall, there is a huge part of the codebase still bearing the local_/remote_ nomenclature, this PR is just a step forward and should have follow-ups.

TheBlueMatt · 2020-08-19T14:52:02Z

I'm thinking about owner_/contributor_

Hmm, I still think its possible to confuse that - the node which sends all the data on the wire here is not the owner, but it feels like they should be the owner since they decided when to create the commitment tx and when to send information about it (though not when to finalize it or broadcast it).

I agree broadcaster is confusing because we often don't broadcast, but I'm not sure how else to describe a potentially-broadcastable transaction. Maybe broadcastable_node_*, though that seems verbose.

ariard · 2020-08-19T18:09:15Z

Hmm, I still think its possible to confuse that - the node which sends all the data on the wire here is not the owner, but it feels like they should be the owner since they decided when to create the commitment tx and when to send information about it (though not when to finalize it or broadcast it).

I don't agree on when to create the commitment as Alice can send a update_add_htlc/commitment_signed to Bob, whom in reply will send back signatures for updating Alice's commitment. So you can analyze as Alice who decided the creation, assuming honest protocol behavior from Bob.

I like owner as ultimately you're deciding for the finalization and thus authorizing this transaction to spend the funding output. Also it underscores you have to reject counterparty contribution if they're not valid (like invalid signatures/pubkeys).

Sounds we won't agree here, @jkczyz do you want serve as a tie-breaker ? Does owner/contributor sound like a meaningful improvement on local/remote :) ?

jkczyz · 2020-08-19T20:41:15Z

Sounds we won't agree here, @jkczyz do you want serve as a tie-breaker ? Does owner/contributor sound like a meaningful improvement on local/remote :) ?

I think I got a little lost in the discussion. :) Could you point me to a concrete example in the PR where these would be used in replace of local/remote?

lightning/src/ln/channel.rs

jkczyz · 2020-08-19T20:29:07Z

lightning/src/ln/channel.rs

-			if self.value_to_self_msat < self.local_channel_reserve_satoshis * 1000 + local_commit_tx_fee_msat {
-				return Err(ChannelError::Close("Cannot receive value that would put us under local channel reserve value".to_owned()));
+			if self.value_to_self_msat < self.channel_reserve_satoshis * 1000 + local_commit_tx_fee_msat {
+				return Err(ChannelError::Close("Cannot receive value that would put us under our channel reserve value".to_owned()));


The side ("our") of the reserve is immaterial here since its implied by this being a receive. I'd try to avoid "us" and "our" in these messages by stating as:

Cannot receive value that would put the channel under its reserve value

That said, there are other messages that use "our" and "their" pronouns it appears.

Good suggestion but I took it a bit differently "Cannot accept HTLC that would put counterparty balance under channel reserve value" as it underscores both purpose of this check and ownership of checked value.

jkczyz · 2020-08-19T20:31:58Z

lightning/src/ln/channel.rs

 		// revoke_and_ack, not on sending commitment_signed, so we add one if have
 		// AwaitingRemoteRevoke set, which indicates we sent a commitment_signed but haven't gotten
 		// the corresponding revoke_and_ack back yet.
-		let our_next_remote_commitment_number = INITIAL_COMMITMENT_NUMBER - self.cur_remote_commitment_transaction_number + if (self.channel_state & ChannelState::AwaitingRemoteRevoke as u32) != 0 { 1 } else { 0 };
+		let next_remote_commitment_number = INITIAL_COMMITMENT_NUMBER - self.cur_counterparty_commitment_transaction_number + if (self.channel_state & ChannelState::AwaitingRemoteRevoke as u32) != 0 { 1 } else { 0 };


I guess this where I'm a bit confused. Here we are mixing "counterparty" and "remote".

My bad, I think this variable was really badly-named. Switched it to next_counterparty_commitment_number as we used this value to build counterparty's commitment transaction.

jkczyz · 2020-08-19T20:33:57Z

lightning/src/ln/channel.rs

-			return Err(ChannelError::Ignore(format!("Cannot send value that would put us under local channel reserve value ({})", chan_reserve_msat)));
+		let chan_reserve_msat = self.channel_reserve_satoshis * 1000;
+		if pending_value_to_self_msat - amount_msat - commit_tx_fee_msat < chan_reserve_msat {
+			return Err(ChannelError::Ignore(format!("Cannot send value that would put us under our channel reserve value ({})", chan_reserve_msat)));


ariard · 2020-08-20T23:09:27Z

Thanks @jkczyz for the new parse. As an example of new owner/contributor usage, see TxCreationKeys in 2fb8a21

@TheBlueMatt what about remittee/recipient instead of owner ? Transactions are like letters, issued and signed by a sender but aiming to an exclusive remittee to whom only they will provide value.

New proposal remittee/countersignatory ?

TheBlueMatt · 2020-08-20T23:55:48Z

remittee/recipient or remittee/countersignatory sounds maybe ok for payments, as they are remitted, but I don't think you'd generally use the term "remit" for pure data - personally I'm still a fan of broadcastor-based language as the whole point of a transaction is to possibly be broadcast, even if it never is.

jkczyz

Thanks @jkczyz for the new parse. As an example of new owner/contributor usage, see TxCreationKeys in 2fb8a21

Is there an argument against why we shouldn't simply use no prefix and counterparty_ in TxCreationKeys as well?

When I read the documentation:

A contributor key is coming from a protocol participant contributing to the computed transaction.

I think that is just a verbose but complicated way of saying the counterparty. So rather than introduce new terminology (i.e. owner/contributor), why not be consistent by using no prefix and counterparty_ as used elsewhere? The nice thing about "counterparty" IMHO is that it is a relative term. That is, it can be used equally well within a channel or within a commitment transaction (regardless of whose it is).

jkczyz · 2020-08-21T23:22:33Z

lightning/src/ln/chan_utils.rs

+	let mut counterparty_contrib = revocation_base_secret.clone();
+	counterparty_contrib.mul_assign(&rev_append_commit_hash_key)?;
+	let mut our_contrib = per_commitment_secret.clone();


I understand that "contrib" is short for "contribution", but using this with "counterparty" may be confusing when "contributor" is later used as the counterparty of "owner" in TxCreationKeys. We can remove this possible confusion by not using "contributor" in TxCreationKeys, FWIW.

jkczyz · 2020-08-21T23:26:07Z

lightning/src/ln/chan_utils.rs

 }

 /// The set of public keys which are used in the creation of one commitment transaction.
 /// These are derived from the channel base keys and per-commitment data.
 ///
+/// A owner key is coming from intented owner of the computed transaction. A contributor


s/A owner/An owner
s/intented/intended

ariard · 2020-08-22T14:10:43Z

I'm settling on broadcaster/countersignatory, broadcastee is too much near-collision.

@jkczyz

why not be consistent by using no prefix and counterparty_ as used elsewhere? The nice thing about "counterparty" IMHO is that it is a relative term. That is, it can be used equally well within a channel or within a commitment transaction (regardless of whose it is).

I think using counterparty_ in this transaction/keys builder helpers would get us back where we're before.
Namely confusing remote as our protocol counterparty with whom we're operating this channel and remote as the protocol participant authorizing the transaction but devoid of capability to broadcast the transaction. And in those modified builders, we can be remote, like when we're initiating a channel update by sending a commitment_signed.

TheBlueMatt · 2020-08-23T00:26:24Z

lightning/src/ln/chan_utils.rs

-	part_b.mul_assign(&commit_append_rev_hash_key)?;
-	part_a.add_assign(&part_b[..])?;
-	Ok(part_a)
+	let mut counterparty_contrib = revocation_base_secret.clone();


Shouldnt this be broadcaster/countersignatory?

That's intentional, as we never derive counterparty revocation key as it's only used to punish them, revocation appears to me directed thus the counterparty_ nomenclature. I concede that can be confusing ?

Its also a pub function, so users can do whatever :p. But, sure, that's fine with me, maybe leave a comment, though.

TheBlueMatt · 2020-08-23T00:26:51Z

I posted this above, but re: no-prefix versions: I'm really not a fan of no prefix - it seems ripe for confusion. "Yep, code reads right, it says to_self_delay" when its really the wrong "to_self_delay". Seems that would be easy to miss in a code review.

ariard · 2020-08-23T23:37:48Z

Updated with restoring the local_ prefix for our to_self_delay in keysinterface, if I understood well your concern. Even if we have context there to dissociate, I concede that can be confusing.

jkczyz

I'm settling on broadcaster/countersignatory, broadcastee is too much near-collision.

SGTM. Please update the commit message for a513747 as it still mentions "owner" and "contributor".

I think using counterparty_ in this transaction/keys builder helpers would get us back where we're before.
Namely confusing remote as our protocol counterparty with whom we're operating this channel and remote as the protocol participant authorizing the transaction but devoid of capability to broadcast the transaction. And in those modified builders, we can be remote, like when we're initiating a channel update by sending a commitment_signed.

Not sure I 100% agree as there shouldn't be a concept of "our" in TxCreationKeys, but I like your choice of "broadcaster" and "countersignatory" much better. No need to argue the point any further. :)

jkczyz · 2020-08-24T16:20:20Z

lightning/src/chain/keysinterface.rs

@@ -318,13 +318,13 @@ pub trait ChannelKeys : Send+Clone {
 	/// protocol.
 	fn sign_channel_announcement<T: secp256k1::Signing>(&self, msg: &msgs::UnsignedChannelAnnouncement, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

-	/// Set the remote channel basepoints and remote/local to_self_delay.
+	/// Set the remote channel basepoints and counterparty/our local_to_self_delay.


Remove "our"?

Variables should be named according to the script semantic which is an invariant with regards to generating a local or remote commitment transaction. I.e a broadcaster_htlc_key will always guard a HTLC to the party able to broadcast the computed transactions whereas countersignatory_htlc_key will guard HTLC to a countersignatory of the commitment transaction.

TheBlueMatt · 2020-09-11T18:09:45Z

lightning/src/chain/keysinterface.rs

-		/// The remote_revocation_pubkey used to derive witnessScript
-		remote_revocation_pubkey: PublicKey
+		/// The counterparty_revocation_pubkey used to derive witnessScript
+		counterparty_revocation_pubkey: PublicKey


s/counterparty//

TheBlueMatt · 2020-09-11T18:10:07Z

lightning/src/chain/keysinterface.rs

 	/// ChannelKeys::pubkeys().delayed_payment_basepoint) and the provided per_commitment_point to
 	/// chan_utils::derive_private_key. The public key can be generated without the secret key
 	/// using chan_utils::derive_public_key and only the delayed_payment_basepoint which appears in
 	/// ChannelKeys::pubkeys().
 	///
-	/// To derive the remote_revocation_pubkey provided here (which is used in the witness
-	/// script generation), you must pass the remote revocation_basepoint (which appears in the
+	/// To derive the counterparty_revocation_pubkey provided here (which is used in the witness


s/counterparty// (but not on the next line down).

TheBlueMatt · 2020-09-11T18:22:14Z

lightning/src/ln/chan_utils.rs

@@ -335,7 +350,7 @@ pub struct ChannelPublicKeys {
 	/// states.
 	pub revocation_basepoint: PublicKey,
 	/// The public key which receives our immediately spendable primary channel balance in
-	/// remote-broadcasted commitment transactions. This key is static across every commitment


Not your bug, but can you fix this comment to not refer to ours/theirs/counterparty/etc - this struct is generic across both sides (and thus should only ever use broadcaster/countersignatory).

Hmmm I keep the counterparty's it makese sense here, as this immediate spend is only available if it's the counterparty commitment ?

TheBlueMatt · 2020-09-11T18:31:29Z

lightning/src/ln/chan_utils.rs

 	let payment_hash160 = Ripemd160::hash(&htlc.payment_hash.0[..]).into_inner();
 	if htlc.offered {
 		Builder::new().push_opcode(opcodes::all::OP_DUP)
 		              .push_opcode(opcodes::all::OP_HASH160)
-		              .push_slice(&PubkeyHash::hash(&revocation_key.serialize())[..])
+		              .push_slice(&PubkeyHash::hash(&broadcaster_revocation_key.serialize())[..])


There are still a number of references to broadcaster_revocation_key - can you remove those too?

TheBlueMatt

Still reviewing, but one comment came up.

TheBlueMatt · 2020-09-14T16:30:39Z

lightning/src/ln/channel.rs

-			if self.value_to_self_msat < self.local_channel_reserve_satoshis * 1000 + local_commit_tx_fee_msat {
-				return Err(ChannelError::Close("Cannot receive value that would put us under local channel reserve value".to_owned()));
+			if self.value_to_self_msat < self.counterparty_selected_channel_reserve_satoshis * 1000 + local_commit_tx_fee_msat {
+				return Err(ChannelError::Close("Cannot accept HTLC that would put counterparty balance under our-announced channel reserve value".to_owned()));


This is about counterparty-announced and our balance, not the other way around.

Previously most of variable fields relative to data belonging to our node or counterparty were labeled "local"/"remote". It has been deemed confusing with regards to transaction construction which is always done from a "local" viewpoint, even if owner is our counterparty

TheBlueMatt

One more note plus this needs squashed or at least 4491986 fixed (it doesn't build on its own).

TheBlueMatt · 2020-09-14T18:28:13Z

lightning/src/ln/chan_utils.rs

-	/// The public key which receives our immediately spendable primary channel balance in
-	/// remote-broadcasted commitment transactions. This key is static across every commitment
+	/// The public key which receives an immediately spendable primary channel balance in
+	/// a broadcaster's commitment transactions. This key is static across every commitment


This reads a bit confusing - its the countersignatory's balance in a broadcaster's transaction, but reading it I'd think its a broadcaster's balance.

TheBlueMatt · 2020-09-14T18:34:26Z

Verified that every commit except 4491986 builds + passes tests, so just that one commit needs fixedup/the next squashed into it.

To avoid reviewers confusion, rename counterparty_to_self_delay to counteparty_selected_contest_delay, i.e the justice delay announced by a channel counterparty restraining our transactions, and to_self_delay to locally_selected_contest_delay, i.e the justice delay announced by us restraining counterparty's transactions We deviate from wider nomenclature by prefixing local data with a locally_ extension due to the leak of this value in transactions/scripts builder, where the confusion may happen. Rename further AcceptChannelData to the new nomenclature.

A TxCreationKeys set represents the key which will be embedded in output scripts of a party's commitment tx state. Among them there is a always a key belonging to counter-party, the HTLC pubkey. To dissociate strongly, prefix keys with broadcaster/countersignatory. A revocation keypair is attributed to the broadcaster as it's used to punish a fraudulent broadcast while minding that such keypair derivation method will be always used by countersignatory as it's its task to enforce punishement thanks to the release secret.

Transaction signing methods are changed from local_/remote_ prefix to newer holder_/counterparty_ wihout any semantic changes.

Comment meaning of holder/counterparty Diverse chan_utils cleanups Cleanups post-cbindings merge Fix misusage of holder_selected_contest_delay instead of counterparty _selected_contest_delay in HolderCommitmentTransaction Fix old payment_point comment

…t#683.

…, lightningdevkit#683.

Small tweaks post #633/#684

TheBlueMatt reviewed May 29, 2020

View reviewed changes

ariard force-pushed the 2020-05-rename-keys-var branch from c09c9fb to e0b9f5b Compare June 9, 2020 00:54

jkczyz reviewed Jun 10, 2020

View reviewed changes

ariard force-pushed the 2020-05-rename-keys-var branch 2 times, most recently from a399d16 to b801a63 Compare June 10, 2020 23:59

jkczyz reviewed Jun 11, 2020

View reviewed changes

ariard force-pushed the 2020-05-rename-keys-var branch 2 times, most recently from 88a2b38 to fa4ee8d Compare June 11, 2020 07:59

ariard force-pushed the 2020-05-rename-keys-var branch from fa4ee8d to 724ad6e Compare August 18, 2020 23:01

jkczyz reviewed Aug 19, 2020

View reviewed changes

ariard force-pushed the 2020-05-rename-keys-var branch from 724ad6e to f763041 Compare August 20, 2020 23:04

jkczyz reviewed Aug 22, 2020

View reviewed changes

ariard force-pushed the 2020-05-rename-keys-var branch from f763041 to 975a68e Compare August 22, 2020 14:06

TheBlueMatt reviewed Aug 23, 2020

View reviewed changes

ariard force-pushed the 2020-05-rename-keys-var branch from 975a68e to 1b590ea Compare August 23, 2020 23:36

jkczyz reviewed Aug 24, 2020

View reviewed changes

ariard force-pushed the 2020-05-rename-keys-var branch from 1b590ea to 7767542 Compare August 26, 2020 14:47

ariard force-pushed the 2020-05-rename-keys-var branch from af4c0e4 to e10aa15 Compare September 11, 2020 13:35

ariard force-pushed the 2020-05-rename-keys-var branch 2 times, most recently from 652553c to 205a84c Compare September 11, 2020 15:27

TheBlueMatt reviewed Sep 11, 2020

View reviewed changes

ariard force-pushed the 2020-05-rename-keys-var branch 2 times, most recently from 9890e6f to 874a1b0 Compare September 14, 2020 16:28

TheBlueMatt reviewed Sep 14, 2020

View reviewed changes

ariard force-pushed the 2020-05-rename-keys-var branch from 874a1b0 to 8c284b8 Compare September 14, 2020 17:19

TheBlueMatt approved these changes Sep 14, 2020

View reviewed changes

TheBlueMatt reviewed Sep 14, 2020

View reviewed changes

Antoine Riard added 6 commits September 14, 2020 14:39

Change ChannelKeys interface nomenclature to holder/counterparty one

9a23130

Transaction signing methods are changed from local_/remote_ prefix to newer holder_/counterparty_ wihout any semantic changes.

Overhaul ChannelMonitor/OnchainTxHandler to new nomenclature

00d063d

Overhaul LocalCommitmentTx to new nomenclature

6133498

Cleanup locally-selected-delay

21d0a95

Comment meaning of holder/counterparty Diverse chan_utils cleanups Cleanups post-cbindings merge Fix misusage of holder_selected_contest_delay instead of counterparty _selected_contest_delay in HolderCommitmentTransaction Fix old payment_point comment

ariard force-pushed the 2020-05-rename-keys-var branch from 8c284b8 to 21d0a95 Compare September 14, 2020 19:22

TheBlueMatt merged commit d5912ec into lightningdevkit:master Sep 14, 2020

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 14, 2020

Update bindings after merge of lightningdevkit#633.

3ab460c

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 14, 2020

Update bindings after merge of lightningdevkit#633 and lightningdevki…

ef34c6d

…t#683.

TheBlueMatt mentioned this pull request Sep 14, 2020

Small tweaks post #633/#684 #690

Merged

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 16, 2020

Update bindings after merge of lightningdevkit#633 and lightningdevki…

e725028

…t#683.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 16, 2020

Update bindings after merge of lightningdevkit#633, lightningdevkit#679…

9b7f98c

…, lightningdevkit#683.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 16, 2020

Update bindings after merge of lightningdevkit#633, lightningdevkit#679…

7ab4273

…, lightningdevkit#683.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 16, 2020

Update bindings after merge of lightningdevkit#633, lightningdevkit#679…

49e0a8c

…, lightningdevkit#683.

TheBlueMatt added a commit that referenced this pull request Sep 16, 2020

Merge pull request #690 from TheBlueMatt/2020-09-633-bindings

d735085

Small tweaks post #633/#684

TheBlueMatt mentioned this pull request Sep 16, 2020

[Peers] Tech Debt: Rename node_id/their_node_id #705

Open


		let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.a_htlc_key, &self.local_keys.b_htlc_key, &self.local_keys.revocation_key);
		let htlc_redeemscript = get_htlc_redeemscript_with_explicit_keys(&this_htlc.0, &self.local_keys.local_htlc_key, &self.local_keys.remote_htlc_key, &self.local_keys.revocation_key);

		fn internal_open_channel(&self, their_node_id: &PublicKey, their_features: InitFeatures, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {
		fn internal_open_channel(&self, counterparty_node_id: &PublicKey, their_features: InitFeatures, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {

Rename a's keys as local's keys and b's keys as remote's keys #633

Rename a's keys as local's keys and b's keys as remote's keys #633

Conversation

ariard commented May 29, 2020

Choose a reason for hiding this comment

ariard May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Jun 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 29, 2020 • edited Loading

Codecov Report

ariard commented Jun 9, 2020

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Jun 11, 2020

TheBlueMatt commented Jun 23, 2020

ariard commented Aug 18, 2020

TheBlueMatt commented Aug 19, 2020

ariard commented Aug 19, 2020

jkczyz commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Aug 20, 2020 • edited Loading

TheBlueMatt commented Aug 20, 2020

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Aug 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 23, 2020 • edited Loading

ariard commented Aug 23, 2020

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

ariard May 29, 2020 •

edited

Loading

TheBlueMatt Jun 6, 2020 •

edited

Loading

codecov bot commented May 29, 2020 •

edited

Loading

ariard Jun 10, 2020 •

edited

Loading

ariard commented Aug 20, 2020 •

edited

Loading

TheBlueMatt commented Aug 23, 2020 •

edited

Loading