contractcourt: consider delivery addresses when evaluating toSelfAmount #8800
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
ProofOfKeags
requested review from
ViktorTigerstrom,
a team,
morehouse and
ziggie1984
and removed request for
a team
ProofOfKeags
changed the title
ProofOfKeags
requested review from
Crypt-iQ
and removed request for
morehouse and
ziggie1984
ProofOfKeags
force-pushed
the
bugfix/8535
branch
3 times, most recently
from
361829c to
9fcac8a
Compare
There was a problem hiding this comment.
Had some minor comments, nicely used the function in the fn package 👏.
I think it might make sense to add the func commenting style into our guidelines so that we don't diverge too much in the codebase in general. (referring to this: // isWalletOutput :: *TxOut -> bool)
Apart from that this change looks good, Release notes for 18.1 are missing.
| }) | ||
| } | ||
|
|
||
| // testCoopCloseWithExternalDeliveryImpl ensurees that we have a valid settled |
There was a problem hiding this comment.
Nit: :s/ensurees/ensures/g
| // belongs to the LND wallet. We use this as a heuristic in the case | ||
| // where we might be looking for spendable force closure outputs. | ||
| // | ||
| // isWalletOutput :: *TxOut -> bool |
There was a problem hiding this comment.
I think it makes sense to either introduce this new way of commenting function into the coding docs so that we don't diverge in our coding style ?
There was a problem hiding this comment.
I'm not sure what the specific ask is here.
The annotation I have here is for informational purposes only to alleviate the need to squint at the function itself to figure out its "shape". The format I use is stolen from Haskell and subtle variations can be found in Standard ML, OCaml, Haskell, Idris, Agda, F#, etc.
In recent years it has been adapted into other more mainstream languages in documentation notes like you can see here, here and here. The reason for this is that it's a fairly space efficient way to communicate the shape of the function without diving into the implementation details.
Are you saying that we need a note in the style guidelines to bless this? Or is it to describe in detail how to interpret a standalone type signature? What would you like to see?
There was a problem hiding this comment.
Are you saying that we need a note in the style guidelines to bless this?
Yes this is what I meant, because I find it useful.
contractcourt/chain_watcher.go
Outdated
| _, addrs, _, err := txscript.ExtractPkScriptAddrs( | ||
| // Doesn't matter what net we actually pass in. | ||
| txOut.PkScript, &chaincfg.TestNet3Params, | ||
| // It doesn't matter what chain params we use here. |
There was a problem hiding this comment.
Nit: explanation why it does not matter would be helpful.
There was a problem hiding this comment.
I wish I knew 😅. I've restored the comment to its original text now. Also worth noting that this is inside the txscript package in btcd.
| } | ||
|
|
||
| // Here we define a function capable of identifying whether an output | ||
| // belongs to the LND wallet. We use this as a heuristic in the case |
There was a problem hiding this comment.
I am not sure I understand this comment, seems to me we currently only use this function in the cooperate close case, why do we mention the force closure output here ?
There was a problem hiding this comment.
While I hadn't looked at this prior to you pointing this out, it does appear that this function's only use site is in a context where we already assume/know it to be a coop close. That said, given that the function is named toSelfAmount, I think it's fair to keep it general enough to handle either case.
Generally, since the logical dependency relationship is that the caller depends on the callee, I don't think it's reasonable to assume how a function might be called. Perhaps if we want to limit it I can rename it to coopToSelfAmount and then trim out any commentary about how it might be used in a FC case. Perhaps this is the right compromise.
|
#8808 was merged so this can be updated now. |
|
think gomod needs to be updated to compile. linter failing as well. code looks good can check again when these are fixed |
ProofOfKeags
force-pushed
the
bugfix/8535
branch
4 times, most recently
from
358e652 to
96fb23f
Compare
| // whether it was from the upfront shutdown, from the delivery address | ||
| // requested at close time, or even an automatically generated one. All | ||
| // coop-close cases can be identified in the following manner: | ||
| shutdown, _ := c.cfg.chanState.ShutdownInfo() |
There was a problem hiding this comment.
so we do not handle DB error, here?
There was a problem hiding this comment.
Since this function doesn't return an error, if we can't reach the shutdown info for whatever reason we treat it as non-existent. Why it failed is inconsequential.
There was a problem hiding this comment.
Oh okay, I was just thinking that maybe if there is something wrong with the DB we can catch it as soon as we encounter it here.
There was a problem hiding this comment.
If this function already had an error in its signature then I'd probably do that. But this would cause a currently non-erroring function to return an error which I'd like to avoid. Swallowing errors can be dangerous but in this case all it does is affect the numbers we report and I don't think that's especially problematic. If it comes out to result in bad UX's we can come back and address it but for now I'd recommend leaving it as is.
| ht.Run("set delivery address at open", func(t *testing.T) { | ||
| tt := ht.Subtest(t) | ||
| testCoopCloseWithExternalDeliveryImpl(tt, true) | ||
| }) | ||
| ht.Run("set delivery address at close", func(t *testing.T) { | ||
| tt := ht.Subtest(t) | ||
| testCoopCloseWithExternalDeliveryImpl(tt, false) | ||
| }) |
There was a problem hiding this comment.
Interesting choice, why this and not TDD?
There was a problem hiding this comment.
What do you mean by TDD? I'm only familiar with the acronym as Test Driven Development, and that is exactly what I've done here.
There was a problem hiding this comment.
Oh, I was referring to something like this:
source: https://golang.cafe/blog/golang-table-test-example
In the code being reviewed above, a number of things were rewritten twice.
There was a problem hiding this comment.
I stole the test vector approach from @ellemouton 's refactoring of one of my earlier itests here:
I also recall @yyforyongyu saying that on a go-forward basis we should discourage table tests like in the image you linked. I tend to agree with this assessment but also would defer this type of decision to the elder wizards. I don't have a principled opinion on how to code test vectors.
There was a problem hiding this comment.
Just my two cents,
For this case, I would do testCoopCloseWithExternalDeliveryImpl and testCoopCloseWithExternalDeliveryImplWithUpfronShutdown instead.
I like using table-driven tests when the outputs of a function are only determined by one or two inputs in a test setup, like this one or this one. They share common heuristics like,
- the method being tested is relatively simple - simple as in the number of inputs it takes and the number of outputs it returns, lines of code, logic, etc.
- the test itself is straightforward, which is usually not the case in itest as we often need a somewhat complicated setup to reach one or multiple states before the actual test.
- the assertions used in the test are NOT conditional based on the values of the table. An example I want to discourage is the multi-hop itest it took me quite some time to maintain while working on the sweeper series and I'm refactoring them now.
I'm also struggling to apply these rules of thumb from time to time because we need to improve the testability of the code. My general take is, as long as the tests are easy to maintain I'm happy. This means if table-driven tests are not used, I'm good, as the tests can always be refactored to use tables (low priority IMO); if table-driven tests are properly used, I'm good. I'm against improper usage of table-driven tests which lead to high maintenance costs.
So in most of cases, I'd avoid using table-driven tests in itest. I'd also recommend this Advanced Testing with Go talk, especially the "REPEAT YOURSELF" part.
Finally I would be very hesitant to make changes to any PR, unless critical, if it already got two approvals and no Request-for-change from others🤓
This commit fixes lightningnetwork#8535 by changing how we assess toSelfAmount inside the chainWatcher. In certain cases users may wish to close out channel funds to external delivery addresses set either during open or close. Prior to this change we only consider addresses that our wallet is aware of. This change now identifies outputs as to_self outputs if the delivery script matches OR if our wallet is aware of the address. In certain edge cases it can be possible for there to be more than one output that matches these criteria and in that case we will return the sum of those values.
ProofOfKeags
force-pushed
the
bugfix/8535
branch
from
a6296f4 to
1fea14f
Compare
There was a problem hiding this comment.
LGTM! Really elegant solution, great work 🚀!
Nit:
If anything, I think the test coverage could be extended a bit to improve the coverage further. I'm therefore leaving some suggestions below on how the coverage could be improved. None of this is blocking, and I'm fine with leaving it as is, so I leave it up to you to decide if you feel it's worth adding:
- Unless I'm missing something, we're currently missing any itest coverage for
SettledBalancein general. I therefore think it would make sense to rename the integration test totestChannelSettledBalance(since we currently have atestChannelUnsettledBalanceintegration test) and also include coverage for cases where noDeliveryAddresshas been specified. - Instead of only asserting that the channel's
SettledBalanceisNotZeroat the last line oftestCoopCloseWithExternalDeliveryImpl, I think it would make sense to assert that theSettledBalanceis the actual value we expect. It would then also make sense to add aPushAmtvalue to theOpenChannelParams. - Furthermore, it would be great if you could add test coverage that proves that the
toSelfAmountreturns the sum of all outputs for which the address either matches the delivery script or is an address that our wallet is aware of. To provide that coverage, after adding aPushAmtvalue to theOpenChannelParams, you could potentially ensure that Alice sets theCloseAddressto Bob's address in theOpenChannelParams, but then let Bob initiate theCloseChannelRPC with one of Alice's addresses set as theDeliveryAddressin theCloseChannelRequest.
Like I said, these are non-blocking suggestions. Great work 🎉!
I'm amenable to this.
Got it. I can add the case where it isn't specified.
This is a bit tricky as determining the balances of coop close transactions are subject to fee and output script factors and this can make solving for the "expected" amount equivalent to solving for the actual amount. We could use that PushAmt though to assert just that it's less/greater than some amount.
This is the suggestion I understand the least. I think the property that you are trying to ensure in this case is that if they each set their delivery address to one that the other controls, then their settled balances should always match each other, irrespective of what the push amount is. Am I interpreting this right? |
Awesome!
👍
Wouldn't we be able to just assert that the Edit:
Sorry, maybe I was a bit too unclear! I hope this is clearer, but let me know if I'm still too unclear: |
Change Description
This commit fixes #8535 by changing how we assess toSelfAmount inside
the chainWatcher.
In certain cases users may wish to close out channel funds to external
delivery addresses set either during open or close.
Prior to this change we only consider addresses that our wallet is
aware of and permitted the possibility of multiple to_self outputs,
which is impossible according to the protocol.
This change now identifies to_self outputs based off of matching
delivery scripts, and if no delivery script is set, fall back to
the first output that our wallet can recognize.
NOTE FOR REVIEWERS: The itest commit is ordered first to confirm that the test fails prior to the change. After the fix commit, the test passes. If you want to confirm that the test is correctly designed you can run the test prior to the fix commit.
Steps to Test
make itest icase=coop_close_with_external_deliveryPull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.