-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Channeldb: Store HTLC Extra TLVs in Onion Blob Varbytes #7710
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to see you came around to this approach! May not be the ideal path, but it gets the job done and also lets us avoid migrations in this area as well. Before proceeding we should still do a manual comb through and ensure there're no other areas where an exact length is used (not serialized as var bytes).
5c9a182
to
a629a22
Compare
Took me a minute to grok, ngl, but seems like the lesser evil (by far)!
Will post separately! WDYT about this PR going in as-is vs combining in a larger preparatory route blinding PR? I lean towards to former, keeping DB workaround PRs more isolated but happy with either approach. |
References to
|
File | Line | Reference |
---|---|---|
channeldb/channel.go | 2004 | onionHash := sha256.Sum256(htlc.OnionBlob[:]) |
channeldb/channel.go | 2012 | onionHash := sha256.Sum256(htlc.OnionBlob[:]) |
channeldb/channel.go | 2104 | copy(onionAndExtraData, htlc.OnionBlob[:]) |
channeldb/channel.go | 2162 | htlcs[i].OnionBlob[:] |
contractcourt
decodePayload reads the HTLC's fixed size blob - these HTLCs are serialized and deserialized with our updated logic as part of commitSet
.
Legacy nodes used LogChainActions
/FetchChainActions
which also serializes and deserializes using var bytes.
File | Line | Reference |
---|---|---|
contractcourt/htlc_incoming_contest_resolver.go | 490 | onionReader := bytes.NewReader(h.htlc.OnionBlob[:]) |
lnwallet/channel.go
HTLCs are stored as var bytes in ChannelCommitment and re-loaded into memory.
File | Line | Reference |
---|---|---|
lnwallet/channel.go | 748 | copy(h.OnionBlob[:], htlc.OnionBlob) |
lnwallet/channel.go | 772 | copy(h.OnionBlob[:], htlc.OnionBlob) |
lnwallet/channel.go | 860 | OnionBlob: htlc.OnionBlob[:] |
a629a22
to
2850989
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love how small and focused this PR is🙏 Because the onion packet is enforced with the 1366 bytes restriction at the encoding/decoding level, I think it's safe to go with this approach. Nice workaround! Left a few comments and I think this is almost good to go⛵️
724d177
to
08ede0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM🏖
08ede0f
to
d1b5c0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart workaround, chapeau 🎓 :)
@@ -612,8 +614,10 @@ func TestChannelStateTransition(t *testing.T) { | |||
LogIndex: uint64(i * 2), | |||
HtlcIndex: uint64(i), | |||
} | |||
htlc.OnionBlob = make([]byte, 10) | |||
copy(htlc.OnionBlob[:], bytes.Repeat([]byte{2}, 10)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Wondering where this testcase came from, I think onion-blobs were always 1366 in size also before this change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well previously a OnionBlob
was just a byte slice, so they could be less than 1366. They never would be, as we wouldn't be able to read them, but unit tests could get away with junk data.
TL;DR: past selves be short-cuttin :')
We know that onion blobs in lightning are _exactly_ 1366 bytes in lightning, but they are currently expressed as a byte slice in channeldb's HTLC struct. Blobs are currently serialized as var bytes, so we can take advantage of this known length and variable length to add additional data to the inline serialization of our HTLCs, which are otherwise not easily extensible (without creating a new bucket).
Take advantage of the variable byte encoding for a known length field to extend HLTCs with additional data.
d1b5c0a
to
8a9dd10
Compare
Latest push just addresses nits and adds some comments, no functionality changes. |
@Roasbeef: review reminder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome changes! very clear and concise. Well done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🌴
Opening this in draft to illustrate the scope of a possible solution for #7297.
This PR demonstrates a workaround for persisting TLV data that is transmitted in
update_add_hltc
:OnionBlob
for HTLCs is serialized as var bytes, but we know this will always be 1366 bytes (as the spec demands).OnionBlob
+TLVStream
as a single var bytes blob where we previously had justOnionBlob
1366 + len(TLV stream)
1366
into theOnionBlob
and treat the rest as our TLV StreamWhen we want to save extra data (as provided in our
PaymentDescriptor
, all we have to do is encode the TLVs and setchanneldb.HTLC.ExtraData
(see last commit for example of where we'd need to set this).