search

ethereum / devp2p

Ethereum peer-to-peer networking specifications
971 stars 273 forks source link

Eth68 encoding of `txType` doesn't match client implementations for `NewPooledTransactionHashes` #245

Closed acolytec3 closed 11 months ago

acolytec3 commented 11 months ago

The current spec for the Eth68 message format for NewPooledTransactionHashes is this: [[txtype₁: P, txtype₂: P, ...], [txsize₁: P, txsize₂: P, ...], [txhash₁: B_32, txhash₂: B_32, ...]]

Geth (followed by Nethermind, Reth, and EthereumJS) have implemented [ txType1: P || txType2: P || txType3: P, [size1: P, size2: P, size3: P], [th1: B_32, th2: B_32, th3: B_32]], the key difference being that the transaction types array is flattened into one concatenated RLP string where each individual byte represents a transaction type rather than an RLP list with elements of type P (assumed to be unpadded bytes though this is not documented anywhere that I could find).

This does make sense since TransactionType is borrowed from EIP2718 where TransactionType is not anticipated to be greater than 0x7f so will always fit in a single byte/uint8.

As such, we should either update the spec for Eth68 to match what has been implemented by the various clients or else have the client teams update code to match the current spec.

Current behavior can be tested using this hive test "Request Blob Pooled Transactions" in the suite linked in this fork of Hive

acolytec3 commented 11 months ago

Just to follow up here with notes from conversation on the R&D Discord. Erigon confirmed they also implemented the "wrong" version, following geth.

There is general agreement to consider either modifying the eth/68 spec to match current implementations or else deprecate and replace with a new eth/69 spec that all teams agree on.

Will discuss this on the dencun testing call today

fjl commented 11 months ago

On the Dencun testing call, it was decided we will change the eth/68 spec to match the thing implementations are doing.

We can change the tx announcement message again in eth/69, whenever that happens.

fjl commented 11 months ago

Spec change: https://github.com/ethereum/devp2p/pull/246