Propagate new blocks after PoW check

[gumb0]

There's a consensus now that it's more efficient for the network as a whole to propagate the blocks as soon as the PoW check is complete. go-ethereum and Parity already do that, we still propagate only after the blocks are fully validated and imported into the chain.

Relevant go-ethereum codes
https://github.com/ethereum/go-ethereum/blob/edc39aaedf526bfb7757a445c1f9dd42f45dc8d4/eth/handler.go#L694-L732
https://github.com/ethereum/go-ethereum/blob/edc39aaedf526bfb7757a445c1f9dd42f45dc8d4/eth/fetcher/fetcher.go#L635-L682

Note that the hashes of the new blocks area announced (with NewBlockHashes message) by go-ethereum only after sucessfull full validation.

Relevant Parity PR openethereum/parity-ethereum#9954

[gumb0]

Also mentioned in the point 1 of https://ethereum-magicians.org/t/ethereum-1-dot-x-a-half-baked-roadmap-for-mainnet-improvements/1995

[halfalicious]

@gumb0 Do we have a function which just verifies the PoW in the block header? I see that we have BlockHeader::verify but that validates the entire block header (e.g. parent hash is valid, block number is valid, gas used < gas limit)

[gumb0]

@halfalicious BlockChain::verifyBlock(block, onBad, ImportRequirements::ValidSeal) should do that, I think.

(Other header checks might no hurt though, because they should be quick comparing to PoW check. So the important distinction is between all pre-execution validation and executing the block; blocks should be propagated before execution)

[halfalicious]

@gumb0 I'm going to start working on this next, here's my tentative plan...first, some context:

In Client, we import blocks into the local chain after we are notified that there are blocks available. The block queue verifier threads perform this notification by calling BlockQueue::m_onReady after successful block import (see BlockQueue::verifierBody). The client sets this function to Client::onBlockQueueReady in Client::init - onBlockQueueReady just sets m_syncBlockQueue to true and the Client's main work loop - Client::doWork - checks this member on each iteration and calls into Client::syncBlockQueue to kick off block import. Note that the actual removal of verified blocks from the block queue and importation into the local chain is done in BlockChain::sync. Additionally, EthereumCapability::maintainBlocks will propagate (up to 19) new blocks from the local chain (which means that they've already been imported i.e. gone through full verification).

My plan - I think that before we make the call to BlockChain::sync to remove the blocks from the block queue and import them into the chain (in Client::syncBlockQueue) we should first retrieve the verified blocks from the queue (BlockQueue::drain) and propagate them (via a new function I'll add to EthereumCapability - note that this function will post the actual work of propagating the blocks to the network thread since it will involve modifying data members which are only accessed on that thread e.g. EthereumPeer::m_knownBlocks). The client can then pass these blocks to a new BlockChain overload which will import them into the local chain.

I'll also update EthereumCapability::maintainBlocks so that it only propagates hashes (since we still want to wait until after blocks have been fully validated before propagating hashes).

Thoughts?

[gumb0]

Yeah sounds good, that's how I would imagine it, too.

The only complication I see is that draining the blocks from BQ involves first the call to BlockQueue::drain() and then BlockQueue::doneDrain() in the end of importing blocks to the chain.

We can't move doneDrain call to right after new drain call in Client, because I think BQ assumes that the blocks are being imported between drain and doneDrain - see it has some member like m_drainingSet, m_drainingDifficulty, those are supposed to refer to the blocks being currently imported. Ah, actually doneDrain expects to get the hashes of the blocks that failed to import, so we can't call it the import is finished.

Calling drain from Client and then doneDrain from BlockChain seems inelegant and error-prone.

I see two possible ways to solve this:

1. Either introduce "peek" kind of function to BlockQueue, which is similar to drain but doesn't really start the drain, just returns next blocks to be drained. Blocks to be drained are in the queue, so this should work correctly as long as we drain the same amount of blocks as we just peeked (but not more, there could be more pushed to the queue before peek and drain)
2. Get rid of this internal draining state in BlockQueue, because it's the reason why we need these two phases - drain and doneDrain. Instead of saving this state in m_drainingSet and m_drainingDifficulty we could keep these blocks in "ready" state (this looks safe to me), and make the caller take care of this state and pass it to doneDrain. Also we'd rename doneDrain and it will be something like

bool BlockQueue::onBlocksImported(h256s const& _good, h256s const& _bad, u256 const& _totalDrainedDifficulty);

The first way might be easier to implement, but the second one could make the code simpler maybe.

[halfalicious]

There's a consensus now that it's more efficient for the network as a whole to propagate the blocks as soon as the PoW check is complete. go-ethereum and Parity already do that, we still propagate only after the blocks are fully validated and imported into the chain.

Relevant go-ethereum codes
https://github.com/ethereum/go-ethereum/blob/edc39aaedf526bfb7757a445c1f9dd42f45dc8d4/eth/handler.go#L694-L732
https://github.com/ethereum/go-ethereum/blob/edc39aaedf526bfb7757a445c1f9dd42f45dc8d4/eth/fetcher/fetcher.go#L635-L682

Note that the hashes of the new blocks area announced (with NewBlockHashes message) by go-ethereum only after sucessfull full validation.

Relevant Parity PR paritytech/parity-ethereum#9954

Regarding Geth only propagating block hashes after full import - pinged the Geth folks about this, they do this because its expected for a client to immediately request blocks that they learn about via NewBlockHashes packet, so if they announce hashes of a block which fails import they can get into a state where clients are requesting blocks that they don't have.

Also, they track the head of each peer's chain to determine if they need to sync with them, and if peers announce headers for blocks that fail import then Geth gets into a strange state of nodes spreading untrue information.

[halfalicious]

Yeah sounds good, that's how I would imagine it, too.

The only complication I see is that draining the blocks from BQ involves first the call to BlockQueue::drain() and then BlockQueue::doneDrain() in the end of importing blocks to the chain.

We can't move doneDrain call to right after new drain call in Client, because I think BQ assumes that the blocks are being imported between drain and doneDrain - see it has some member like m_drainingSet, m_drainingDifficulty, those are supposed to refer to the blocks being currently imported. Ah, actually doneDrain expects to get the hashes of the blocks that failed to import, so we can't call it the import is finished.

Calling drain from Client and then doneDrain from BlockChain seems inelegant and error-prone.

@gumb0 Thanks for calling out doneDrain. I agree that we don't want to call drain/doneDrain in different classes, but how about creating a BlockChain::sync overload which both accepts a list of blocks to import and returns a list of bad blocks? Then Client can call drain, pass the blocks to EthereumCapability for propagation, then call into the BlockChainSync overload to import the blocks (and receive the list of bad blocks), then call doneDrain?

I see two possible ways to solve this:

1. Either introduce "peek" kind of function to `BlockQueue`, which is similar to `drain` but doesn't really start the drain, just returns next blocks to be drained. Blocks to be drained are in the queue, so this should work correctly as long as we drain the same amount of blocks as we just peeked (but not more, there could be more pushed to the queue before peek and drain)

Agreed, if we drain more than we peeked we won't propagate the blocks that we drained-but-didn't-peek.

2. Get rid of this internal draining state in `BlockQueue`, because it's the reason why we need these two phases - `drain` and `doneDrain`. Instead of saving this state in `m_drainingSet` and `m_drainingDifficulty` we could keep these blocks in "ready" state (this looks safe to me), and make the caller take care of this state and pass it to `doneDrain`. Also we'd rename `doneDrain` and it will be something like

bool BlockQueue::onBlocksImported(h256s const& _good, h256s const& _bad, u256 const& _totalDrainedDifficulty);

The first way might be easier to implement, but the second one could make the code simpler maybe.

I like the second approach since I agree that it should simplify the block queue code a little bit, plus it sounds similar to my proposed approach above...I think what I'll do is have the client call BlockQueue::drain to get the blocks, then propagate them via EthereumCapability, then pass them to an overload of BlockChain::sync which will return the bad blocks in addition to the imported blocks, then call the new BlockQueue::onBlocksImported function which will remove the imported blocks from the ready set and update m_knownBad and call updateBad_WITH_LOCK.

[gumb0]

I agree that we don't want to call drain/doneDrain in different classes, but how about creating a BlockChain::sync overload which both accepts a list of blocks to import and returns a list of bad blocks? Then Client can call drain, pass the blocks to EthereumCapability for propagation, then call into the BlockChainSync overload to import the blocks (and receive the list of bad blocks), then call doneDrain?

Yeah, this sounds good, too, and simpler than my second approach.

I would not rename doneDrain then, to keep emphasizing that drain and doneDrain must be used together, because they init/clear some internal state of BQ.