multi: explicitly define InboundFees in ChannelUpdate and ChannelEdgePolicy #9897
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
8c460c1 to
61c798b
Compare
| } | ||
|
|
||
| if len(tlvRecords) != 0 { | ||
| a.ExtraOpaqueData = tlvRecords |
There was a problem hiding this comment.
This will still include the inbound type. Is that on purpose? Or should we remove the inbound fee type as we should never consume/read that from the extra opaque data from now on?
Not sure on the implications, just thinking aloud.
There was a problem hiding this comment.
yeah this is on purpose. For v1 messages at least, we just continue to store the entire TLV blob. The same is done in channel_ready for the new NextLocalNonce record.
In this commit, we start validating the extra opaque data of a channel edge policy before persisting it. We just check that the data is valid TLV. NOTE: we recently [started validating](lightningnetwork@1410a09) this at the lnwire level. So really, no new update will reach the DB layer without this already being checked. But we check it again here so that the DB API behaves correctly as its own unit.
Here we add an explicit InboundFee field to the ChannelEdgePolicy struct. Then, in the graph KVStore, at deserialisation time, we extract the InboundFee from the ExtraOpaqueData. Currently we do this at higher levels but we are going to move it to the DB layer so that when we add the SQL implementation of the graph store, we can have explicit columns for inbound fees. We need to account for the fact that we may have invalid TLV already persisted though and we dont want to fail if we deserialise those necessarily. So we return ErrParsingExtraTLVBytes now if we fail to parse the extra bytes as TLV and then we let the callers handle it similarly to how ErrParsingExtraTLVBytes is handled in that we dont necessarily fail if we receive one of these errors. As of this commit, we can now expect the InboundFee field of a ChannelEdgePolicy to be set (if inbound fees are set on the policy) for any update that we read from disk.
For any call-site where we extract inbound fees from a models.ChannelEdgePolicy object that was deserialised from disk, we can now just use the new InboundFee field on the object since we know that it would have been populated at deserialisation time. Note that for all these call-sites, if a failure previously happened on decoding of the TLV stream, the error would be ignored and the edge would just be skipped. This behaviour is now still the same given how ErrParsingExtraTLVBytes is handled on the DB layer.
Like the previous commit, here we can start directly using the InboundFee on the models.ChannelEdgePolicy object since we know we read it from disk and so the InboundFee field will be populated accordingly. NOTE: unlike the previous commit, behaviour is slightly different here since previously we would error out here if TLV parsing failed whereas now, the DB call will just skip the error and return a nil policy. This should be ok since this is explicitly only dealing with our own updates and so our TLV should always be valid.
In this commit, we make sure to set the new field wherever appropriate. This will be any place where the ChannelEdgePolicy is constructed other than its disk deserialisation.
Now that we know that the InboundFee on the ChannelEdgePolicy is always set appropriately, we can update the GraphCache UpdatePolicy method to take the InboundFee directly from the ChannelEdgePolicy object.
And then use that new field instead of parsing from extra opaque data.
Also add a temporariy replace to the tlv package which can be removed as soon as the PR that includes this commit is merged and a new tag for the tlv package has been created.
Remove the previously added TODOs which would extract InboundFee info from the ExtraOpaqueData of a ChannelUpdate at the time of ChannelEdgePolicy construction. These can now be replaced by using the newly added InboundFee record on the ChannelUpdate message.
| var inboundFee lnwire.Fee | ||
| typeMap, err := edge.ExtraOpaqueData.ExtractRecords(&inboundFee) | ||
| if err != nil { | ||
| return edge, fmt.Errorf("%w: %w", ErrParsingExtraTLVBytes, err) |
There was a problem hiding this comment.
Q: do we plan to do anything with this error? Right now we return it and then match everywhere if it's ErrParsingExtraTLVBytes to ignore it. Maybe we could use the same approach we already do in rpcserver.go's extractInboundFeeSafe(data lnwire.ExtraOpaqueData) lnwire.Fee and ignore it right here at parsing time? wdyt?
There was a problem hiding this comment.
the thing that is nice about returning it is then we skip the whole edge rather than just leaving it as &lnwire.Fee{} and still making use of the edge.
leaving it as is lets us not load these invalid policies.
There was a problem hiding this comment.
(gonna request override merge in the mean time - happy to address action items from this discussion in a follow up)
In this PR we add explicit fields for the InboundFee record on both the
lnwire.ChannelUpdatemessage as well as on ourmodels.ChannelEdgePolicyinternal type.Previously, we would extract the inbound fee record from the extra TLV bytes at any call-site where we needed it. But now we add explicit fields for it which will also open the way for our SQL implementation of the graph store to have an explicit column for it in the
channel_policiestable. It will also mean that for our SQL impl, we dont need the extra round trip of fetching TLV data in order to get the inbound fee for an update.PR structure
We start bottom up:
lnwire.ChannelUpdatetype with the new TLV recordOnce this PR is merged, we can tag the TLV package and remove the replace added here in the go.mod file
Part of #9795