feat: finish concurrent body downloader

[onbjerg]

• Retries timed out requests
• Adds tests
• Removes timeout duration config from the Downloader trait - I think this should be up to the client

To do:

• Request multiple block bodies per call to BodiesClient (see below) (see discussion below)

@mattsse In the bodies stage PR you mentioned that we should request multiple bodies per call to BodiesClient instead of one per call. I'm still not entirely sure why that would be best, so let's discuss.

For the body downloader to work, we must know what block body lines up with what block number since we want to emit them in order of block number, not in order of request completion. As I understood it in the bodies stage PR, there is no guarantee that the returned Vec from the network request has this ordering guarantee, but there is also not any way to identify what body in that Vec matches with what block. Can you clarify here? I.e., if I request a set of bodies Vec<H256> and I get a set of Vec<BlockBody>, how do I line up what H256 matches with what BlockBody?

[mattsse]

For the body downloader to work, we must know what block body lines up with what block number since we want to emit them in order of block number, not in order of request completion.

yes, block number and block hash we get from the downloaded header, right?

there is no guarantee that the returned Vec from the network request has this ordering guarantee

this is not quite clear to me either, at least judging by the spec: https://github.com/ethereum/devp2p/blob/master/caps/eth.md#blockbodies-0x06 @Rjected wdyt?
Because there's no hash or block info in the BlockBodies response. So they have to match @Rjected? I don't know how geth/erigon does this

[Rjected]

Yeah, the block bodies (if they exist) should be returned in the order they are requested, if the bodies are available.

here is how geth services block body requests

[mattsse]

I still suck at reading go, lots of magic variables in there -.-

but the core question is: how safe is this actually because this does not guarantee that there are no gaps

https://github.com/ethereum/go-ethereum/blob/add337e0f7bad02f3cf535c66cd31f252b0b5c99/eth/protocols/eth/handlers.go#L228

and since there's no way of matching a Body to a block this is not trivial, or am I missing something?

knowing how the GetBody request is created and the response is handled would be more useful

[rakita]

The only way to be sure that the body belongs to block (aka header) is to create transaction_root and check it. This is an expensive operation as in essence it hashes all transactions.

For Openethereum side, it would return bodies like this: https://github.com/openethereum/openethereum/blob/6c2d392d867b058ff867c4373e40850ca3f96969/crates/ethcore/sync/src/chain/supplier.rs#L334-L346

And it would process reponse here: https://github.com/openethereum/openethereum/blob/6c2d392d867b058ff867c4373e40850ca3f96969/crates/ethcore/sync/src/chain/handler.rs#L353-L376
https://github.com/openethereum/openethereum/blob/6c2d392d867b058ff867c4373e40850ca3f96969/crates/ethcore/sync/src/block_sync.rs#L439
and it does calculate transaction and uncle root here to verify if block body is okey or not: https://github.com/openethereum/openethereum/blob/6c2d392d867b058ff867c4373e40850ca3f96969/crates/ethcore/sync/src/blocks.rs#L442-L456

edit: geth seems to do the same as OE: https://github.com/ethereum/go-ethereum/blob/add337e0f7bad02f3cf535c66cd31f252b0b5c99/eth/protocols/eth/handlers.go#L380-L391

[mattsse]

that makes sense, sounds like something we have to do regardless.

So we can do:

request them in batches -> compute tx root -> map to header?

[Rjected]

yeah, I guess indexing headers by transactions root makes sense, so they can be linked to body responses

[onbjerg]

Unsure if this is necessary, currently I request one body per call and assume the body is correct for a corresponding hash. These bodies are then returned in the order of the block number attached to the header hash. Later on, I verify the bodies and if a body fails verification I only commit the bodies that did not fail verification (assuming they are also sequential).

In other words, I don't see the benefit of

1. Indexing headers by transaction root
2. Pre-emptively computing the transactions root to match bodies to a list of headers
3. Fetching the header from db for a given transaction root (on top of already fetching bodies by block number btw)
4. Then validating the rest of the body (ommers)
5. Then writing the body

To guarantee the order vs what I am doing now:

1. Use information we already have (header hash, block number pairs)
2. Fire one request per header (but still multiple requests at a time)
3. Emit the bodies in order of information from step 1
4. Validating the body
5. If valid write, if not unwind

Does it matter if I request one body per call to the p2p layer (but obviously multiple requests at a time) vs multiple bodies per request?

that makes sense, sounds like something we have to do regardless.

Per above it would cost more db reads and potentially a lot more computation I think?

Edit: It would also mean that the bodies downloader would need a full db tx since it would need to actively look up headers instead of just being fed header hashes for bodies we want

[mattsse]

I see,

Fire one request per header (but still multiple requests at a time)
Emit the bodies in order of information from step 1

at this point we just assume that the returned body is valid. So requesting 10 bodies via 10 requests is the equivalent of requesting 10 bodies from 1 peer? Because we assume they're correct at this point?

[onbjerg]

at this point we just assume that the returned body is valid. So requesting 10 bodies via 10 requests is the equivalent of requesting 10 bodies from 1 peer? Because we assume they're correct at this point?

I guess so, the difference being that the downloader can relate each request (because of the future returned) to a block hash, so we can verify the body later on. If we requested 10 bodies from 1 peer, and we have no guarantee of order, then we can't do that without going thru what you described.

[mattsse]

If we requested 10 bodies from 1 peer, and we have no guarantee of order

Right, additional assumption would be they're returned in order, which geth+erigon should do.

I guess we can start with 1per req and then do some testing.

going from 1 to many hashes per payload shouldn't be a big problem.

[onbjerg]

Alright, I created an issue so we can track there: #226

This should be ok for MVP

Edit: Test failure is unrelated and being discussed/resolved in #204

[rakita]

One thing here, if we go for one block body per request and we are probably moving that validation of transaction root in some other down the line stage.

Out of 100 blocks, we could have a malicious node that sends us the wrong body (or faulty node) do we revert all blocks or could we require just that one body from the body stage, how would that unwind look?

[codecov]

Codecov Report

Merging #220 (ea1512c) into main (4936d46) will increase coverage by 0.48%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
+ Coverage   66.90%   67.39%   +0.48%     
==========================================
  Files         217      217              
  Lines       18292    18386      +94     
==========================================
+ Hits        12239    12391     +152     
+ Misses       6053     5995      -58     

Impacted Files	Coverage Δ
crates/stages/src/stages/bodies.rs	96.05% <ø> (+0.66%)	⬆️
crates/net/bodies-downloaders/src/concurrent.rs	90.06% <92.59%> (+90.06%)	⬆️
crates/net/discv4/src/lib.rs	68.29% <0.00%> (+0.44%)	⬆️
crates/interfaces/src/p2p/bodies/error.rs	40.00% <0.00%> (+30.00%)	⬆️
crates/interfaces/src/p2p/bodies/client.rs	100.00% <0.00%> (+100.00%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[gakonst]

Out of 100 blocks, we could have a malicious node that sends us the wrong body (or faulty node) do we revert all blocks or could we require just that one body from the body stage, how would that unwind look?

If we're only requesting one block at a time this doesn't matter right? We still need to calculate the tx root as a validity check, but there should be no block reversion right?

As for the multi block case, could we have a OrderedVerifiedStream where headers are guaranteed to be returned in order and verified? And have that wrap the responses we get and have it handle everything?

[rakita]

Out of 100 blocks, we could have a malicious node that sends us the wrong body (or faulty node) do we revert all blocks or could we require just that one body from the body stage, how would that unwind look?

If we're only requesting one block at a time this doesn't matter right? We still need to calculate the tx root as a validity check, but there should be no block reversion right?

If we are calculating tx root as validity of request in body stage it would not matter. More on it here: #226 (comment)

[rkrasiuk]

lgtm!

[onbjerg]

We can follow up with the additional stuff based on convo in #226