Skip to content

Fix ability for plugins to exchange custom Lightning Messages with other LN nodes [PeerSwap]#2495

Merged
remyers merged 7 commits intoACINQ:masterfrom
remyers:peerswap-plugin-prep
Jan 27, 2023
Merged

Fix ability for plugins to exchange custom Lightning Messages with other LN nodes [PeerSwap]#2495
remyers merged 7 commits intoACINQ:masterfrom
remyers:peerswap-plugin-prep

Conversation

@remyers
Copy link
Contributor

@remyers remyers commented Nov 18, 2022

These changes are a prerequisite for the PeerSwap plugin.

This PR introduces a potentially breaking change to existing plugins by changing how the codec parses unknown messages.

Custom Lightning Messages were previously assumed to be prepended with two-bytes for the size of the unknown message blob. Now unknown messages are assumed to be sent as just the custom message type followed by a blob of data, with no assumption that the first two bytes give the size of the following blob.

Unknown Lightning Messages sent to a channel will be forwarded to the channel peer if they are listed as a custom message used by a loaded plugin.

The change to unknownMessageCodec caused channelUpdateCodecWithType to incorrectly return success with an UnknownMessage, instead of failure. This was solved by using just the LightningMessageCodec instead of meteredLightningMessageCodec which fell back to UnknownMessage for invalid messages.

I also unsealed TransactionWithInputInfo so that plugins can use checkSpendable with custom on-chain transactions.

@thomash-acinq
Copy link
Contributor

Custom Lightning Messages were previously assumed to be prepended with two-bytes for the size of the unknown message blob. Now unknown messages are assumed to be sent as just the custom message type followed by a blob of data, with no assumption that the first two bytes give the size of the following blob.

This is an assumption of TLVs, does it mean that you're using messages that are not TLVs? Why?

@remyers remyers force-pushed the peerswap-plugin-prep branch from 7cf95a0 to 61df644 Compare November 18, 2022 12:44
@remyers
Copy link
Contributor Author

remyers commented Nov 18, 2022

This is an assumption of TLVs, does it mean that you're using messages that are not TLVs? Why?

The length is still implicitly encoded, none of the other Lightning Message codecs explicitly encode the length of the payload like codecUnknownMessage did before this change.

@thomash-acinq
Copy link
Contributor

The length is still implicitly encoded, none of the other Lightning Message codecs explicitly encode the length of the payload like codecUnknownMessage did before this change.

It looked like a TLV at first glance but you're right, it's not, so the tag doesn't have to be followed by a size.

@remyers remyers force-pushed the peerswap-plugin-prep branch from 61df644 to 5565bad Compare November 18, 2022 14:29
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #2495 (a6e6527) into master (9ae4dea) will increase coverage by 0.02%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master    #2495      +/-   ##
==========================================
+ Coverage   84.98%   85.01%   +0.02%     
==========================================
  Files         198      200       +2     
  Lines       15839    15833       -6     
  Branches      682      675       -7     
==========================================
- Hits        13461    13460       -1     
+ Misses       2378     2373       -5     
Impacted Files Coverage Δ
...rc/main/scala/fr/acinq/eclair/io/Switchboard.scala 84.61% <66.66%> (-1.10%) ⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 89.64% <100.00%> (+0.16%) ⬆️
...fr/acinq/eclair/wire/protocol/FailureMessage.scala 83.56% <100.00%> (+0.46%) ⬆️
.../eclair/wire/protocol/LightningMessageCodecs.scala 99.35% <100.00%> (-0.65%) ⬇️
...-core/src/main/scala/fr/acinq/eclair/package.scala 74.28% <0.00%> (-8.41%) ⬇️
...q/eclair/channel/publish/ReplaceableTxFunder.scala 85.18% <0.00%> (-1.73%) ⬇️
...cinq/eclair/payment/receive/MultiPartHandler.scala 93.65% <0.00%> (-0.65%) ⬇️
...cala/fr/acinq/eclair/crypto/TransportHandler.scala 90.60% <0.00%> (-0.56%) ⬇️
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 86.27% <0.00%> (ø)
... and 17 more

thomash-acinq
thomash-acinq previously approved these changes Nov 18, 2022
@remyers
Copy link
Contributor Author

remyers commented Nov 21, 2022

This issue of whether or not to include the type prefix is resolved in BOLT 4 PR #979. We should be able to remove parsing of historical failure messages with un-prefixed channel_updates.

@remyers
Copy link
Contributor Author

remyers commented Nov 21, 2022

Is there a way to find out how many failure messages we get that are missing the type prefix for the embedded channel_update? It would be nice to remove entirely the ambiguity in parsing failure messages now that the spec is updated.

@tnull
Copy link

tnull commented Nov 21, 2022

Is there a way to find out how many failure messages we get that are missing the type prefix for the embedded channel_update? It would be nice to remove entirely the ambiguity in parsing failure messages now that the spec is updated.

I would also be interested in some numbers, as I'd like to be able to remove said parsing ambiguity from LDK/rust-lightning. However, I assume it's not ready yet, as LND still doesn't seem to write the type prefix?: lightningnetwork/lnd#6461

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM, only a couple of nits

@remyers remyers merged commit f901aea into ACINQ:master Jan 27, 2023
@remyers remyers deleted the peerswap-plugin-prep branch January 27, 2023 18:57
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.

5 participants