[DRAFT] Quit application: Stop P2P faster

[kiminuo]

Fixes #10155

There are 2 commits:

1. 80b8a71 - I think we should have this connect cancellation in place.
2. 0bf9653 corresponds with kiminuo/NBitcoin@46dfc4b commit (NBitcoin.Kimi fork 7.0.25.1)

Explanation

Now what is happening. We have P2pNetwork.cs where we initialize a NodesGroup:

WalletWasabi/WalletWasabi/BitcoinP2p/P2pNetwork.cs

Line 97 in 969b6bb

var nodes = new NodesGroup(Network, connectionParameters, requirements: Constants.NodeRequirements);

so on NBitcoin's side we start NodesGroup.StartConnecting(). So far so good.

The process of stopping P2pNetwork relies on NodesGroup.Disconnect() and there is a slight issue in the NBitcoin impl:

/// <summary>
/// Drop connection to all connected nodes
/// </summary>
public void Disconnect()
{
	_Disconnect.Cancel();
	_ConnectedNodes.DisconnectAll();
}

... and the issue is that NBitcoin does not cancel connecting of a not-yet-connected node (if there is one) in NodesGroup.StartConnecting. How is that possible? One would expect that:

1. _Disconnect.Cancel(); stops connecting to any not-yet-connected node, and
2. that _ConnectedNodes.DisconnectAll(); disconnects all connected nodes.

The issue seems to be in

• https://github.com/MetacoSA/NBitcoin/blob/fb13874fa43b881b55db9325b1e92a3d77e0bdbb/NBitcoin/Protocol/Node.cs#L838
• https://github.com/MetacoSA/NBitcoin/blob/fb13874fa43b881b55db9325b1e92a3d77e0bdbb/NBitcoin/Protocol/Node.cs#L851 (notice that this is the same method as @soosr observed in Hanging Wasabi process #10155 (comment))

and the issue is that _Connection (NodeConnection) has its own cancellation token that is not being cancelled by NodesGroup.Disconnect() (here).

And that's why kiminuo/NBitcoin@46dfc4b makes sense to me.

[kiminuo]

#10325 (comment) -> closing

[molnard]

If this fixes #10155 it should not be closed because of #10325.

I would keep this open. Do you think it is worth fixing this in NBitcoin?

[kiminuo]

I would keep this open. Do you think it is worth fixing this in NBitcoin?

Yeah, it would be good to fix it there¹. If my assessment is correct, NBitcoin's users would benefit from it as well.

Footnotes

1. Given how NBitcoin's P2P code is written, I think it would be for the best not to use it at all and have our own code instead. ↩

[stale]

This has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.