DynComms Development Branch #8755
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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
force-pushed
the
refactor/lnwallet-channel
branch
from
6d0dc33
to
f83ac41
Compare
18 tasks
ProofOfKeags
force-pushed
the
refactor/lnwallet-channel
branch
4 times, most recently
from
2778680
to
a4ee3cd
Compare
ProofOfKeags
force-pushed
the
refactor/lnwallet-channel
branch
3 times, most recently
from
54d6de0
to
27860c7
Compare
ProofOfKeags
force-pushed
the
refactor/lnwallet-channel
branch
from
27860c7
to
db59f02
Compare
ProofOfKeags
force-pushed
the
refactor/lnwallet-channel
branch
2 times, most recently
from
b2b0307
to
9a1a6a0
Compare
ProofOfKeags
force-pushed
the
refactor/lnwallet-channel
branch
from
9a1a6a0
to
3cca61f
Compare
ProofOfKeags
force-pushed
the
refactor/lnwallet-channel
branch
3 times, most recently
from
4f9cab7
to
8113434
Compare
ProofOfKeags
force-pushed
the
refactor/lnwallet-channel
branch
4 times, most recently
from
cf02b81
to
aaaaa3a
Compare
ProofOfKeags
force-pushed
the
refactor/lnwallet-channel
branch
from
98e9ac1
to
a3f6644
Compare
This was referenced Jul 29, 2024
ProofOfKeags
force-pushed
the
refactor/lnwallet-channel
branch
from
a3f6644
to
4a29fab
Compare
This commit begins the process of moving towards a more principled means of state tracking. We eliminate the mutateState argument from processAddEntry and processRemoveEntry and move the responsibility of mutating said state to the call-sites. The current call-sites of these functions still have their *own* mutateState argument which will be eliminated during upcoming commits. However, following the principle of micro-commits I opted to break these changes up to make review simpler.
This commit redoes the API and semantics of processFeeUpdate to make it consistent with the semantics of it's sister functions. This is part of an ongoing series of commits to remove mutateState arguments pervasively from the codebase. As with the previous commit this makes state mutation the caller's responsibility. This temporarily increases code duplication at the call-sites, but this will unlock other refactor opportunities.
In this commit we observe that the previous commit reduced the role of this function to a single assignment statement with numerous newly irrelevant parameters. This commit makes the choice of inlining it at the two call-sites within evaluateHTLCView and removing the funciton definition entirely. This also allows us to drop a huge portion of newly irrelevant test code.
This commit makes the observation that the nextHeight parameter of these two functions is no longer used by those funcitons themselves as a result of extracting the state mutation to the call-sites. This removes the parameter entirely.
In line with previous commits we are progressively removing the mutateState argument from this call stack for a more principled software design approach. NOTE FOR REVIEWERS: We take a naive approach to updating the tests here and simply take the functionality we are removing from evaluateHTLCView and run it directly after the function in the test suite. It's possible that we should instead remove this from the test suite altogether but I opted to take a more conservative approach with respect to reducing the scope of tests. If you have opinions here, please make them known.
This commit removes another raw boolean value and replaces it with a more clear type/name. This will also assist us when we later try and consolidate the logic of evaluateHTLCView into a single coherent computation.
This commit moves the collection of updates behind a Dual structure. This allows us in a later commit to index into it via a ChannelParty parameter which will simplify the loops in evaluateHTLCView.
This commit simplifies how we compute the commitment fee rate based off of the live updates. Prior to this commit we processed all of the FeeUpdate paymentDescriptors of both ChannelParty's. Now we only process the last FeeUpdate of the OpeningParty
We had four for-loops in evaluateHTLCView that were exact mirror images of each other. By making use of the new ChannelParty and Dual facilities introduced in prior commits, we consolidate these into two for-loops.
This further reduces loop complexity in evaluateHTLCView by using explicit filter steps rather than loop continue statements.
This commit observes that processAddEntry and processRemoveEntry are only invoked at a single call-site. Here we inline them at their call-sites, which will unlock further simplifications of the code that will allow us to remove pointer mutations in favor of explicit expression oriented programming. We also delete the tests associated with these functions, the overall functionality is implicitly tested by the TestEvaluateHTLCView tests.
Here we return the balance deltas from evaluateHTLCView rather than passing in references to variables that will be modified. It is a far cleaner and compositional approach which allows readers of this code to more effectively reason about the code without having to keep the whole codebase in their head.
This commit takes the CommitChainEpochHistory defined in the last commit and adds it to the OpenChannel structure. As of this commit it is essentially redundant with the ChanCfgs but it will capture the history of the ChanCfgs when we add the ability to update them.
…each This commit changes the way we create breach retributions to use the CsvDelay we compute from the CommitChainEpochHistory so as to account for the possibility that the channel parameters have changed since opening.
ProofOfKeags
force-pushed
the
refactor/lnwallet-channel
branch
from
f24deb3
to
b3cb1b7
Compare
8 tasks
|
@ProofOfKeags, remember to re-request review from reviewers when ready |
|
Completed a high level pass on this, great refactor with really nice commit structure 🙏 |
ProofOfKeags
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Description
Depends on #8270.Depends on #8981
This is a series of single responsibility commits that improve the state of affairs within the channel state machine. The ultimate goal here in service of the Dynamic Commitments project is to make it such that we no longer have the
updateLogs containpaymentDescriptors as the fundamentally tracked unit of update. This is unfortunately a long road which requires a number of changes which on their face do not seem necessary. I assure you that every one of these commits is ultimately in service of that goal, though.It is highly recommended that you review commits one by one and in order. One of the consequences of having atomic, semantics-invariant commits is that some subsequent commits may ultimately make prior commits partially or fully obsolete. This is the price of making commit-by-commit review easy.
There is no specific requirement that every commit in this PR go in at once. This PR is mergeable at every commit and will be semantics-invariant.
Godspeed 🫡
Steps to Test
make unit pkg=lnwallet
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.