Skip to content

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

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.

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

Merged
merged 8 commits into from
Sep 14, 2020

Conversation

ariard
Copy link

@ariard ariard commented May 29, 2020

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.

@@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

@ariard ariard May 29, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Jun 6, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link

codecov bot commented May 29, 2020

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Author

ariard commented Jun 9, 2020

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.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

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.

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)?,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@ariard ariard Jun 10, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.


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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Dropped prefix local_ for keys.

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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_.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

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.

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, but "remote".

Copy link
Author

Choose a reason for hiding this comment

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

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

Comment on lines 4372 to 4379
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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

Right I think here that's zeal. Corrected.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

You right, updated comments to make it clearer.

Comment on lines -2214 to +2269
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> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

@ariard
Copy link
Author

ariard commented Jun 11, 2020

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
Copy link
Collaborator

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
Copy link
Author

ariard commented Aug 18, 2020

@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
Copy link
Collaborator

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
Copy link
Author

ariard commented Aug 19, 2020

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
Copy link
Contributor

jkczyz commented Aug 19, 2020

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?

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()));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

// 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 };
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

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)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise.

@ariard
Copy link
Author

ariard commented Aug 20, 2020

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
Copy link
Collaborator

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.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

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).

Comment on lines 240 to 243
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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

/// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@ariard
Copy link
Author

ariard commented Aug 22, 2020

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.

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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldnt this be broadcaster/countersignatory?

Copy link
Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Aug 23, 2020

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
Copy link
Author

ariard commented Aug 23, 2020

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.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

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. :)

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove "our"?

@ariard
Copy link
Author

ariard commented Aug 26, 2020

Updated with last comments.

@@ -319,13 +319,13 @@ pub trait ChannelKeys : Send+Clone {
/// protocol.
fn sign_channel_announcement<T: secp256k1::Signing>(&self, msg: &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/local_to_self_delay.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do we think about counterparty_set_to_local_delay and local_set_to_counterparty_delay? I still have a hard time thinking that this is that much clearer.

/// Countersignatory's HTLC Key
pub countersignatory_htlc_key: PublicKey,
/// Broadcaster's Payment Key (which isn't allowed to be spent from for some delay)
pub delayed_payment_key: PublicKey,
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 leave the broadcaster_ prefix here?

Copy link
Author

Choose a reason for hiding this comment

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

There is only one delayed_payment_key per-commitment transaction. Previously revocation_key didn't have a prefix. This is converging towards this nomenclature antecedent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, its the only one, but there's no harm in indicating who's it is, and it can only help readability.

Err(_) => return Err(())
};
let local_htlcpubkey = match chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint) {
Ok(local_htlcpubkey) => local_htlcpubkey,
let htlcpubkey = match chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Can we leave the local_ prefix here?

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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

let mut part_b = per_commitment_point.clone();
part_b.mul_assign(&secp_ctx, &commit_append_rev_hash_key)?;
part_a.combine(&part_b)
let mut counterparty_contrib = revocation_base_point.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, on the other hand, my point at https://github.com/rust-bitcoin/rust-lightning/pull/633/files#r475152575 stands (and maybe its nice that they use the same nomenclature)

}

/// 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 broadcaster key is coming from potential broadcaster of the computed transaction.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/is coming from/is provided by/

shutdown_pubkey: PublicKey,
destination_script: Script,

// Our commitment numbers start at 2^48-1 and count down, whereas the ones used in transaction
// generation start at 0 and count up...this simplifies some parts of implementation at the
// cost of others, but should really just be changed.

cur_local_commitment_transaction_number: u64,
cur_remote_commitment_transaction_number: u64,
cur_commitment_transaction_number: u64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we keep local here?

@@ -326,19 +326,19 @@ pub(super) struct Channel<ChanSigner: ChannelKeys> {
// is received. holding_cell_update_fee is updated when there are additional
// update_fee() during ChannelState::AwaitingRemoteRevoke.
holding_cell_update_fee: Option<u32>,
next_local_htlc_id: u64,
next_remote_htlc_id: u64,
next_htlc_id: u64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here.

@@ -442,7 +442,7 @@ macro_rules! secp_check {

impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
// Convert constants + channel value to limits:
fn get_our_max_htlc_value_in_flight_msat(channel_value_satoshis: u64) -> u64 {
fn get_max_htlc_value_in_flight_msat(channel_value_satoshis: u64) -> u64 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here and for derive_dust_limit.

let max_to_self_delay = u16::min(config.peer_channel_config_limits.their_to_self_delay, MAX_LOCAL_BREAKDOWN_TIMEOUT);
if msg.to_self_delay > max_to_self_delay {
return Err(ChannelError::Close(format!("They wanted our payments to be delayed by a needlessly long period. Upper limit: {}. Actual: {}", max_to_self_delay, msg.to_self_delay)));
let max_locally_selected_contest_delay = u16::min(config.peer_channel_config_limits.their_to_self_delay, MAX_LOCAL_BREAKDOWN_TIMEOUT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldnt this be counteraprty_selected?

Copy link
Author

Choose a reason for hiding this comment

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

I think it's a bit more subtle, it's the max counterparty_selected as enforced by our configuration, but yeah updated.

let remote_channel_reserve_satoshis = Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(msg.funding_satoshis);
if remote_channel_reserve_satoshis < our_dust_limit_satoshis {
return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). our_dust_limit_satoshis is ({}).", remote_channel_reserve_satoshis, our_dust_limit_satoshis)));
if remote_channel_reserve_satoshis < dust_limit_satoshis {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same, maybe don't drop our.

@@ -80,7 +80,7 @@ pub enum SpendableOutputDescriptor {
/// 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
/// call to ChannelKeys::on_accept) and the provided per_commitment point
/// to chan_utils::derive_public_revocation_key.
/// to chan_utils::derive_public_broadcaster_revocation_key.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comments/variables/etc here still have a ton of references to remote_, to_self_delay, etc.

Copy link
Author

Choose a reason for hiding this comment

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

Right corrected, note there is still remote/local references around SpendableOutputDescriptors as they're relative to ChannelMonitor (and I'll do post-anchor to avoid a confusing rebase).

@@ -319,13 +319,13 @@ pub trait ChannelKeys : Send+Clone {
/// protocol.
fn sign_channel_announcement<T: secp256k1::Signing>(&self, msg: &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_selected/locally_selected_contest_delay.
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 change the function names in ChannelKeys (above)

Copy link
Author

Choose a reason for hiding this comment

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

Add a new commit scoping ChannelKeys under new nomenclature.

@@ -80,7 +80,7 @@ pub enum SpendableOutputDescriptor {
/// To derive the remote_revocation_pubkey provided here (which is used in the witness
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still says remote_, as does the variable name further down.

Copy link
Author

Choose a reason for hiding this comment

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

Updated all ChannelMonitor/OnchainTxHandler-side, as it was tied down with using this version of Remote.


Ok(local_commitment_tx.get_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx))
Ok(holder_commitment_tx.get_local_sig(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe lets rename this to get_commitment_sig or get_tx_sig to mirror get_htlc_sigs instead of using the "local" name.

Copy link
Author

Choose a reason for hiding this comment

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

Done, LocalCommitmentTx -> HolderCommitmentTx

@@ -217,87 +217,100 @@ pub fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_com

/// Derives a per-commitment-transaction revocation key from its constituent parts.
///
/// Only the transaction broadcaster owns a valid witness to propagate a revoked
/// commitment transaction, thus per_commitment_secret always come from broadcaster
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: s/come/comes/, twice plus in comments below.

Copy link
Author

Choose a reason for hiding this comment

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

Not gonna to take it, to avoid modifying commits

@@ -217,87 +217,100 @@ pub fn derive_public_key<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, per_com

/// Derives a per-commitment-transaction revocation key from its constituent parts.
///
/// Only the transaction broadcaster owns a valid witness to propagate a revoked
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/revoked/revocable/ plus in the comment below.

Copy link
Author

Choose a reason for hiding this comment

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

Not gonna to take it, to avoid modifying commits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, isn't this fully opposite, though - only the countersignatory of the originally-broadcasted commitment transaction can create a valid witness to broadcast a revocation key? I suppose its now a little confused - the commitment-tx-countersignatory is now the broadcaster of the revocation-punishment transaction (hence the per_commitment_secret is sent). I don't know that this needs to be captured in the names per se, but its probably worth at least fixing the comment (a new commit is fine by me, though).

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

"holder" is growing on me. :)

@@ -277,18 +277,18 @@ pub(super) struct Channel<ChanSigner: ChannelKeys> {
latest_monitor_update_id: u64,

#[cfg(not(test))]
local_keys: ChanSigner,
holder_keys: ChanSigner,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add documentation for "holder" and "counterparty" on the relevant structs/traits?

Copy link
Author

Choose a reason for hiding this comment

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

Add a solo comment in Channel. As it now used everywhere I don't know if it useful to replicate through the codebase, but not strong opinion ?

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

/// Set the remote channel basepoints and counterparty/local_to_self_delay.
/// Set the counterparty channel basepoints and counterparty_selected/locally_selected_contest_delay.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to prefer "locally" over "holder"?

Copy link
Author

Choose a reason for hiding this comment

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

Add a cleanup commit, iteration from a previous round of review.

let local_keys = self.build_local_transaction_keys(self.cur_local_commitment_transaction_number)?;
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_holder_commitment_transaction_number)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that there are a ton of places in channel.rs that still have the /local_// conversion instead of /local_/holder_/. It seems they're all in function-scope variables so I'm ok with it if you dont want to fix it, but figured I'd note in case you or Jeff feel strongly.

Copy link
Author

Choose a reason for hiding this comment

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

Yes at first try I was aiming to savagely converse everything then figured out a lot of references are meddled with ChannelState names.

I'm aiming to fix it when we refactor/cleanup further Channel internals. And have some on anchor branch.

@@ -3219,7 +3221,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {

/// Gets the fee we'd want to charge for adding an HTLC output to this Channel
/// Allowed in any state (including after shutdown)
pub fn get_our_fee_base_msat<F: Deref>(&self, fee_estimator: &F) -> u32
pub fn get_fee_base_msat<F: Deref>(&self, fee_estimator: &F) -> u32
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Sep 8, 2020

Choose a reason for hiding this comment

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

This should probably be holder (therea re a bunch of places which did the s/our// translation instead of s/our/holder/, but I think this is the only non-funciton-scope one).

pub fn get_their_htlc_minimum_msat(&self) -> u64 {
self.our_htlc_minimum_msat
pub fn get_counterparty_htlc_minimum_msat(&self) -> u64 {
self.holder_htlc_minimum_msat
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a new bug, but this looks very broken - I'm kinda sad we don't have test coverage for it.

Copy link
Author

Choose a reason for hiding this comment

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

That's was the starting point of : #670, and we know how it ends up. Add a reference in #680

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, I dont recall you mentioning this - I was under the impression 670 was NOTABUG, this is definitely a bug.

/// The broadcaster's revocation key which is used to allow the broadcaster of the commitment
/// transaction to provide their counterparty the ability to punish them if they broadcast
/// an old state.
pub broadcaster_revocation_key: PublicKey,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd be happier if this just said "revocation_key" to match "per_commitment_point" - its a private key that requires private data from both sides, and really only the countersignatory can ever learn the private key part, but its awkward to suggest its owned by either side. Note that this change would effect a few other cases of "revocation_key" in this file.

Copy link
Author

Choose a reason for hiding this comment

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

Good call, shared keys are hard to attribute ownership. Corrected and normally other references spread over.

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.
/// The remote_revocation_pubkey used to derive witnessScript
remote_revocation_pubkey: PublicKey
/// The counterparty_revocation_pubkey used to derive witnessScript
counterparty_revocation_pubkey: PublicKey
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/counterparty//

/// 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

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

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())[..])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Still reviewing, but one comment came up.

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()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Corrected.

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
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

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

/// 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

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

Antoine Riard added 6 commits September 14, 2020 14:39
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
@TheBlueMatt 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
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 14, 2020
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 16, 2020
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 16, 2020
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 16, 2020
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 16, 2020
TheBlueMatt added a commit that referenced this pull request Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants