htlcswitch: only pipeline settle if add is locked-in #6246
Conversation
Crypt-iQ
added
safety
Crypt-iQ
force-pushed
the
settle_pipeline_check
branch
from
687cdb3
to
02e7f75
Compare
htlcswitch/link_test.go
Outdated
| alice.coreLink.cfg.OnChannelFailure = func(_ lnwire.ChannelID, | ||
| _ lnwire.ShortChannelID, linkErr LinkFailureError) { | ||
| linkErrors <- linkErr | ||
| } |
There was a problem hiding this comment.
Do we need to re-set this?
There was a problem hiding this comment.
Yes since restart will re-create a coreLink and use a new OnChannelFailure
| case <-linkErrors: | ||
| t.Fatal("should not have received a link error") | ||
| case <-time.After(5 * time.Second): | ||
| // success |
There was a problem hiding this comment.
We're always going to have to wait 5 seconds here, could we update OnChannelFailure above to fail the test if it is called?
There was a problem hiding this comment.
Sorry I don't understand the suggestion. Do you mean instead of waiting 5 seconds or in addition to waiting? We could update it to fail, but as-is when it's called it does fail
There was a problem hiding this comment.
I think she means that rather than always wait 5 seconds, is it possible to instead use tighter synchronization (so fail the test if we get some error).
There was a problem hiding this comment.
good suggestion - i was able to achieve this by checking if ForwardPackets was called instead of waiting 5 seconds. There is still a five second in one case which I couldn't think of how to get around, but it does not lengthen the test in the happy path
| case <-linkErrors: | ||
| t.Fatal("should not have received a link error") | ||
| case <-time.After(5 * time.Second): | ||
| // success |
There was a problem hiding this comment.
I think she means that rather than always wait 5 seconds, is it possible to instead use tighter synchronization (so fail the test if we get some error).
| @@ -6659,6 +6659,164 @@ func TestShutdownIfChannelClean(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| // TestPipelineSettle tests that a link should only pipeline a settle if the | |||
| // related add is fully locked-in meaning it is on both sides' commitment txns. | |||
| func TestPipelineSettle(t *testing.T) { | |||
There was a problem hiding this comment.
Excellent test case reproduction!
There was a problem hiding this comment.
Would be cool to eventually translate these into some flat file format which can be used on the spec level to test implementations. IIRC we had a very early version of something like this, but it was never fully maintained.
There was a problem hiding this comment.
Yes i think the spec could benefit from an exhaustive list of state transition diagrams (add, settle, fee, closing signed etc) that show valid and invalid sequences
|
|
||
| if !lockedin { | ||
| l.fail( | ||
| LinkFailureError{code: ErrInvalidUpdate}, |
There was a problem hiding this comment.
Bolt 2 language regarding update_fulfill_htlc: https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#requirements-10
- bullet point 3 says that the sending node MUST NOT send update_fulfill_htlc until the add is locked-in. The language of MUST NOT is confusing, does this mean the recipient has to force close? IMO following the spec here to the letter may be a little superfluous as the sender shouldn't be sending this in the first place, and the receiver did nothing wrong. So I believe it's fine to not force close and instead shut down the link.
- Under the receiving node requirements "If the id does not correspond to an HTLC in its current commitment transaction:...MUST send a warning and close the connection, or send an error and fail the channel."
- Again, the sender should not be doing this. We don't close the channel or send a warning, but we do shut down the link and send an error. From lnd's PoV errors are benign, but I believe that other implementations force close upon receiving the error.
- If other implementations would force close anyways due to our error, does it make sense to just make this
l.faila force close?
There was a problem hiding this comment.
So I believe it's fine to not force close and instead shut down the link.
Given that if a commitment sig isn't sent after/during this early fulfill, IMO just taking down the link lets things resume vs the hard route of needing to eat those chain fees.
Crypt-iQ
force-pushed
the
settle_pipeline_check
branch
from
02e7f75
to
f01ed87
Compare
The counter-party shouldn't be doing this anyways as they would be giving away a preimage for free. Them doing this would bork their own channel due to open circuits not getting trimmed on startup. Removing this faulty behavior also makes it easier to reason about the circuit logic.
Crypt-iQ
force-pushed
the
settle_pipeline_check
branch
from
f01ed87
to
0ebfdc4
Compare
The counter-party shouldn't be doing this anyways as they would be giving away a preimage for free. Them doing this would bork their own channel due to open circuits not getting trimmed on startup. Removing this faulty behavior also makes it easier to reason about the circuit logic.