Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
acolytec3 opened this issue Aug 4, 2023 · 3 comments · Fixed by #246

Comments

@acolytec3
Copy link

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
Copy link
Author

acolytec3 commented Aug 7, 2023

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
Copy link
Collaborator

fjl commented Aug 7, 2023

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
Copy link
Collaborator

fjl commented Aug 7, 2023

Spec change: #246

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants