search

lightningnetwork / lnd

Lightning Network Daemon ⚡️
MIT License
7.72k stars 2.09k forks source link

Beat [3/4]: prepare resolvers to handle the `blockbeat` #9276

Open yyforyongyu opened 1 week ago

yyforyongyu commented 1 week ago

Depends on

NOTE: itest is fixed in the final PR

Turns out mounting blockbeat in ChannelArbitrator can be quite challenging (unit tests, itest, etc). This PR attempts to implement it in hopefully the least disruptive way - only chainWatcher implements Consumer, and the contract resolvers are kept stateless (in terms of blocks). The main changes are,

  1. contract resolvers are refactored - the original Resolve method is broken into two steps: 1) Launch the resolver, which handles sending the sweep request, and 2) Resolve the resolver, which handles monitoring the spending of the output.
  2. chainWatcher implements Consumer in the following PR.

Alternatives

The original attempt is to make the resolvers subscribe to a blockbeat chan, as implemented in #8717. The second attempt is to make the resolvers also blockbeat Consumer, as implemented here.

This third approach is chosen as 1) it greatly limits the scope otherwise a bigger refactor of channel arbitrator may be needed, and 2) the resolvers can be made stateless in terms of blocks, and be fully managed by the channel arbitrator. In other words, when a new block arrives, the channel arbitrator decides whether to launch the resolvers or not, so the resolvers themselves don't need this block info.

In fact, there are only two resolvers that subscribe to blocks, the incoming contest resolver, which uses block height to decide whether to give up resolving an expired incoming htlc; and the outgoing contest resolver, which uses the block height to choose to transform itself into a timeout resolver. IMO if we can remove the inheritance pattern used in contest resolver -> time/success resolver and manage to transform resolvers in channel arbitrator, we can further remove those two block subscriptions. As for now, we can leave them there as they have little impact on the block consumption order enforced by the blockbeat.

coderabbitai[bot] commented 1 week ago

[!IMPORTANT]

Review skipped

Auto reviews are limited to specific labels.

:label: 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 , please review it.` - `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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.` - `@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 using 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. ### Other keywords and placeholders - Add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. - Add `@coderabbitai summary` to generate the high-level summary at a specific location in the PR description. - Add `@coderabbitai` anywhere in the PR title to generate the title automatically. ### CodeRabbit Configuration 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://docs.coderabbit.ai) for detailed information on how to use CodeRabbit. - Join our [Discord Community](http://discord.gg/coderabbit) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
github-actions[bot] commented 1 week ago

Pull reviewers stats

Stats of the last 30 days for lnd: User Total reviews Time to review Total comments
guggero
🥇		 25
▀▀▀▀		 15h 44m))
 30
▀▀
yyforyongyu
🥈		 12
▀▀		 1d 8h 24m))
▀		 33
▀▀▀
ellemouton
🥉		 8
▀		 16h 51m))
 21
▀▀
ziggie1984
 5
▀		 16h 15m))
 9
▀
ProofOfKeags
 4
▀		 5d 4h 23m))
▀▀		 14
▀
bhandras
 3
 1d 14h 22m))
▀		 2
dstadulis
 3
 1h 26m))
 5
Roasbeef
 2
 4d 13h 56m))
▀▀		 9
▀
ViktorTigerstrom
 2
 2d 14h 26m))
▀		 5
alexbosworth
 1
 4d 11h 58m))
▀▀		 1
ffranr
 1
 2h 9m))
 0
bitromortac
 1
 16h 16m))
 0
saubyk
 1
 17h 10m))
 0
Roasbeef commented 4 days ago

IMO if we can remove the inheritance pattern used in contest resolver -> time/success resolver and manage to transform resolvers in channel arbitrator, we can further remove those two block subscriptions. As for now, we can leave them there as they have little impact on the block consumption order enforced by the blockbeat.

Minor point, it it isn't inheritance, it's composition, with the inner resolver replacing the outer one. Go technically doesn't have inheritance at all.

yyforyongyu commented 3 days ago

why dees Resolve need to split up in order to be incorporated into the new blockbeat architecture?

So we split it into two steps, step one is Launch, which offers the inputs to the sweeper once triggered by block X, and before that block X is sent to the sweeper. This way we can ensure if block X closes a channel, its contract resolutions will offer these inputs to the sweeper, before the sweeper thinks there is nothing to be swept in block X. Step two is then then waiting for the sweeping tx to be confirmed.

Is that that since we don't know which ones will be launched ahead of time, we can't set up the blockbeat consumer DAG upfront?

Yeah this part too - so these sub-consumers, like chain watcher and channel arbitrator, are ephemeral and are not managed by the blockbeat dispatcher. Some previous discussion can be found here. Basically it's easier to let the chainArb to manage the channelArb and chainArb here.