Invalid headers sent by peers are not tracked

Rjected commented 1 year ago

Describe the bug

In the test Invalid Ancestor Chain Sync, Invalid Timestamp (and others), the test stalls because we always return SYNCING and fail:

INFO (Invalid Ancestor Chain Sync, Invalid Timestamp, Invalid P9'): Response from main client: {SYNCING <nil> <nil>}
>> (e3f56847) {"jsonrpc":"2.0","id":24,"method":"engine_forkchoiceUpdatedV1","params":[{"headBlockHash":"0xdb5bb9829af442b13e9251374f216a7ba4ed9f7908e800e037c40ecbe31f9664","safeBlockHash":"0xdb5bb9829af442b13e9251374f216a7ba4ed9f7908e800e037c40ecbe31f9664","finalizedBlockHash":"0x0000000000000000000000000000000000000000000000000000000000000000"},null]}
<< (e3f56847) {"jsonrpc":"2.0","result":{"payloadStatus":{"status":"SYNCING","latestValidHash":null,"validationError":null},"payloadId":null},"id":24}
INFO (Invalid Ancestor Chain Sync, Invalid Timestamp, Invalid P9'): Response from main client fcu: {SYNCING <nil> <nil>}
>> (e3f56847) {"jsonrpc":"2.0","id":25,"method":"engine_newPayloadV1","params":[{"parentHash":"0x2e1ce2d889533e9d4ec52b239a1680166641694fa828c3689d8470fa5e357007","feeRecipient":"0x0000000000000000000000000000000000000000","stateRoot":"0x63356759e1a29ad16cbe306cdf55eb107f69fda2bf025f5b21efd2773a643133","receiptsRoot":"0x5d721667f3f41c6e8a5e630b2c5f259855d2365513295c4fd0c5d05498ce6489","logsBloom":"0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000","prevRandao":"0x3dd2306d45eb63035a6d86dae76b8ec301b3e39e14da41727995748020776be3","blockNumber":"0xf","gasLimit":"0x30a4bf","gasUsed":"0xa860","timestamp":"0x1243","extraData":"0x01","baseFeePerGas":"0x853fd7b","blockHash":"0xdb5bb9829af442b13e9251374f216a7ba4ed9f7908e800e037c40ecbe31f9664","transactions":["0x02f86c0709843b9aca008506fc23ac00830124f89400000000000000000000000000000000000003160180c080a0bb476cdbc61448c8f3df3a60b239ece15aecef4b3e0e2ea83bb826e14d014286a056697ae094e913705310289f44afa0503f911444a5e766fc43cfafb3791eb3da"]}]}
<< (e3f56847) {"jsonrpc":"2.0","result":{"status":"SYNCING","latestValidHash":null,"validationError":null},"id":25}
INFO (Invalid Ancestor Chain Sync, Invalid Timestamp, Invalid P9'): Response from main client: {SYNCING <nil> <nil>}
>> (e3f56847) {"jsonrpc":"2.0","id":26,"method":"engine_forkchoiceUpdatedV1","params":[{"headBlockHash":"0xdb5bb9829af442b13e9251374f216a7ba4ed9f7908e800e037c40ecbe31f9664","safeBlockHash":"0xdb5bb9829af442b13e9251374f216a7ba4ed9f7908e800e037c40ecbe31f9664","finalizedBlockHash":"0x0000000000000000000000000000000000000000000000000000000000000000"},null]}
<< (e3f56847) {"jsonrpc":"2.0","result":{"payloadStatus":{"status":"SYNCING","latestValidHash":null,"validationError":null},"payloadId":null},"id":26}
INFO (Invalid Ancestor Chain Sync, Invalid Timestamp, Invalid P9'): Response from main client fcu: {SYNCING <nil> <nil>}
FAIL (Invalid Ancestor Chain Sync, Invalid Timestamp, Invalid P9'): Timeout waiting for main client to detect invalid chain

This is because reth penalizes the test peer for serving invalid headers:

2023-06-06T04:41:47.584096Z  INFO reth::cli: Executing stage pipeline_stages=1/13 stage=Headers from=0 checkpoint=0
2023-06-06T04:41:47.584143Z DEBUG execute{stage=Headers}: sync::stages::headers: Commencing sync tip=Hash(0x2e1ce2d889533e9d4ec52b239a1680166641694fa828c3689d8470fa5e357007) head=0x67ead97eb79b47a1638659942384143f36ed44275d4182799875ab5a87324055
2023-06-06T04:41:47.584223Z TRACE downloaders::headers: Update sync target current=None new=0x2e1ce2d889533e9d4ec52b239a1680166641694fa828c3689d8470fa5e357007
2023-06-06T04:41:47.584244Z TRACE downloaders::headers: Request new sync target new=Tip(0x2e1ce2d889533e9d4ec52b239a1680166641694fa828c3689d8470fa5e357007)
2023-06-06T04:41:47.584555Z TRACE downloaders::headers: Received sync target head=Some(0) hash=0x2e1ce2d889533e9d4ec52b239a1680166641694fa828c3689d8470fa5e357007 number=14
2023-06-06T04:41:47.584570Z TRACE downloaders::headers: Requesting headers HeadersRequest { start: Number(13), limit: 13, direction: Falling }
2023-06-06T04:41:47.584572Z TRACE downloaders::headers: Submitting headers request request=HeadersRequest { start: Number(13), limit: 13, direction: Falling }
2023-06-06T04:41:47.584857Z TRACE downloaders::headers: Received headers response len=13
2023-06-06T04:41:47.584869Z TRACE downloaders::headers: Validating non-empty headers response requested_block_number=13 highest=13
2023-06-06T04:41:47.585929Z TRACE downloaders::headers: Failed to validate header error=HeaderValidation { hash: 0xaf9dfb2148f7ad2acbf890c1c5ab763c2354b81e0f9bdac0430b69d4c87752c1, error: TimestampIsInPast { parent_timestamp: 4672, timestamp: 4672 } }
2023-06-06T04:41:47.585943Z TRACE downloaders::headers: Penalizing peer peer_id=0x795870779a5a05efb93f2db9063177164b2e026871959f3cdb52af6aa9d8f6d38f92f5246b0d3807f2ed9cda05f87b4754e62987af0617fde0fb30a8894c5fc6 error=HeaderValidation { hash: 0xaf9dfb2148f7ad2acbf890c1c5ab763c2354b81e0f9bdac0430b69d4c87752c1, error: TimestampIsInPast { parent_timestamp: 4672, timestamp: 4672 } }
2023-06-06T04:41:47.585948Z TRACE downloaders::headers: Submitting headers request request=HeadersRequest { start: Number(13), limit: 13, direction: Falling }
2023-06-06T04:41:47.586229Z TRACE downloaders::headers: Received headers response len=13
2023-06-06T04:41:47.586240Z TRACE downloaders::headers: Validating non-empty headers response requested_block_number=13 highest=13
2023-06-06T04:41:47.586547Z TRACE downloaders::headers: Failed to validate header error=HeaderValidation { hash: 0xaf9dfb2148f7ad2acbf890c1c5ab763c2354b81e0f9bdac0430b69d4c87752c1, error: TimestampIsInPast { parent_timestamp: 4672, timestamp: 4672 } }

Because we are fetching the header by number, we have no way of verifying that the peer is serving a header which is part of the canonical chain, and need to re-request the header, in case the peer is malicious and providing bogus header data.

If we were fetching the header by hash, we would know that the peer is serving the requested header when we validate its hash. If we were to request an invalid block by hash, we should not re-request that header, because we will always get an invalid result.

We may want to differentiate between headers that we fetch by hash, and headers that we fetch by number, for this reason. Then, we can:

send a headers request by hash
validate that the response's hashes line up
validate the other properties of the header
- if a header fails validation, we can abort the headers stage completely, and report + invalidate the bad block

Similarly, for the full block downloader, we should request by hash so we can invalidate the bad block.

Steps to reproduce

Run the Invalid Ancestor Chain Sync, Invalid Timestamp test:

./hive --client reth --sim "ethereum/engine" --sim.limit "engine-api/Invalid Ancestor Chain Sync, Invalid Timestamp"

Node logs

No response

Platform(s)

No response

What version/commit are you on?

No response

Code of Conduct

[X] I agree to follow the Code of Conduct

mattsse commented 1 year ago

this is more of a pipeline issue

an invalid block by hash, we should not re-request

the headers stage does not know that

the question here is, why is the pipeline here active in the first place?

Rjected commented 1 year ago

the question here is, why is the pipeline here active in the first place?

because the CL mocker sends a FCU, and this is the first FCU, so we are running the pipeline

mattsse commented 1 year ago

ah sigh

oh, what if we always fetch the full block first before triggering pipeline, this way we can also check the distance

Rjected commented 1 year ago

Makes sense, so #3039 should probably be followed up with distance checks, which should address #2964

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 14 days with no activity.

onbjerg commented 1 year ago

Ping @Rjected @mattsse on this

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 14 days with no activity.

Rjected commented 1 year ago

Going to ignore the Headers stage case for now, since these hive tests now use the live sync downloaders. Now, the problem is that the full block downloaders just report a bad message here, ban the peer, and continue to request the header. Invalid headers are not propagated up to the engine. https://github.com/paradigmxyz/reth/blob/4dbd8835e5f06ddfc5c0987046773d592dbff860/crates/interfaces/src/p2p/full_block.rs#L494-L495

To solve this, we could have the full block downloader (and range downloader) return either a regular SealedBlock, or another variant for invalid headers or blocks. To allow the beacon engine to mark it as invalid, we would have to propagate this result up to here: https://github.com/paradigmxyz/reth/blob/4dbd8835e5f06ddfc5c0987046773d592dbff860/crates/consensus/beacon/src/engine/mod.rs#L1576-L1578

Here, the engine can mark it as invalid

Rjected commented 7 months ago

This is still an issue, we could initialize the future with a channel that the full block future pushes to if it has capacity, otherwise it should not block on insert - would be DoS risk.

But before this, we need to alter the validate_headers_range method: https://github.com/paradigmxyz/reth/blob/96149f05bedf52ef1a3b00c50e827e9d2992ecb3/crates/interfaces/src/consensus.rs#L39

to return the header that is invalid on error.

paradigmxyz / reth