Delay considering a channel closed when seeing an on-chain spend #2936

remyers · 2024-11-04T10:58:05Z

See issue #2437

When an external channel is spent, the router will add it to a new spentChannels map instead of immediately removing it from the graph.

If after 12 blocks an entry in the spentChannels map is not part of a splice, it will be removed as before.

When a newly added channel is validated, if it spends the shared output of a recently spent channel then it is a splice.

A splice will update the shortChannelId and capacity of graph edges of the parent channel instead of removing the parent's edges and adding edges for a new channel. A splice will also reuse and adjust the parent edge's low/high balance bounds information based on the splice.

remyers · 2024-11-04T11:07:11Z

@thomash-acinq how do you think a splice should change the low/high bounds estimate for a graph edge? This PR currenty increases the 'high' for the edge by the amount a spliced channel's capacity increases and decrease the 'low' for the edge by the amount it's capacity decreases. Is that correct? I tried to copy the logic used currently when an channel is added or removed, but only for the change in capacity instead of the entire channel's capacity.

thomash-acinq · 2024-11-04T13:45:54Z

@thomash-acinq how do you think a splice should change the low/high bounds estimate for a graph edge? This PR currenty increases the 'high' for the edge by the amount a spliced channel's capacity increases and decrease the 'low' for the edge by the amount it's capacity decreases. Is that correct? I tried to copy the logic used currently when an channel is added or removed, but only for the change in capacity instead of the entire channel's capacity.

That looks good.

eclair-core/src/main/scala/fr/acinq/eclair/router/Graph.scala

eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala

remyers · 2024-11-28T10:51:10Z

I refactored spentChannels in d7b9bea.

Now spentChannels will map from the txid of the tx that spends a funding tx, to the scid of the funding tx. If there are RBF attempts, then there will be multiple entries of (spending tx -> scid) .

Each spending tx will be watched until it has 12 confirmations (2 x the depth of a validated node announcement). I do not think we need a new config parameter for this. If we do add a new parameter, then it should never be less than 12 blocks to ensure that any channel announcement for a splice has time to confirm.

If a spending tx confirms after 12 blocks without a channel announcement being validated for the spending tx, then the normal flow for removing a channel occurs.

If a channel announcement is validated before the spending tx has 12 confirmations, then a new flow for updating the graph for a splice occurs.

When a spending tx confirms to remove or splice a channel, then all spentChannels entries that map to the scid of the original funding tx will be removed.

t-bast

I haven't fully reviewed the tests yet, but the implementation looks good to me, my comments are mostly nit! Can you rebase after addressing my comments, and then we'll work on finalizing the tests and merging this?

eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala

eclair-core/src/main/scala/fr/acinq/eclair/router/Validation.scala

eclair-core/src/main/scala/fr/acinq/eclair/router/Graph.scala

eclair-core/src/main/scala/fr/acinq/eclair/router/BalanceEstimate.scala

Issue ACINQ#2437 When an external channel is spent, add it to the `spentChannels` list instead of immediately removing it from the graph. RBF attempts can produce multiple spending txs in the mempool for the same channel. The `spendChannels` list maps the txid of the spending tx to the scid of the spent channel. When a channel announcement is validated with a funding tx on the `spentChannels` list, consider the new channel a splice of the corresponding spent channel. A splice updates the graph edges to preserve balance estimate information in the graph. If a spending tx from the `spentChannels` list is deeply buried before appearing in a valid channel announcement, remove the corresponding spent channel edge from the graph. Also remove any corresponding spending tx entries on the `spentChannels` list.

Also fixed two other issues: - add the splice channel to the routing graph as a new channel if the parent channel does not exist (could be pruned) - when a splice confirms, also send ChannelLost to remove the parent scid from the FrontRouter

t-bast

This is looking mostly good, I need to spend a bit more time on tests. I've added a commit here with fixes for my comments: e85512b

I haven't fully fixed the tests though to expect the new commands sent to the watcher, I'll let you fix that!

eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala

eclair-core/src/main/scala/fr/acinq/eclair/router/Validation.scala

eclair-core/src/test/scala/fr/acinq/eclair/integration/ChannelIntegrationSpec.scala

We unwatch: - channels that have been spliced or closed - RBF candidates of a splice or closing transaction We revert the modification to channel updates to avoid invalidating the signature.

- Fixup so Router does not send `UnwatchTxConfirmed` only for the spending txs that will never confirm. Channel also has a `WatchTxConfirmed` event that may trigger later and should not be removed. - Fixup channel integration tests to generate enough blocks to deeply confirm a channel has closed once before waiting for the channel to close. - Fixup router tests to check that funding tx spend is unwatched after it's spending tx confirms

eclair-core/src/main/scala/fr/acinq/eclair/router/Validation.scala

eclair-core/src/test/scala/fr/acinq/eclair/integration/ChannelIntegrationSpec.scala

eclair-core/src/test/scala/fr/acinq/eclair/router/RouterSpec.scala

- fix the three places I missed for generating blocks to create deeply confirmed closing txs - improved how the router looks for UnwatchedExternalSpent message to lookup the txid from the scid - changed the comment in Validation be more clear

t-bast

LGTM, let's see if the e2e integration tests find any issues, but otherwise this should be ready to merge.

- I am not sure yet why the Router does not update the scid/capacity after the splice.

remyers · 2024-12-11T22:53:56Z

I pushed a preliminary integration test in 87c4346

The scids do not update in the channels (we probably need PR #2941), and the Router does not seem to be getting the balance capacity update. Any tips?

- when the Router receives a AvailableBalanceChanged after a splice it now updates the capacity of the channel to the current capacity of the latest commitment.

eclair-core/src/main/scala/fr/acinq/eclair/router/Validation.scala

And fix test.

- This test demonstrates that local channels update their capacity, but we can not test the remote node (carol) because the ChannelAnnouncement is ignored because it has a duplicate scid. After PR ACINQ#2941 we can fix this test.

commit.md

eclair-core/src/test/scala/fr/acinq/eclair/integration/basic/fixtures/MinimalNodeFixture.scala

- remove commit.md

- the spent output should match the whole Outpoint of the funding tx

t-bast

LGTM, let's go 🚀

remyers force-pushed the 2410-delay-close branch from 5fbb4a9 to d1109fa Compare November 5, 2024 14:59

remyers marked this pull request as ready for review November 6, 2024 09:33

t-bast reviewed Nov 20, 2024

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/router/Graph.scala Outdated Show resolved Hide resolved

eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala Outdated Show resolved Hide resolved

eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala Outdated Show resolved Hide resolved

t-bast added the Optech Make Me Famous! label Nov 25, 2024

remyers force-pushed the 2410-delay-close branch from 929f85d to 2637de3 Compare November 28, 2024 09:24

remyers commented Nov 28, 2024

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala Show resolved Hide resolved

remyers force-pushed the 2410-delay-close branch from 2637de3 to 4e713a3 Compare November 28, 2024 10:35

remyers mentioned this pull request Dec 2, 2024

Splicing out funds should update router balances #2912

Open

t-bast reviewed Dec 2, 2024

View reviewed changes

remyers added 3 commits December 9, 2024 15:04

Simplify graph vertex updates and add tests

31c57f4

Fix nits/issues found by @t-bast

43a486e

Also fixed two other issues: - add the splice channel to the routing graph as a new channel if the parent channel does not exist (could be pruned) - when a splice confirms, also send ChannelLost to remove the parent scid from the FrontRouter

remyers force-pushed the 2410-delay-close branch from 4e713a3 to 43a486e Compare December 9, 2024 14:38

Fixed failing integration tests

e79c65b

remyers force-pushed the 2410-delay-close branch from 53215cf to e79c65b Compare December 9, 2024 18:54

t-bast reviewed Dec 10, 2024

View reviewed changes

t-bast and others added 2 commits December 10, 2024 20:08

Unwatch obsolete transactions

0f250b1

We unwatch: - channels that have been spliced or closed - RBF candidates of a splice or closing transaction We revert the modification to channel updates to avoid invalidating the signature.

t-bast reviewed Dec 11, 2024

View reviewed changes

remyers force-pushed the 2410-delay-close branch from 03c5629 to 007adc5 Compare December 11, 2024 14:08

Fixed tests and a comment

fa06b15

- fix the three places I missed for generating blocks to create deeply confirmed closing txs - improved how the router looks for UnwatchedExternalSpent message to lookup the txid from the scid - changed the comment in Validation be more clear

t-bast reviewed Dec 11, 2024

View reviewed changes

Add preliminary gossip integration test

87c4346

- I am not sure yet why the Router does not update the scid/capacity after the splice.

Fix to update the capacity of a public channel in the Router

7554711

- when the Router receives a AvailableBalanceChanged after a splice it now updates the capacity of the channel to the current capacity of the latest commitment.

t-bast reviewed Dec 12, 2024

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/router/Validation.scala Outdated Show resolved Hide resolved

Set channel capacity in balance update method

1c6424f

And fix test.

Fix to basic splice gossip test

df17db2

- This test demonstrates that local channels update their capacity, but we can not test the remote node (carol) because the ChannelAnnouncement is ignored because it has a duplicate scid. After PR ACINQ#2941 we can fix this test.

t-bast reviewed Dec 13, 2024

View reviewed changes

commit.md Outdated Show resolved Hide resolved

eclair-core/src/test/scala/fr/acinq/eclair/integration/basic/fixtures/MinimalNodeFixture.scala Outdated Show resolved Hide resolved

remyers added 2 commits December 13, 2024 14:58

fixup! Set channel capacity in balance update method

abe3779

- remove commit.md

Fix watcherAutoPilot WatchExternalChannelSpent

98b419d

- the spent output should match the whole Outpoint of the funding tx

t-bast approved these changes Dec 13, 2024

View reviewed changes

remyers merged commit c390560 into ACINQ:master Dec 13, 2024
1 check passed

remyers mentioned this pull request Dec 13, 2024

Delay considering a channel closed when seeing an on-chain spend #2437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay considering a channel closed when seeing an on-chain spend #2936

Delay considering a channel closed when seeing an on-chain spend #2936

remyers commented Nov 4, 2024 •

edited

Loading

remyers commented Nov 4, 2024 •

edited

Loading

thomash-acinq commented Nov 4, 2024

remyers commented Nov 28, 2024 •

edited

Loading

t-bast left a comment

t-bast left a comment •

edited

Loading

t-bast left a comment

remyers commented Dec 11, 2024

t-bast left a comment

Delay considering a channel closed when seeing an on-chain spend #2936

Delay considering a channel closed when seeing an on-chain spend #2936

Conversation

remyers commented Nov 4, 2024 • edited Loading

remyers commented Nov 4, 2024 • edited Loading

thomash-acinq commented Nov 4, 2024

remyers commented Nov 28, 2024 • edited Loading

t-bast left a comment

Choose a reason for hiding this comment

t-bast left a comment • edited Loading

Choose a reason for hiding this comment

t-bast left a comment

Choose a reason for hiding this comment

remyers commented Dec 11, 2024

t-bast left a comment

Choose a reason for hiding this comment

remyers commented Nov 4, 2024 •

edited

Loading

remyers commented Nov 4, 2024 •

edited

Loading

remyers commented Nov 28, 2024 •

edited

Loading

t-bast left a comment •

edited

Loading