[KILO]: Quarantine paymentDescriptor to the lnwallet package#8981
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 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ProofOfKeags
added
the
size/kilo
ProofOfKeags
changed the title
ProofOfKeags
force-pushed
the
refactor/payment-descriptor-quarantine
branch
2 times, most recently
from
509032f to
32cdf57
Compare
ProofOfKeags
force-pushed
the
refactor/payment-descriptor-quarantine
branch
2 times, most recently
from
cc6272f to
548c6a5
Compare
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Very nice - left some comments re how we can break the large commit 05ac04d since this refactor is touching the most critical part.
carlaKC
left a comment
carlaKC
left a comment
There was a problem hiding this comment.
Beautiful cleanup, lowkey blows my mind that this works so cleanly. cACK, just did a first pass to familiarize myself - still need to do a more thorough run through of the details!
+1 for YY's commit restructuring comments.
This is in preparation for upcoming changes where we try and change the fundamental unit of the update log to a LogUpdate, as opposed to the former PaymentDescriptor.
Is there a PR/issue where we attempt this? Would like to add some context for myself.
ProofOfKeags
force-pushed
the
refactor/payment-descriptor-quarantine
branch
from
548c6a5 to
aebec95
Compare
ProofOfKeags
force-pushed
the
refactor/payment-descriptor-quarantine
branch
2 times, most recently
from
5096e9e to
dd4e90e
Compare
|
@ProofOfKeags, remember to re-request review from reviewers when ready |
ProofOfKeags
force-pushed
the
refactor/payment-descriptor-quarantine
branch
from
dd4e90e to
fb3f3dc
Compare
I will do that but I don't want to hear any complaints about diff churn. There was a reason this was done as one commit originally.
|
ProofOfKeags
force-pushed
the
refactor/payment-descriptor-quarantine
branch
6 times, most recently
from
2c0b8a3 to
baf8f4f
Compare
Here we add a function that is capable of recovering LogUpdates from paymentDescriptors and we refactor the lnwallet code to use this rather than doing JIT inline construction of the LogUpdates.
This is done as part of a systematic removal of PaymentDescriptor from the mechanics of the htlcswitch package.
This is done as part of a systematic removal of PaymentDescriptor from the mechanics of the htlcswitch package.
…ignature This is part of a systematic removal of PaymentDescriptor from the mechanics of the htlcswitch package.
This is part of a systematic removal of PaymentDescriptor from the mechanics of the htlcswitch package.
…ature This is part of a systematic removal of PaymentDescriptor from the mechanics of the htlcswitch package.
…ll signature This is part of a systematic removal of PaymentDescriptor from the mechanics of the htlcswitch package.
…returns This is part of a systematic removal of PaymentDescriptor from the public API of the lnwallet package. This marks the last change needed before we make the PaymentDescriptor structure private.
…ails This is part of a systematic removal of PaymentDescriptor from the mechanics of the htlcswitch package.
This function is no longer used as of the last commit and it is the last remaining leak of the PaymentDescriptor type through the public API.
ProofOfKeags
force-pushed
the
refactor/payment-descriptor-quarantine
branch
from
4a2a693 to
f3f2b4f
Compare
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
LGTM🦾
In the future it may be nice to have some docs explaining the relationship among htlcPacket, paymentDescriptor and LogUpdate, which I believe are the core data structures used in different subsystems.
In case others wanna review it, my summary is: overall this PR is a refactoring effort for lnwallet, with the only functional change in this line.
- Before this PR, the log updates in the
fwdPkgare pointing to a list of unfiltered updates, withfwdPkg.Addpointing to all the adds andfwdPkg.SettleFailspointing to all the settles/fails we've received and stored inremoteLogUpdate. - After this PR, these updates are now filtered using the remote/local chain tail, which I believe is the right change.
|
Looks like the only test failures are from something going on with the sweeper? |
yeah flakes |
The objective of this commit is to make paymentDescriptor a private data structure so we can quarantine it to the lnwallet package. To accomplish this we had to prevent it from leaking out via the arguments or return values of the public functions in lnwallet. This naturally had consequences for the htlcswitch package as we choose other mechanisms for tracking the data that paymentDescriptor was responsible for. Astoundingly, this was highly successful and allowed us to remove a ton of redundant code. The diff for this commit represents a substantial reduction in total lines of code as well as extraneous arguments and return values from key functions. This also sets the stage for future commits where we actually will be attempting to rid lnwallet of paymentDescriptor completely.
ProofOfKeags
force-pushed
the
refactor/payment-descriptor-quarantine
branch
from
f3f2b4f to
93d17a4
Compare
6 participants
Change Description
This PR's main contribution is quarantining the paymentDescriptor type to the lnwallet package. This is in preparation for upcoming changes where we try and change the fundamental unit of the update log to a LogUpdate, as opposed to the former PaymentDescriptor. This is because we will functionally eliminate the PaymentDescriptor as a meaningful idea and instead just have a linear sequence of log updates accompanied by some sparse indices for other metadata. To effectively accomplish this, however, we have to reduce the scope of making significant changes to this data structure and that begins by making sure it does not leak outside the package.
The primary observation that allows us to do this is to realize that the only other place this is currently used is in the
processRemote[Adds|SettleFails]functions within the ChannelLink. What is notable about this is that this information is already duplicated in the forwarding package itself. A forwarding package is already a sequence of updates that is committed by a given commitment transaction. Since the link does not make use of the metadata beyond that we simply just make it use the LogUpdates inside the forwarding package rather than the accompanying payment descriptors.In my best assessment this is completely safe to do and so far none of the testing I have done has revealed that we cannot do this. However, it is worth paying special attention to what I am doing here and ensuring that it does not miss an important detail on which we rely.
Steps to Test
make unit pkg=lnwallet
make itest
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.