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

routing: delay pruning closed channels from the channel graph by 12 blocks #7865

Open
Roasbeef opened this issue Aug 1, 2023 · 4 comments
Open
Labels
advanced Issues suitable for very experienced developers brainstorming Long term ideas/discussion/requests for feedback enhancement Improvements to existing features / behaviour P2 should be fixed if one has time path finding protocol splicing

Comments

@Roasbeef
Copy link
Member

Roasbeef commented Aug 1, 2023

Spec Change & Potential Implications

A bit over a year ago a changed was merged into the spec to advise nodes to wait 12 blocks before removing spent funding outputs. This has implications for splciign, as if nodes just wait before removing a channel, then from the PoV of path finders, it's as if the channel never closed. In this case, the only thing lost then is channel age, which some (?) algos use as a proxy for reliability. We should catch up to observe this while also thinking about path finding and re-org implications.

On the path finding side, if we see a channel X with 1 BTC, which is then "closed", and replaced with a channel with 1.1 BTC, for the 12 block interval, would we then see a 2.1 BTC link between the two parties, or a 1.1 BTC link? All our path finding algos now use the capacity to factor into the apriori probability, so done in an iterative manner, we'd think the channel is getting large and larger, and try to send payments through that would likely fail. Does this matter much in practice?

Re re-orgs, it's possible that a channel is closed, a few blocks elapse, then the close itself is re-org'd out. In this case, we haven't yet removed the channel yet as it's still valid, but would the nodes continue to observe the channel as if it was never closed?

Architecture Overview

The FilteredChainView is used today to handle removing spent channels for a given block. Upon start up we'll scan forward in the chain to find all closed channels since we were down. This logic is also re-org aware, as it's able to detect we were on a stale chain to ensure we consume the proper blocks. Once caught up, for each new block, we'll prune the closed channels from the channel graph.

Suggested Implementation Path

Given the above, it may make the most sense to actually implement this logic on the filtered block level. So rather than notify for the chain tip, it first gives a notification for 12 blocks back, then has a lagging pointer from there on.

The catch up logic would then just (?) lag behind by 12 blocks, reducing the bounds of this loop:

lnd/routing/router.go

Lines 803 to 815 in 1871970

// If we're not yet caught up, then we'll walk forward in the chain
// pruning the channel graph with each new block that hasn't yet been
// consumed by the channel graph.
var spentOutputs []*wire.OutPoint
for nextHeight := pruneHeight + 1; nextHeight <= uint32(bestHeight); nextHeight++ {
// Break out of the rescan early if a shutdown has been
// requested, otherwise long rescans will block the daemon from
// shutting down promptly.
select {
case <-r.quit:
return ErrRouterShuttingDown
default:
}

This change would also affect most of our itests as well, since they always assert that the channels aren't present in the graph right after the close tx is mined. We can either add a build tag to ignore this for itests, or have every test mine 12 blocks at the end, then assert that all the channels are gone.

@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour brainstorming Long term ideas/discussion/requests for feedback advanced Issues suitable for very experienced developers path finding protocol splicing labels Aug 1, 2023
@saubyk
Copy link
Collaborator

saubyk commented Aug 2, 2023

On the path finding side, if we see a channel X with 1 BTC, which is then "closed", and replaced with a channel with 1.1 BTC, for the 12 block interval, would we then see a 2.1 BTC link between the two parties, or a 1.1 BTC link? All our path finding algos now use the capacity to factor into the apriori probability, so done in an iterative manner, we'd think the channel is getting large and larger, and try to send payments through that would likely fail. Does this matter much in practice?

Wouldn't this mean that nodes with spliced channels would experience a dip in their routing traffic for a while?

@Crypt-iQ
Copy link
Collaborator

Crypt-iQ commented Aug 2, 2023

Is 12 blocks enough in high fee environments?

@Roasbeef
Copy link
Member Author

Roasbeef commented Aug 3, 2023

Is 12 blocks enough in high fee environments?

I don't think the fee confirmation delay matters, what does matter is the amount of time that elapses between the splice transaction getting confirmed and a sender seeing the new channel announcement. While the splice transaction is unconfirmed, the sender still sees the channel as being open, once it's closed though, then as is they'll remove it from the graph.

The key assumption here is that "everyone the matters" will see the new channel announcement within 12 blocks of the splice transaction (seen as a close to the uninitiated) confirms.

@Roasbeef
Copy link
Member Author

Roasbeef commented Aug 3, 2023

Thinking about it a bit more, I think the concerns re path finding only apply for the 12 block interval that an observant client detects both channels as being usable. If the client recognizes that channel 2 actually spends channel 1, then they can update their review and use the capacity for channel 2.

@saubyk saubyk added the P2 should be fixed if one has time label Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
advanced Issues suitable for very experienced developers brainstorming Long term ideas/discussion/requests for feedback enhancement Improvements to existing features / behaviour P2 should be fixed if one has time path finding protocol splicing
Projects
None yet
Development

No branches or pull requests

3 participants