[KILO]: DynComms Prefactor: Refactor/evaluate htlc view #9097
Conversation
ProofOfKeags
added
refactoring
dynamic commitments
size/kilo
|
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/evaluate-htlc-view
branch
from
1a3d207
to
2eb03bb
Compare
|
@yyforyongyu: review reminder |
ProofOfKeags
force-pushed
the
refactor/evaluate-htlc-view
branch
from
2eb03bb
to
96dcf2d
Compare
ProofOfKeags
changed the title
ProofOfKeags
force-pushed
the
refactor/evaluate-htlc-view
branch
2 times, most recently
from
238720a
to
d2e2ef6
Compare
| // corresponding to whoseCommitmentChain. | ||
| isUncommitted := func(update *paymentDescriptor) bool { | ||
| switch update.EntryType { | ||
| case Add: |
There was a problem hiding this comment.
Just looking for a sanity check on my understanding here, nothing actionable:
- Previously we would only mutate entries with zero add height that are not in
skipUs/skipThemmaps. - This change reduces the check to only look at add height.
This is okay because:
- An
Addentry is only inskipUs/skipThemif a settle/fail entry exits for them in the other party's log. - We can't have an
Addwith zero add height in theskiplist, because the add MUST be irrevocably committed before we try to resolve it
There was a problem hiding this comment.
Yes that's right. However I'd also offer a different way of looking at it:
- Adds only ever have the add height set.
- Settles/Fails only have the remove height set.
- FeeUpdates have both.
- The definition of "uncommitted" is not having the correct entry from the above list set.
- The point of returning the uncommitted updates is that we want to set the heights in the outer scope as opposed to threading the mutateState argument.
- When the next height gets locked in, everything that is uncommitted gets its heights set (for the given
whoseCommitChain)
| // TestProcessAddRemoveEntry tests the updating of our and their balances when | ||
| // we process adds, settles and fails. It also tests the mutating of add and | ||
| // remove heights. | ||
| func TestProcessAddRemoveEntry(t *testing.T) { |
There was a problem hiding this comment.
CI failing rn, but I would like to sanity check coveralls to make sure we're not losing any coverage before merge.
The purpose of this commit is to begin the process of packing symmetric fields into the newly introduced Dual structure. The reason for this is that the Dual structure has a handy indexing method where we can supply a ChannelParty and get back a value. This will cut down on the amount of branching code in the main lines of the codebase logic, making it easier to follow what is going on.
ProofOfKeags
force-pushed
the
refactor/evaluate-htlc-view
branch
from
d2e2ef6
to
35cef2b
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.
ProofOfKeags
force-pushed
the
refactor/evaluate-htlc-view
branch
4 times, most recently
from
7126446
to
7e3cfe2
Compare
|
CI is "as green as it gets". Coveralls unit is flat. All comments addressed. One possible TODO outstanding launched by a comment from @carlaKC. Re-requesting for final confirmation. |
ProofOfKeags
force-pushed
the
refactor/evaluate-htlc-view
branch
from
7e3cfe2
to
9bb9dd7
Compare
| } | ||
| } | ||
|
|
||
| // TestProcessAddRemoveEntry tests the updating of our and their balances when |
There was a problem hiding this comment.
I understand that we do not need to check the mutate state anymore but where do we now test that the balances are updated correctly for the different payment descriptors ?
There was a problem hiding this comment.
Moreover the mutation is now pulled out into an upper level, are there unit-tests for it ? Cannot see a unit-test for computeView which now does the mutating now, maybe we could add one ?
There was a problem hiding this comment.
as in the lifetime payment flows? I can try to design a test around that but I don't think the lifetime payment flow stuff is mission critical. With regards to the state updates of the payment descriptor commit heights, that is already implicitly tested by the itests during any payment that gets made, I think.
There was a problem hiding this comment.
ok normally I tend to favour having a complete unit-test suite rather than relying on itests, but given the fact that computeview had no unit-test beforehand having an unit-test for computeView would be nice but it's a non-blocking comment.
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.
ProofOfKeags
force-pushed
the
refactor/evaluate-htlc-view
branch
from
9bb9dd7
to
5307e7a
Compare
It has been re-run and has passed. |
Change Description
This is the next logical chunk extracted off the front of #8755. This is a pure refactor change. The core behavior of the code modified should be identical.
Steps to Test
make unit
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.