Channel internal refactoring and cleanups

[ariard]

Our current channel code is functional but is a bit bloated and it would be great to clean it when we do any accounting changes (like buffer spikes, anchor output), understanding correctness of our channel policy or state machines.

Opening this issue to track all changes we may find desirable:

• Rename channel variables (see Rename a's keys as local's keys and b's keys as remote's keys #633)
• Rename, document channel/inbound_htlc/outbound_htlc state machines
• Split channel state machine from htlc ones (AwaitingRemoteRevokeToAnnounce/AwaitingAnnouncedRemoteRevoke must die)
• Document and verify accounting logic (see as a starter ariard@9697578, also logs rust-bitcoin http://gnusha.org/rust-bitcoin/2020-06-11.log)
• Encapsulate channel variables by logic with safe methods
• Split build_commitment_transaction between accounting and transaction generation, the second is a function of the one
• ?

(Tagging this at good first issue, some items on this list like documentation are really good starters to understand LN internals well)

[TheBlueMatt]

This seems pretty broad and somewhat opaque. Moreover, things like “splitting the channel and HTLC state machines” I’m not sure is really practical, so it seems strange to put it in an issue unless you have a specific workable proposal for it. I think there’s some cleanup worth doing, mostly in terms of finding ways to make the accounting logic more robust/tested, but I’m not sure about the rest here.

…

On Jun 23, 2020, at 01:32, Antoine Riard ***@***.***> wrote:  Our current channel code is functional but is a bit bloated and it would be great to clean it when we do any accounting changes (like buffer spikes, anchor output), understanding correctness of our channel policy or state machines. Opening this issue to track all changes we may find desirable: Rename channel variables (see #633) Rename, document channel/inbound_htlc/outbound_htlc state machines Split channel state machine from htlc ones (AwaitingRemoteRevokeToAnnounce/AwaitingAnnouncedRemoteRevoke must die) Document and verify accounting logic (see as a starter ***@***.***, also logs rust-bitcoin http://gnusha.org/rust-bitcoin/2020-06-11.log) Encapsulate channel variables by logic with safe methods Split build_commitment_transaction between accounting and transaction generation, the second is a function of the one ? (Tagging this at good first issue, some items on this list like documentation are really good starters to understand LN internals well) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[ariard]

Moreover, things like “splitting the channel and HTLC state machines” I’m not sure is really practical, so it seems strange to put it in an issue unless you have a specific workable proposal for it.

Yes I do see how to split them in a practical and safe way, was starting to do it while working on it but feel it was a bit of a digression. I agree it's not a priority now but on the long term their obscurity may reveal harmful.

About the rest, I think they are shared by other contributors, see Jeff comment here :

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

[TheBlueMatt]

Right, pulling accounting logic out into separate bits to add such abstractions within channel makes sense, especially as an effort to validate it more thoroughly, I don't think I was debating that?

[TheBlueMatt]

Removing good first issue, dunno exactly what this would entail but certainly it'd be a lot for a new contributor.