Make channel reserve variable names less confusing.

[valentinewallace]

Previous to this commit, variables such as their_channel_reserve
referred to the channel reserve that we are required to keep,
(the value is initially set by the remote). Similarly,
variables such as our_channel_reserve referred to the channel
reserve that we require the remote to keep.

Change this to use local_channel_reserve / remote_channel_reserve
to refer to the the channel reserve that the local is required to keep
and the channel reserve that the remote is required to keep, respectively.

I liked @jkczyz's suggestion in this comment so went with those names, but open to other options.

Closes #181.

[codecov]

Codecov Report

Merging #613 into master will increase coverage by 0.00%.
The diff coverage is 96.96%.

@@           Coverage Diff           @@
##           master     #613   +/-   ##
=======================================
  Coverage   91.12%   91.12%           
=======================================
  Files          34       34           
  Lines       20544    20545    +1     
=======================================
+ Hits        18720    18721    +1     
  Misses       1824     1824           

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	86.42% <95.83%> (+<0.01%)	⬆️
lightning/src/ln/functional_tests.rs	97.04% <100.00%> (ø)

---

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 9098240...1b656f4. Read the comment docs.

[TheBlueMatt]

Maybe to_self_channel_reserve_satoshis? "local" is also somewhat overloaded in a few ways whereas to_self is pretty clear in that its the value to us.

[jkczyz]

@TheBlueMatt I see "local" used quite a bit throughout channel.rs, but I don't have a firm grasp as to how the term is overloaded. Could you provide some examples where it is overloaded and how they differ from each other?

[TheBlueMatt]

local can refer to a payment to us (the "local node"), or a local transaction (ie one which we can broadcast, for which the signature was provided by "the remote node"). I think those are the two main ones, but certainly conflict in a few places AFAIR.

[jkczyz]

I wouldn't consider these instances of "local" as overloaded (i.e., used with two different meanings). Rather, they seem to be consistently used to qualify an entity or concept that can exist on either side of a channel.

If a similar relationship exists for channel reserve (which it does seem), then using similar naming makes for greater consistency and is something we should strive for.

I'd be more interested in knowing if there is something that is currently qualified by "local" which doesn't have a "remote" counterpart (not necessarily in code but at least conceptually), or vice versa. Then there may be an argument for renaming that instead.

Note: I'm not necessarily arguing for using "local" and "remote" everywhere. There are place where there may be more appropriate terms (e.g., "sender" and "receiver", "funder" and "fundee").

[valentinewallace]

While local may have those other meanings, local_channel_reserve as a whole is pretty plainly descriptive and has an obvious meaning.

My 2 sats. to_self is a compromise I could live with.

[TheBlueMatt]

I guess the confusion I'd have in this context is do we mean "local" as in "amount to the local node kept as a buffer" or "local" as in "amount the local node decided should be kept as a buffer" - in general I find "our" "local" "their" and "remote" to all mean the same thing and trying to make some kind of differentiation about what means what that is only relevant to channel.rs makes my head spin.

[jkczyz]

I guess the confusion I'd have in this context is do we mean "local" as in "amount to the local node kept as a buffer" or "local" as in "amount the local node decided should be kept as a buffer"

I think the question should be irrelevant for the reasons I gave in #181 (comment). Or put another way, there is no "local node" in this abstraction. The abstraction is simply a channel that has two ends.

in general I find "our" "local" "their" and "remote" to all mean the same thing and trying to make some kind of differentiation about what means what that is only relevant to channel.rs makes my head spin.

We should definitely use the terms consistently both within and across modules. And if we are not, we should correct that or come up with more suitable concepts. If it makes your head spin, it's bound to make the user's head explode! 🤯 Obscurity causes complexity, and complexity makes code hard to understand as has been demonstrated.

(FWIW, I think a compelling case can be made that possessive pronouns like "our" and "their" lead to ambiguity and shouldn't be used in naming. I have similar feelings about "self".)

One alternative is to remove the prefixes entirely by using a struct for each end of the channel, grouping the relevant fields without needing a prefix. Or possibly making smaller abstractions that encapsulated some functionality. Or a combination of both. channel.rs has nearly 4300 lines of non-test code and Channel has 56 fields (!) if I accurately counted them. It seems ripe for refactoring. That said, let's limit the scope of this PR to a simple rename if we can. 😃

[valentinewallace]

I guess the confusion I'd have in this context is do we mean "local" as in "amount to the local node kept as a buffer" or "local" as in "amount the local node decided should be kept as a buffer"

I guess I'd point out that me, Jeff, Antoine and yuntai all assumed that local would mean the former and not the latter, so I'd argue that evidence suggests your confusion may be pretty rare and not apply to the majority of people (no offense intended).

[ariard]

I'd be more interested in knowing if there is something that is currently qualified by "local" which doesn't have a "remote" counterpart (not necessarily in code but at least conceptually), or vice versa. Then there may be an argument for renaming that instead.

IIRC no, can't find a counter-example on-the-fly.

The abstraction is simply a channel that has two ends.

Yes but you even process from a single-side. You received some of your local settings from remote. And you may build remote transactions from your "local" viewpoint but there shouldn't be confusion.

Obscurity causes complexity, and complexity makes code hard to understand as has been demonstrated.

I fairly agree with that. I've already introduced bug in the past due to confusion between their_to_self/our_to_self in ChannelMonitor.

Or possibly making smaller abstractions that encapsulated some functionality

Agree too, we should be avoid being Linux with a 200-fields task_struct

I would like also to raise your awareness about name keys like a_htlc_key or b_htlc_key in get_htlc_redeemscript_with_explicit_keys functions-like. If someone wants to burn them I would be happy to bring the spark :p

[TheBlueMatt]

(FWIW, I think a compelling case can be made that possessive pronouns like "our" and "their" lead to ambiguity and shouldn't be used in naming. I have similar feelings about "self".)

I think I'm increasingly agreeing with this.

I guess I'd point out that me, Jeff, Antoine and yuntai all assumed that local would mean the former and not the latter, so I'd argue that evidence suggests your confusion may be pretty rare and not apply to the majority of people (no offense intended).

That's pretty compelling. I'm just gonna merge it, if someone feels compelled to come up with even better names in the future, we can open another PR.

[TheBlueMatt]

In the future, best to keep bugfixes to separate commits from pure-renaming. Otherwise I'll think you're trying to slip something in :p.

[valentinewallace]

Ha noted, thanks