search

lightningnetwork / lnd

Lightning Network Daemon ⚡️
MIT License
7.65k stars 2.07k forks source link

contractcourt: consider delivery addresses when evaluating toSelfAmount #8800

Closed ProofOfKeags closed 3 months ago

ProofOfKeags commented 4 months ago

Change Description

This commit fixes https://github.com/lightningnetwork/lnd/issues/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_delivery

Pull Request Checklist

Testing

Code Style and Documentation

📝 Please see our Contribution Guidelines for further guidance.

coderabbitai[bot] commented 4 months ago

[!IMPORTANT]

Review skipped

Auto reviews are limited to specific labels.

Labels to auto review (1) * llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

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?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` - `@coderabbitai help me debug CodeRabbit configuration file.` 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) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to do a full review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
ziggie1984 commented 4 months ago

https://github.com/lightningnetwork/lnd/pull/8808 was merged so this can be updated now.

Crypt-iQ commented 4 months ago

think gomod needs to be updated to compile. linter failing as well. code looks good can check again when these are fixed

ProofOfKeags commented 3 months ago

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:

I'm amenable to this.

Unless I'm missing something, we're currently missing any itest coverage for SettledBalance in general. I therefore think it would make sense to rename the integration test to testChannelSettledBalance (since we currently have a testChannelUnsettledBalance integration test) and also include coverage for cases where no DeliveryAddress has been specified.

Got it. I can add the case where it isn't specified.

Instead of only asserting that the channel's SettledBalance is NotZero at the last line of testCoopCloseWithExternalDeliveryImpl, I think it would make sense to assert that the SettledBalance is the actual value we expect. It would then also make sense to add a PushAmt value to the OpenChannelParams

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.

Furthermore, it would be great if you could add test coverage that proves that the toSelfAmount returns 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 a PushAmt value to the OpenChannelParams, you could potentially ensure that Alice sets the CloseAddress to Bob's address in the OpenChannelParams, but then let Bob initiate the CloseChannel RPC with one of Alice's addresses set as the DeliveryAddress in the CloseChannelRequest.

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?

ViktorTigerstrom commented 3 months ago

I'm amenable to this.

Awesome!

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.

Wouldn't we be able to just assert that the SettledBalance matches the sum of the TxOuts of the closing tx (returned by ht.MineClosingTx(chanPoint)) that we're interested in for the respective the test? You could then use the PushAmt to easily find Bob's output (not the one with the DeliveryAddress Alice has set), or do address matching (but that'll likely be much more complicated). If you feel that this get's overcomplicated though, feel free to implement your PushAmt less/greater idea though!

Edit: If you do implement my suggested amount assertion + my suggestion (1.) above, I do think it makes sense to also add assertion of the SettledBalance from Bob's side, to prove that if Alice doesn't set the DeliveryAddress to an address that Bob controls, Alice output amount isn't included in Bob's SettledBalance.

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?

Sorry, maybe I was a bit too unclear! I hope this is clearer, but let me know if I'm still too unclear: What I was referring to was that the current test code provides coverage for a case where only the isDeliveryOutput function matches in the toSelfAmount function. If you implement suggestion (1.) of my previous comment, that should add coverage for a case where only the isWalletOutput function matches. What my suggestion (3.) is trying to achieve, is to implement coverage for the edge case where there are outputs in the closing tx where both the isDeliveryOutput function and the isWalletOutput matches, just to prove that the function indeed sums the amount of those outputs if that's the case. My suggestion was attempting to achieve such a case in a simple way, as Alice set DeliveryAddress will then make the isDeliveryOutput function match, and Bob's set DeliveryAddress will then make the isWalletOutput function match when evaluating the SettledBalance from Alice perspective. Though, if you have an easier way of achieving a scenario where there are outputs in the closing tx where both the isDeliveryOutput function and the isWalletOutput matches, feel more than free to implement that!