Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add router support for batched splices #2989

Merged
merged 5 commits into from
Feb 5, 2025
Merged

Conversation

remyers
Copy link
Contributor

@remyers remyers commented Jan 29, 2025

A splice tx could involve more than one parent channel. The Router must track the set of channels spent by a given tx until matching channel announcements are received that splice the parent channels or the spend tx is deeply buried.

If a splice tx spends more than one parent channel between the same nodes, then there's no way to know which new channel announcement corresponds to which parent channel. We simply update the first one found.

@remyers remyers requested a review from t-bast January 29, 2025 12:04
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was more complex than I expected, I hadn't initially thought about the fact that we cannot map a spliced channel to its parent (but it's fine we don't care about which parent we map it to).

@remyers remyers force-pushed the batch-splices branch 2 times, most recently from 19e5538 to 69db84b Compare February 3, 2025 11:12
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a rebase, almost there!

A splice tx could involve more than one parent channel. The Router must track the set of channels spent by a given tx until matching channel announcements are received or the spend tx is deeply buried.

If a splice tx spends more than one parent channel between the same nodes, then there's no way to know which new channel announcement corresponds to which parent channel. We simply update the first one found.
Also added a function in `BaseRouterSpec` to add a second b-c channel for the batch splice test in `Routerspec`. The alternative to adding a 2nd b-c channel in `BaseRouterSpec` for all tests requires too many other tests to need updating.
@remyers
Copy link
Contributor Author

remyers commented Feb 5, 2025

Needs a rebase, almost there!

Thanks for the review! Rebased on master.

I also added some new checks in the gossip integration test to check the state of the channels set in the Router after the splice. I found it included the original parent channel when it should have only had the splice_ab and bc channels. The fix was to wait for Alice to receive both initial local channel updates before doing the splice. Otherwise the 2nd update (from Bob?) after opening the original ab channel came after the first splice update and added the original channel as a new channel in the Router.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few test nits, otherwise LGTM.

Previous check would succeed even if one or both updates were None.
If assert fails, should call getRouterData again for the next awaitAssert loop.
@remyers remyers merged commit 5abf99e into ACINQ:master Feb 5, 2025
1 check passed
@remyers remyers deleted the batch-splices branch February 6, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants