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

WIP: Improve t8n pipeline allowing for invalid tx fields during t8n.evaluate #272

Closed

Conversation

spencer-tb
Copy link
Collaborator

Following this issue: #242, but only for tx fields.

Allows for an invalid transaction field to be used within t8n.

WIP: Currently this adapts the previous issue: #243, where a tx.to address is set to None. Testing this filled test on hive for geth now correctly returns an appropriate response.

error="invalid transaction 0: rlp: input string too short for common.Address, decoding into (types.BlobTx).To

@spencer-tb spencer-tb force-pushed the invalid-transaction-to-address branch from 3e9901f to 94a55e8 Compare August 28, 2023 14:33
@marioevz
Copy link
Member

I don't really like the idea of the framework automatically replacing a value that it thinks it's correct for the tester in the case of an overflow.

For example, what is the "correct" equivalent of the overflowing timestamp value when testing the beacon root pre-deploy?

There is no single answer to this because there could be different bugs in different clients:

  1. The client implementation internally wraps around when setting uint64 to $2^{64}$ to zero, so the test equivalent would be sending a 0 to t8n in the timestamp and verifying that the storage is set to zero in the appropriate places, and then setting $2^{64}$ on the built header rlp.
  2. The client for some bizarre reason interprets $2^{64}$ as $2^{64}-1$, so the test equivalent would be sending a $2^{64}-1$ to t8n in the timestamp and verifying that the storage is set to $2^{64}-1$ in the appropriate places, and then setting $2^{64}$ on the built header rlp.
  3. The client implementation internally uses uint256 and $2^{64}$ is read as is, so the test equivalent would be sending a $2^{64}$ to t8n in the timestamp and verifying that the storage is set to $2^{64}$ in the appropriate places, and also setting $2^{64}$ on the built header rlp.

(1) and (2) are somewhat doable just in this framework, for both of them we have:

  • a value that goes to the t8n
  • a value that ends up in the header RLP

but both of these values require care from the tester, and there could be multiple test cases with different values on each.

(3) is more complicated and requires either:

  • support from t8n to modify the storage appropriately in the result or
  • execution-spec-tests having the means to modify the alloc values returned by t8n and then calculating the state root and placing the modified state root in the header, along with the modified timestamp value

Sure we could simply modify the values passed to t8n and then modify the header and call it a day, but the problem is that this is not an effective test case because if the client incorrectly reads an overflowing timestamp header field and then proceeds to execute the block, but the state root is still incorrect, it will still return INVALID and the test will pass, but we didn't catch the bug where the client did read the incorrect header.

Another topic is that there are at least two different places where there is incorrect parsing of a fields:

  1. On engine_newPayload via the Engine API
  2. Via syncing when reading the block in RLP format

(1) is how we already test on the pyspec sim, but for (2) I don't think we have an easy way to deliver this block yet, as we would need to peer with the client via devp2p, send an engine_forkchoiceUpdated without a prior engine_newPayload and then handle block serving via the devp2p.

@marioevz
Copy link
Member

I think for this PR we can settle with making options (1) and (2) feasible and open issues to track the viability of (3) and the hive syncing tests on the hive repo.

For (1) and (2) I would suggest we create an special object that has attributes that contain:

  • the value sent to the t8n tool
  • the value that goes into the header RLP and the Engine API field for hive

And then behave accordingly when preparing the environment JSON output to t8n, and the header building/JSON for the Engine API.

@spencer-tb
Copy link
Collaborator Author

Closing in favor of the method used in: #379

Where we can override the tx rlp within blocks by serializing the Transaction dataclass into its respective invalid tx rlp.

@spencer-tb spencer-tb closed this Feb 7, 2024
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 this pull request may close these issues.

None yet

2 participants