Longest chain timeout

[dangershony]

When a peer that connected with a longer chain got disconnected before our node downloaded the blocks we need to disconnect and reset the longest chain to one of the connected peers

[dangershony]

[Consensus] Longest chain timeout

[Aprogiena]

This issue applies to both - Bitcoin and Stratis,

but in case of Stratis, there seems to be an attack scenario for POS where one is able to assemble a chain of block headers (but not blocks!) and present them as a longer chain, the node will probably try to get that chain, which will result in either the peer providing invalid block (he does not have those blocks), or he will refuse to provide them (won't reply). What we need to do here is that we make sure such node is disconnected (immediately [and banned] if provided invalid block; or after some timeout if it does not respond) and then we need to resolve as @dangershony mentioned above - we need to reset our longest chain to the best chain of currently connected peers (and make sure to handle a situation where we don't have any connected peers when that happens, so switch to the best chain of any new peer that connects later).

[bokobza]

[Chain] Longest chain timeout

[Aprogiena]

So what was the approach we wanted to take here? Have we agreed on this going to ChainHeadersBehavior at the end or not?

[Aprogiena]

I've forgotten the resolution, but I can remember now that we've mentioned that we need to access all nodes, which is not easy to do in CHB. I think I've suggested putting list of nodes into ChainState. But don't know if we made further progress on the discussion and resolved it somehow.

So what is the design here?

[dangershony]

We need to first identify if this is a consensus problem or node problem, and also how to approach the fix:

1. on disconnect, default to the next longest chain form one of the peers.
2. when asking for a block and the block does not arrive or its invalid.
   There is much more complexity with option 2.

This is probably a node problem (and not only consensus) while its easier to determine false headers when we have a consensus mechanism in place, this issue can also effect nodes that are not running the consensus verification code. (a light wallet that was given a fake longer chain will not swap back to the correct chain until its longer, or until the blocks are asked).

In my view the approach to fix this would be on a disconnect of a peer the following should happen.

• check if the disconnected peer is beyond checkpoint/assume-valid.
• check if the peer is the same as chain.Tip
• if yes compare tip to all other connected peers to check if it was the only that was longer
• if yes and consensus tip (chainState.ConsensusTip ) is behind role back chain.Tip to chainState.ConsensusTip or the longest peer.

[mikedennis]

Thanks guys. This is all new code for me so apologize in the advance for dumb questions 😀
After a quick initial look I see one of two potential approaches:

1. Do disconnect logic in ChainHeaderBehavior. As Apro mentioned it looks like CHB is lacking access to the other peers so we could add access to peers via ChainState and then do the disconnect logic here: https://github.com/stratisproject/StratisBitcoinFullNode/blob/master/src/Stratis.Bitcoin/Base/ChainHeadersBehavior.cs#L309-#L316

2. Or perhaps we could do the disconnect logic in Connection Manager: https://github.com/stratisproject/StratisBitcoinFullNode/blob/master/src/Stratis.Bitcoin/Connection/ConnectionManager.cs#L311-#L318
   We could add an ShouldRollback property to CHB that does has the logic of the last condition of your list and access it via
   ChainHeadersBehavior chainHeadersBehavior = node.Behavior<ChainHeadersBehavior>();

I'm leaning towards 2. Seems more like a connection manager issue than a behavior issue. What do you guys think?

[Aprogiena]

@dangershony explain

check if the disconnected peer is beyond checkpoint/assume-valid.

afaik CP/AV is on higher level, not on headers level, so what do you want to achieve with this check?

[Aprogiena]

@mikedennis

1. DetachCore might be better, or not?
2. not sure, i'm not familiar with CM

[dangershony]

afaik CP/AV is on higher level, not on headers level

Why not? assume valid has a block id thats all we need to identify a header is not part of the main chain.

[Aprogiena]

if you have AV or CP at height X with hash h(X)
and you are now working with peer that disconnected and its chain was longest at its height was X-1000
then you don't know if that AV/CP block with hash h(X) is on the chain if you only have headers up to h(X-1000)

so what i'm saying is that i can't see what you want to check or how does that check helps in that scenario

what could help you is that if the peer has tip at height X+100, and the next best peer has X-100, then of course if you know you want chain with X, you don't need to go back to X-100, just X, but that's different scenario and maybe not even worth considering

[noescape00]

I think we can do it like this (mostly based on @dangershony's proposal):

In ConnectionManager on the peer removal we can check if we have any peers that are as advanced as the removed one. If we do- we're fine and finish here because LookaheadBlockPuller will reassign the download task to some other peer.
Otherwise find the most advanced peer and check if it's chain is ahead of our block store tip. If it does- switch to his chain. Otherwise just rewind to the consensus tip.

And on top of that change the BlockPuller.OnStalling().
Now if the peer doesn't provide blocks that were assigned to it in a reasonable time we release all the download tasks and reassigning them to other peers basing on the quality score (it's actually random, but weighted with the quality score). The problem is if there is just 1 peer that has the chain long enough the download task will be assigned to it no matter the quality score.
I suggest that before releasing the download jobs we check if there is any peer with chain long enough to reassign the download to AND not minimal quality score. If not- wait some more just in case it's a really slow peer and disconnect from it if it still can't provide anything. If there is- reassign the job to with non-minimal quality score- just release the download tasks.
This will ensure having a timeout on a chain if the peer (or peers) that announced the best chain doesn't provide any blocks.

Let me know what you think about it. If all are fine with the suggested solution I'll proceed to implementing this.

[Aprogiena]

First of all, I think block puller behaviour should inform the block puller to remove the assignments for its peer when it's disconnected.

This is important because otherwise we would wait for not connected peer to deliver something. I can't remember if we have any mechanism in place for this situation, but I wouldn't expect that we have.

Note that we will refactor connection manager and therefore it is hard to design anything with connection management at this point, we should first do the refactoring and then come back to this issue.

There is issue in the GitHub already for that problem with one peer, it should not be mixed with this issue.

I think we need to discuss this more but not before the big refactoring because it doesn't make sense until that. There's also another issue that I will be talking about tomorrow that should be discussed before we start implementing anything here.

So regular notice – please do not start with implementation

[noescape00]

First of all, I think block puller behaviour should inform the block puller to remove the assignments for its peer when it's disconnected.
This is important because otherwise we would wait for not connected peer to deliver something. I can't remember if we have any mechanism in place for this situation, but I wouldn't expect that we have.

Yeah, we're already doing that- BlockPullerBehavior calls ReleaseAllPeerDownloadTaskAssignments on detach core.

Note that we will refactor connection manager and therefore it is hard to design anything with connection management at this point, we should first do the refactoring and then come back to this issue.

Ok, reasonable. Let's first do the #1090

[Aprogiena]

Oh, do we? Silly me then, my expectation was worse than reality

[dangershony]

Ok bring this back to live I think we definitely need to implement such a behaviour, this will also help a bit with the headerx problem (but not solve it obviously)

Consider this behaviour to only live in the Consensus feature
When a peer gets disconnected we check

• Peer.Tip if it within the same chain as our Headers.Tip
• No other peer is on that same chain
• Consensus.Tip is behind (here we could potentially check if the fork is inside Consensus.Tip (the trusted base like you call it) then we could assume we are ok)

If all above conditions are met jump to the next best Header.Tip

The puller needs to potentially also get involved perhaps if we get a timeout form a peer on receiving a block we disconnect and ban it (or we keep a counter and ban it after x failed timeouts) then on disconnect the behaviour will get triggered and if the node has provided us with bad headers we will swap to a better node.

If we only have once connection then we ofcourse do nothing.

[Aprogiena]

On Skype we have mentioned that we would like to separate switching of the best chain from disconnected peer and some kind of timeout. So the very 1st PR here should only deal with the switch of the chain when a peer disconnects.

[Aprogiena]

If we only have once connection then we ofcourse do nothing.

I would say that if we have only one connection and it gets disconnected, then we switch headers tip to consensus tip

[Aprogiena]

The timeout part should probably be solved after header X together with miner block data withdrawal Issue

[dangershony]

then we switch headers tip to consensus tip

Yeah sounds correct to me

Yes as we discussed this can be two separate PRs 1. switch on disconnect 2. disconnect on timeout.

[Aprogiena]

We are now working on this issue, basic idea is clear and we all agree on that, but the concrete implementation may differ little bit, but hopefully that is okay.

[noescape00]

already fixed on master