Skip to content

htlcswitch: allow variable length failure messages in channel state machine#7031

Closed
joostjager wants to merge 2 commits intolightningnetwork:masterfrom
bottlepay:receive-malformed-length-encoding
Closed

htlcswitch: allow variable length failure messages in channel state machine#7031
joostjager wants to merge 2 commits intolightningnetwork:masterfrom
bottlepay:receive-malformed-length-encoding

Conversation

@joostjager
Copy link
Contributor

@joostjager joostjager commented Oct 13, 2022

This PR shows an alternative approach to support variable length failure messages that is less invasive than #7030.

Required for #6913

@joostjager joostjager changed the title htlcswitch: allow variable length failure messages htlcswitch: allow variable length failure messages in channel state machine Oct 13, 2022
@joostjager joostjager force-pushed the receive-malformed-length-encoding branch from 6ccf5fa to ed7dcf8 Compare October 14, 2022 09:03
Copy link
Collaborator

@Crypt-iQ Crypt-iQ left a comment

Choose a reason for hiding this comment

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

not sure if this is still relevant, but left a comment

// bytes.
failReason := pd.FailReason
if len(pd.FailReason) >= varLenFailureMarkerLen {
failReason = failReason[varLenFailureMarkerLen:]
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 this logic would also have to be in reforwardSettleFails in the switch. otherwise i think it's possible that we receive a failure reason near 65535 bytes, add 300 bytes, and then the switch reforwards the failure and the incoming link is unable to send the message given the wire constraint

Copy link
Contributor Author

@joostjager joostjager Nov 14, 2022

Choose a reason for hiding this comment

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

Ah yes, it should indeed. But I guess this PR is no longer relevant as we decided to only allow 256+ byte failures. Adding the marker length was intended as a solution to also support variable length short failures.

@joostjager
Copy link
Contributor Author

In #6913 (comment), we decided to simply not allow failure messages shorter than 256 bytes. Closing this for now.

@joostjager joostjager closed this Dec 14, 2022
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.

2 participants