[2/4] - peer: add new abstract message router

[Roasbeef]

In this commit, we add a new abstract message router. Over time, the goal is that this message router replaces the logic we currently have in the readHandler (the giant switch for each message).

With this new abstraction, can reduce the responsibilities of the readHandler to just reading messages off the wire and handing them off to the msg router. The readHandler no longer needs to know where the messages should go, or how they should be dispatched.

This will be used in tandem with the new protofsm module in an upcoming PR implementing the new rbf-coop close.

NOTE: This is a remade version of this branch, see this comment: #8434 (comment)

[coderabbitai]

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
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • 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 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 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 for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[github-actions]

Pull reviewers stats

Stats of the last 30 days for lnd:

	User	Total reviews	Time to review	Total comments
	guggero 🥇	9 ▀▀	1d 6h 41m	19 ▀▀
	Roasbeef 🥈	6 ▀	12h 16m	6 ▀
	ProofOfKeags 🥉	6 ▀	2d 23h 9m ▀	53 ▀▀▀▀
	ellemouton	6 ▀	4d 8h 21m ▀	22 ▀▀
	yyforyongyu	5 ▀	19h 48m	3
	bhandras	4 ▀	4d 10h 42m ▀	5
	Crypt-iQ	2	6d 11h 18m ▀▀	5
	morehouse	2	7d 15h 43m ▀▀	1
	saubyk	2	8h 12m	0
	Chinwendu20	1	21h 28m	2
	ziggie1984	1	10d 15h 59m ▀▀▀	2

[ProofOfKeags]

Overall straightforward.

Biggest request is to move the sendQuery_ mechanics to fn so that we can make broad use of it (seems 100% generic).

I also find myself confused by the Either[error, error] construction as Either[A, A] == A for all A. I couldn't see any visible semantics difference here so was hoping you could shed some light on the choice.

Other than that there are some nits, and a more controversial nudge towards not having a closure determine routability but rather do explicit message enumeration. But I know we've disagreed on that before and I can yield if you have confidence that this is the better way.

[morehouse]

Replying to #8434 (comment) on the original PR.

@morehouse I think this PR needs to be viewed in context with the protofsm PR and the rbf-close PR above. For new sub-systems created using the protofsm pattern, the FSM executor already has all the methods needed to be used as a MsgEndpoint, so you actually don't need unique boiler plate for each new sub-system one wishes to route to. Instead, you implement the states, then make the FSM executor using those states, and you're already able to register the endpoint from that.

I'm not a fan of the protofsm change either. See #8337 (comment).

In terms of the existing messages, although there're about half a dozen messages handled in the main readHandler loop, they only go to around 3 unique sub-systems, so I think the amount of extra boiler plate would be minimal. Eventually this msg router logic can also replace the existing msg stream usage, so activeMsgStreams and the msgStream struct itself.

Good point -- we can simplify the routing logic in peer/brontide.go by pushing some of the logic into the target subsystems. Note we could also do this without introducing dynamic behavior.

msgStream or something similar could be refactored into a utility module used by each subsystem to queue its incoming messages in whatever way makes sense to that module.

You'll still be able to analyze all the registered endpoints in a single place, as all registration will happen when the peer struct is created (if you don't register at the start, if a message comes along it'll be unhandled), so there can still be a single place where all the endpoints are registered (loadActiveChannels or w/e that ends up being refactored into).

The current interface does not enforce a static set of endpoints. To properly do that, we should remove RegisterEndpoint altogether and require endpoints to be specified in the constructor.

But if we're keeping the static behavior anyway, and the routing logic will still be enumerated within peer/brontide.go, why should we introduce all this new boilerplate? We can achieve the same goal with less complexity by keeping the current switch statement (and optionally simplifying it a bit).

See above for the convo re how to restrict that if we wish. Given the general MsgRouter interface, one could create something like a UniqueMsgRouter that enforces a 1:1 mapping between messages and endpoints.

If we keep the switch statement, we easily and automatically enforce 1:1 mapping since each message will be routed to exactly one case in the switch.

This PR would also allow users to re-instantiate the peer struct outside of lnd in a new Go program, and be able to add handlers for new custom messages or other protocol features without modifying the lnd code.

What is the intended use case here? Non-LND usage is not mentioned anywhere in the PR or commits.

In your mind, how can we achieve something like that while also requiring all message handling to be statically declared in a single place? When you say statically declared, I take that to mean that:

1. All the messages are enumerated via a switch statement with the core type.

2. The peer then also needs to know _where_ the message should be sent to, which means include a ref to that as an attribute in the peer struct.

If this dynamic behavior is really needed, we could add a routeMsg callback to peer.Config that will override LND's default routing logic. This would allow the user to write their own routing switch statement.

I totally agree that at times tight coupling of components can mean less code, whereas loose coupling invariably means a layer of indirection which can add more code. Ultimately, I think the flexibility is worth it, as the peer struct has gotten so bloated over the past few years with the addition of new features.

Bloat can be removed while keeping the basic switch statement. The switch would enumerate all messages statically, and route them to one of the 3 subsystems. All secondary routing could be pushed into the subsystems themselves.

[Roasbeef]

To properly do that, we should remove RegisterEndpoint altogether and require endpoints to be specified in the constructor

We don't examine feature bits until the peer struct is actually active, the feature bits checks are effectively runtime checks. Based on feature bits, you may want to register some endpoints, but not others.

What is the intended use case here? Non-LND usage is not mentioned anywhere in the PR or commits.

Let's say I'm making some custom channel factory protocol. I don't necessarily want to put all the code into lnd as it isn't fully baked yet, but I want to re-use all the components to handle the wire message parsing, etc, etc. With this PR, I can make a MsgEndpoint in a new Go program, then initialize the MsgRouter as a local variable, pass it in to the main config, then register the endpoint. With this, I'm able to dispatch/handle any new custom messages (and potentially consume normal funding messages) w/o needing to use the custom message RPCs.

For an example where lnd is bundled amongst other protocol related sub-systems/daemons (in a new Go program outside of lnd), look no farther than Lightning Terminal: https://github.com/lightninglabs/lightning-terminal.

We can achieve the same goal with less complexity by keeping the current switch statement (and optionally simplifying it a bit).

With just the current switch statement, the above example isn't possible.

If this dynamic behavior is really needed, we could add a routeMsg callback to peer.Config that will override LND's default routing logic. This would allow the user to write their own routing switch statement.

I think if you follow this suggestion to its logical conclusion, you'd get something that looks very similar to this PR. With just a call back, you force a single sub-system to effectively handle all messages. The current approach lets the endpoints come and go if they wish. This is also already the case, but maybe less obvious, eg: we only make and route to the existing coop state machine once we initiate the closing process.

Stepping back, this is a ~450 line diff, if you look at the final PR where this is used, I really don't think the amount of boiler plat added is more than if we added an entirely new sub-system to implement a new custom protocol.

[ProofOfKeags]

The current approach lets the endpoints come and go if they wish

This is right. I do think that it will improve the flexibility of the LND architecture at the expense of being able to verify its correctness and how the components interact. Similar to the way that LISP macros massively improve expressivity at the cost of understanding (without good discipline). Are we prepared to make this tradeoff? My instinct is usually against this stuff but I have a pretty extreme bias towards verifiability and can't effectively weigh the benefit of being able to hook into LND from outside of it.

[Roasbeef]

FWIW, this pattern was used with great success to implement some of the tapd asset channel functionality. We were able to make lnd inside of another process, make a msg router to register for new custom messages, then pass that into lnd with each peer knowing to dispatch those special messages to the new sub system.

[lightninglabs-deploy]

@Crypt-iQ: review reminder
@ProofOfKeags: review reminder

[ProofOfKeags]

I've opened up a nice simplification PR #9010. Other than that my only comment is that I don't think we should leak the actual endpoint handlers via the public interface. Seems super sus.

[Roasbeef]

Using master a base for this PR now.

[Roasbeef]

Pulled in @ProofOfKeags's commits, and rebased on top of master. Also pulled in some from #8960

[ProofOfKeags]

So fresh. So clean.

I think we can (and should) do away with the globalMsgRouter field and just replace it with a query to the config. You can also call stop on the condition that p.cfg.MsgRouter != p.msgRouter

[ProofOfKeags]

I really wish that go would let you just do p.msgRouter.WhenSome(Router.Start) but alas...

[ProofOfKeags]

😘👌🏼

[ProofOfKeags]

this field is totally unnecessary. I think we just inline the call to cfg.MsgRouter.IsSome()

[Roasbeef]

Ah yeah can do that too: we can check against the config version as it has a life cycle independent of the peer itself.