[WIP] Perform validation in a separate thread to message handling.

rebroad commented at 9:52 am on January 20, 2017: contributor

Have been testing this for a few weeks now - in my experience it helps to avoid moments during IBD where the best chain activation causes download to stop (due to the window being emptied, as no further blocks are being requested while the message handler thread is on hold while waiting for the chain to finish activating) - this becomes especially important with other changes I am testing where the MAX_BLOCKS_IN_FLIGHT_PER_PEER is significantly reduced (based upon bandwidth of each host).

I might have omitted some locks that need to be used here - and also might be able to incorporate this code into existing files rather than needing new ones - but it is certainly ready enough for a proof of concept I think.

Also possible improvements might be to use signals to break out of the sleep (as was done in 351593b9c8687b4da2e86769899b8b61ea44b630), although during IBD I don’t think the saving of 100ms is critical.

Perform validation in a separate thread to message handling. ad63e8bc7b

fanquake added the label Validation on Jan 20, 2017

TheBlueMatt commented at 3:52 pm on January 20, 2017: member

Because our p2p protocol defines most messages as completely blocking (ie if you send a block message the remote peer must have processed it before processing any other messages from you), I dont think this is the right approach. I think we’re gonna need something more along the lines of multiple ProcessMessages threads (as well as all the lock contention fixes that are going into 0.14).

joseroubert08 approved

rebroad commented at 1:56 am on January 21, 2017: contributor

@TheBlueMatt you are of course correct, given that the protocol is defined by the code, but was this designed intentionally? Is there any advantage in this blocking?

Upon further thought, I don’t think this is a concern, as this PR only affects during IBD and not when the chain is synced, so the behaviour is the same, but just means that ping times are not affected (as they are currently) and the block download no longer freezes during IBD.

But if during IBD you did want to re-inforce behaviour, you could simply postpone responding to getblocks and getheader requests while fActivateChain is true - what would be the benefit in doing this though?

TheBlueMatt commented at 4:30 am on January 21, 2017: member

Indeed, this fact developed by accident simply because that’s how the implementation worked. However, as time went on, other implementations (and even Bitcoin Core, though to a lesser extent) started relying on this part of the protocol, so we can’t do much to change it now.

Even still, it isn’t such a bad design, in the end, as it both simplifies some stuff and bounds how much work a node can make us do - one thread’s worth.

On 01/21/17 01:56, R E Broadley wrote:

@TheBlueMatt https://github.com/TheBlueMatt you are of course correct, given that the protocol is defined by the code, but was this designed intentionally? Is there any advantage in this blocking?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9599#issuecomment-274224234, or mute the thread https://github.com/notifications/unsubscribe-auth/AAnoHo9vXWweenIdUihdKoEJnZxflcNkks5rUWXIgaJpZM4LpHv-.

rebroad commented at 6:43 am on January 21, 2017: contributor

@TheBlueMatt With this PR it’s still only 1 thread that does the work. I’m not sure how the blocking simplifies stuff - it makes ping times fairly unreliable, and it also causes nodes to timeout and become unresponsive, thereby delaying IBD of all nodes, not just the blocking node. It makes it harder to detect misbehaving nodes also by the fact that the base-line behavior is more unreliable. I think overall, making validation have its own thread will in time make the whole network more reliable and make the network smoother (i.e. less latency fluctuations and timeouts).

I’m not yet understanding any reasons for keeping the blocking behavior - which implementations rely on it please?

TheBlueMatt commented at 3:25 pm on January 21, 2017: member

Having latency go up when a node is overloaded provides useful information to its peers - anything where it’s peers care about the latency to a node they probably care about whether it can respond quickly to messages.

At least Bitcoinj relies on it in a few places to, eg, send a getdata followed by ping to know whether the getdata will be responded to.

On January 21, 2017 1:43:46 AM EST, R E Broadley notifications@github.com wrote:

@TheBlueMatt With this PR it’s still only 1 thread that does the work. I’m not sure how the blocking simplifies stuff - it makes ping times fairly unreliable, and it also causes nodes to timeout and become unresponsive, thereby delaying IBD of all nodes, not just the blocking node. It makes it harder to detect misbehaving nodes also by the fact that the base-line behavior is more unreliable. I think overall, making validation have its own thread will in time make the whole network more reliable and make the network smoother (i.e. less latency fluctuations and timeouts).

I’m not yet understanding any reasons for keeping the blocking behavior

which implementations rely on it please?

– You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9599#issuecomment-274241951

rebroad commented at 1:19 am on January 23, 2017: contributor

@TheBlueMatt To try to use an analogy to your argument. Which would you rather have? The ebola virus AND useful information about it - or neither? The useful information about poor latency might be useful, but if we can remove the latency - are we really going to miss it because the useful information about it has gone also? Anyway, we’ll still have the historical information about it - but I don’t see how the argument of “being able to observe it” is sufficient reason to keep it. I must be missing something - can you elaborate perhaps?

TheBlueMatt commented at 1:40 pm on January 23, 2017: member

Even if we were to decide it was useless, we couldn’t remove it. As noted, other clients rely on our serialization behavior pretty heavily (and I’m sure we do, to some extent, though I don’t recall any specific places where right now).

On January 22, 2017 8:19:19 PM EST, R E Broadley notifications@github.com wrote:

@TheBlueMatt To try to use an analogy to your argument. Which would you rather have? The ebola virus AND useful information about it - or neither? The useful information about poor latency might be useful, but if we can remove the latency - are we really going to miss it because the useful information about it has gone also?

– You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9599#issuecomment-274376530

gmaxwell commented at 2:48 am on January 29, 2017: contributor

It is a bad design to handle protocol messages out of order for a single peer.

rebroad commented at 1:43 pm on January 29, 2017: contributor

@gmaxwell I agree, but, I think the only place that this could happen with this PR is if a node serves a block and then immediately sends either a getblocks or a getheaders. I’m not aware any nodes are currently doing this, and if they are, I could modify this PR to delay responding to the getheaders or getblocks until the chain has been updated quite easily. If this functionality is so important, perhaps a test ought to be added to test for this?

sipa commented at 4:01 pm on April 9, 2017: member

This seems to break the P2P protocol, closing.

sipa closed this on Apr 9, 2017

MarcoFalke locked this on Sep 8, 2021

[WIP] Perform validation in a separate thread to message handling. #9599