Zcash: Decrease MAX_HEADERS_RESULTS

[str4d]

The Equihash parameters were changed in the z8 release (#1182), which increased the solution size in the block header from 129 bytes to 2049 bytes, and thus the size of the block header from 269 bytes to 2189 bytes. This had an unintentional side-effect: once the block chain height passed 958, any newcomers to the network trying to download the whole blockchain (or anyone whose local blockchain was more than 958 blocks behind) would be sent messages that were too large, because of the following defined constants:

• MAX_HEADERS_RESULTS = 2000
• MAX_PROTOCOL_MESSAGE_LENGTH = 2 * 1024 * 1024

This inability to download blocks was misinterpreted as a DoS attack (#1256).

The solution is to reduce MAX_HEADERS_RESULTS. The problem will be somewhat mitigated by #1260 (which reduces the solution size to 1344 bytes), so we don't have to reduce it as far as the current testnet would require. In order to stay within MAX_PROTOCOL_MESSAGE_LENGTH, we should not have MAX_HEADERS_RESULTS > 1400.

Note that reducing MAX_HEADERS_RESULTS does not require a hard fork (although #1260 does, so we will be resetting the testnet anyway). But _increasing_ MAX_HEADERS_RESULTS does require a hard fork, because peers are considered to be misbehaving if they send more than MAX_HEADERS_RESULTS headers in response to a getheaders request. We should take this into account when choosing MAX_HEADERS_RESULTS.

Edit: Kudos for finding the bug goes to @ebfull (https://github.com/zcash/zcash/issues/1289#issuecomment-241809682)

[nathan-at-least]

Nice work on tracking this down! How did you determine this was the cause?

[ebfull]

(It was me.) I ran a local node with -debug and ran the alpha testnet node with -debug and saw the overflow errors. I guessed that it might be that the block headers are larger in z8 due to the Equihash parameter change, @str4d computed it and it matched up against the overflow size.

[daira]

Maybe at the point where we declare the parameters for each network in chainparams.cpp (e.g. https://github.com/zcash/zcash/blob/zc.v0.11.2.latest/src/chainparams.cpp#L62 for mainnet), we can have a static assert (using a constant-expression macro to compute the header length from the parameters) that they're consistent with MAX_HEADERS_RESULTS and MAX_PROTOCOL_MESSAGE_LENGTH.

[defuse]

Wow! So "1337" was a coincidence after all? This is a really cool bug, a good example of changes we might be not knowing to make (#826).