Improve documentation for InboundHTLCState enum states.

[valentinewallace]

@jkczyz related to a previous discussion on Slack -- would appreciate your input on whether this is clarifying (though another reviewer will probably have to confirm that everything's correct here, first).

[codecov]

Codecov Report

Merging #634 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #634   +/-   ##
=======================================
  Coverage   91.26%   91.26%           
=======================================
  Files          35       35           
  Lines       20918    20918           
=======================================
  Hits        19090    19090           
  Misses       1828     1828           

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	86.62% <ø> (ø)

---

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 f08d610...96daffa. Read the comment docs.

[jkczyz]

Yes, definitely much clearer! Especially with the example.

I think a further improvement could be to rename the enum variants. When I was struggling to understand these states, the following questions came to mind:

• What is being announced? Is it different for each place "announce" is used? Seems like a source of ambiguity.
• Is "remote" redundant in the "awaiting" states? IMHO, it makes the identifiers a bit of a mouthful.

By answering the first point, I think we can come up with some more intuitive names.

[valentinewallace]

Yes, I'd agree with those assessments --

Do you think this would be clearer, for the first 3?

Offered(PendingHTLCStatus)
AwaitingPrevStateRAAToAccept(PendingHTLCStatus)
AwaitingFinalRAA(PendingHTLCStatus)

I prefer the terms "offered" and "accepted", since they align with the spec better.

If the first Q was not rhetorical, it seems to me like there are 3 different definitions of "announced" here. In RemoteAnnounced, Announced == offered. In AwaitingRemoteRevokeToAnnounce, Announce = accept. In AwaitingAnnouncedRemoteRevoke, Announced = "sent"(?). Though @TheBlueMatt would have to confirm these guesses...

[jkczyz]

I like Offered. And I'd like to make sure "accepted" has a clear definition.

For the Awaiting states, given there are comments that each implies AwaitingRemoteRevoke, one possibility is to not encode that information in the name at all. Rather, name states in a logically consistent manner with respect to the HTLC itself and define what those names mean in terms of messages that have been passed within the documentation.

One idea:

enum InboundHTLCState {
	Offered(PendingHTLCStatus),
	Pending(PendingHTLCStatus),
	Accepted(PendingHTLCStatus),
	Committed,
	Removed(InboundHTLCRemovalReason),
}

I'm not sure if there are better names for Pending and Accepted, so let me know if something better comes to mind. I would want to make sure we aren't overloading the term "pending" elsewhere for instance. And even better if we are consistent with terminology used elsewhere. So maybe "Held" or something similar would be more appropriate.

Thoughts?

[valentinewallace]

Agree pending is a bit overloaded, but I like the direction of those names. I slightly preferred LocalRemoved since the remote has not removed the HTLC, so it's not fully removed per se.

My definition of "accepted" means the HTLC is definitely in our commitment tx and we've sent commitment sigs for it. I can explicitly define it the comments. But, the spec doesn't explicitly define it.

edit: removed my suggestion for the Pending replacement, wasn't a fan actually

[jkczyz]

FYI, my argument for using Held instead of Pending would be if it corresponded to HTLCs in the holding cell, which I'm not certain about.

Don't have a strong opinion yet on LocalRemoved. One argument for leaving the prefix out -- or keeping it in for that matter -- is that each state could possibly be similarly prefixed by either "Local" or "Remote" if there is some ambiguity. Though I hope that with whatever naming we decide on, the "inbound" context makes it clear which. The same goes for renaming the states within OutboundHTLCState. So trying a similar renaming there may help determine what sort of prefixing, if any, works best.

[TheBlueMatt]

Nice! Thanks.

[TheBlueMatt]

I feel a bit awkward calling this the reason - is true, it is a reason, but the other reason is because RAA "commits" a state, but doesn't specify which state, leaving an implication of it ACK'ing the latest state (commitent_signed that we sent).

[valentinewallace]

Let me know if the new version is good. I added the second reason as the last sentence of the paragraph

[valentinewallace]

I like Offered. And I'd like to make sure "accepted" has a clear definition.

For the Awaiting states, given there are comments that each implies AwaitingRemoteRevoke, one possibility is to not encode that information in the name at all. Rather, name states in a logically consistent manner with respect to the HTLC itself and define what those names mean in terms of messages that have been passed within the documentation.

One idea:

enum InboundHTLCState {
	Offered(PendingHTLCStatus),
	Pending(PendingHTLCStatus),
	Accepted(PendingHTLCStatus),
	Committed,
	Removed(InboundHTLCRemovalReason),
}

I'm not sure if there are better names for Pending and Accepted, so let me know if something better comes to mind. I would want to make sure we aren't overloading the term "pending" elsewhere for instance. And even better if we are consistent with terminology used elsewhere. So maybe "Held" or something similar would be more appropriate.

Thoughts?

@jkczyz Upon further reflection, I'm in favor of all these names, except Pending, which I'm in favor of switching to Blocked. Sorta captures that the HTLC's progress is blocked on an RAA. And I'd probably like to do a separate PR, since this one seems pretty close to mergable + self-contained. Would agree that it'd be best to change the names of the Inbound and Outbound HTLC states in the same PR, though, to make sure they align well.

[valentinewallace]

FYI, my argument for using Held instead of Pending would be if it corresponded to HTLCs in the holding cell, which I'm not certain about.

Don't have a strong opinion yet on LocalRemoved. One argument for leaving the prefix out -- or keeping it in for that matter -- is that each state could possibly be similarly prefixed by either "Local" or "Remote" if there is some ambiguity. Though I hope that with whatever naming we decide on, the "inbound" context makes it clear which. The same goes for renaming the states within OutboundHTLCState. So trying a similar renaming there may help determine what sort of prefixing, if any, works best.

IMO "Held" could be a tad misleading since these HTLCs are a subset of all HTLCs in the holding cell, afaict from the holding cell code?

[jkczyz]

@jkczyz Upon further reflection, I'm in favor of all these names, except Pending, which I'm in favor of switching to Blocked. Sorta captures that the HTLC's progress is blocked on an RAA. And I'd probably like to do a separate PR, since this one seems pretty close to mergable + self-contained. Would agree that it'd be best to change the names of the Inbound and Outbound HTLC states in the same PR, though, to make sure they align well.

SGTM. Yes, Blocked is much better!

IMO "Held" could be a tad misleading since these HTLCs are a subset of all HTLCs in the holding cell, afaict from the holding cell code?

Good point -- I think you're right.