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
[5/5]: lnwallet: extensions to aux leaf store to integrate custom channels #8641
base: aux-leaf-signing
Are you sure you want to change the base?
Conversation
Important Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Pull reviewers statsStats of the last 30 days for lnd:
|
lnwallet/commitment.go
Outdated
@@ -654,25 +654,29 @@ type AuxLeafStore interface { | |||
// FetchLeavesFromView attempts to fetch the auxiliary leaves that | |||
// correspond to the passed aux blob, and pending fully evaluated HTLC | |||
// view. | |||
FetchLeavesFromView(prevBlob tlv.Blob, view *HtlcView, | |||
FetchLeavesFromView(chanState *channeldb.OpenChannel, prevBlob tlv.Blob, |
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.
Alternatively could just be that special funding blob (the one in the open channel struct), we could also use a type alias here. Unsure though if we do need all that other information or not.
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.
We need the local/remote channel config, custom blob, thaw height, initiator and channel type. So it's easiest to just pass in the whole channel state. Not sure what a type alias would give us? Other than perhaps being a bit shorter.
lnwallet/commitment.go
Outdated
// of the pkScript and CLTV timeout, and the value is the index the | ||
// output should be placed at. The CLTV timeout is encoded as a big | ||
// endian before hashing and is assumed to be 0 for non-HTLC outputs. | ||
// sha256(pkScript || bigEndian(cltvDelta)) -> outputIndex |
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.
IIUC, this also incudes the value as well.
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.
Ah yes, good catch. Fixed.
lnwallet/commitment.go
Outdated
// lets us skip sending the entire transaction over, instead we'll just | ||
// send signatures. | ||
if auxLeaves.IsSome() && | ||
auxLeaves.UnsafeFromSome().CustomOutputOrder != nil { |
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.
If we want to mask this a bit more, then we can have a functional option to pass in a new custom sorting interface/type. Default is the existing one, then we know to override it at the config level for these special channels.
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.
Good point. I'm meeting you half way: Instead of passing in a function, we now require a sort function to be returned. But that way at least the sorting logic is not in the lnd
code base anymore.
lnwallet/commitment.go
Outdated
@@ -660,7 +660,8 @@ type AuxLeafStore interface { | |||
// correspond to the passed aux blob, and pending original (unfiltered) | |||
// HTLC view. | |||
FetchLeavesFromView(chanState *channeldb.OpenChannel, prevBlob tlv.Blob, | |||
unfilteredView *HtlcView, | |||
unfilteredView *HtlcView, isOurCommit bool, ourBalance, | |||
theirBalance lnwire.MilliSatoshi, |
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.
Balances needed for the new sorting algo?
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.
Yes, for sorting, making sure we have non-dust BTC amounts for any asset amounts and for determining whether we need commitment anchor outputs (which we need to create exclusion proofs for).
d288b6a
to
f7f875b
Compare
cb514a1
to
f35aead
Compare
f35aead
to
cf965f0
Compare
1858e19
to
ffa19c0
Compare
cf965f0
to
0e6a9dd
Compare
This PR should now contain everything from the PoC branch and is ready for review. |
5a834c1
to
7b9e627
Compare
c0b5902
to
6123a1c
Compare
3b45569
to
12d4694
Compare
12d4694
to
44fe836
Compare
This commit adds an optional data parser that can inspect and in-place format custom data of certain RPC messages.
44fe836
to
5401a54
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.
I appreciate that each commit pretty much does one thing 👍
# Allow fmt.Printf() in lncli. | ||
- path: cmd/lncli/* | ||
# Allow fmt.Printf() in commands. | ||
- path: cmd/commands/* |
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.
This refactoring commit seems unnecessary in this PR. Would be simpler to do this sort of change once the saga is merged into master
perhaps.
The commit body should explain why this change is necessary.
// AddHeight returns a pointer to the height at which the HTLC was added to the | ||
// commitment chain. The height is returned based on the chain the HTLC is | ||
// being added to (local or remote chain). | ||
func AddHeight(htlc *PaymentDescriptor, remoteChain bool) *uint64 { | ||
if remoteChain { | ||
return &htlc.addCommitHeightRemote | ||
} | ||
|
||
return &htlc.addCommitHeightLocal | ||
} | ||
|
||
// RemoveHeight returns a pointer to the height at which the HTLC was removed | ||
// from the commitment chain. The height is returned based on the chain the HTLC | ||
// is being removed from (local or remote chain). | ||
func RemoveHeight(htlc *PaymentDescriptor, remoteChain bool) *uint64 { |
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.
Why do these functions return pointer types?
type AuxDataParser interface { | ||
// InlineParseCustomData replaces any custom data binary blob in the | ||
// given RPC message with its corresponding JSON formatted data. This | ||
// transforms the binary (likely TLV encoded) data to a human-readable | ||
// JSON representation (still as byte slice). | ||
InlineParseCustomData(msg proto.Message) error | ||
} |
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.
This commit adds an interface without an implementation. Was that intentional? I think we should clarify the commit message if it was intentional. Otherwise the implementation is unexpectedly missing.
// When the HTLC was decoded from the wire, the ExtraData field was | ||
// parsed and split into the BlindingPoint and CustomRecords. Which | ||
// means we don't have to copy over the ExtraData itself into the | ||
// PaymentDescriptor, just the encoded CustomRecords. |
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.
I'm not sure if this comment is true. The ExtraData
field still might contain data for a field which is lower in type than the custom records type range but not a defined field on the HTLC.
To avoid sorting issues with identical HTLCs (equal size, equal payment hash, equal CLTV), we need to also use the HTLC index to be able to distinguish between them.
Depends on #8632.
Extends the aux leaf store with more required parameters.
Link to all PRs in the saga: