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

t8ntool support merge rules #24003

Closed
winsvega opened this issue Nov 27, 2021 · 14 comments · Fixed by #24546
Closed

t8ntool support merge rules #24003

winsvega opened this issue Nov 27, 2021 · 14 comments · Fixed by #24546

Comments

@winsvega
Copy link
Contributor

Add new fork to t8ntool --fork Merge for example.
That tells the evm to work with PoS mode. meaning env section follows the nonce, mixhash, difficulty requrements.
random bytecode and so on.

@holiman
Copy link
Contributor

holiman commented Dec 2, 2021

Yes, we need to add this.

@MariusVanDerWijden
Copy link
Member

MariusVanDerWijden commented Jan 4, 2022

I've implemented something here: #24141
For testing the 4399 opcode

@marioevz
Copy link
Member

Hi @MariusVanDerWijden, should we have the terminalTotalDifficulty somewhere as the input to the tests?

In my opinion, I think the difficulty==0 should be a verification rather than an input to the test.

@winsvega suggests that perhaps we should have a "Merge" fork to validate these.

I'm inclined towards having TTD and then if totalDifficulty >= TTD, verify that difficulty==0, opcode difficulty()==currentRandom, etc.

Let me know what you think.

@winsvega
Copy link
Contributor Author

"terminalTotalDifficulty" needs to be in chain config for the client same way we used to have "berlinForkNumber" and so on.

T8n tool should have --fork "Merge" to activate evm in pos mode. The input changes to have "currentRandom", the difficulty calculation dissapear

@winsvega
Copy link
Contributor Author

any news? I can start updating the tests for the PoS format as the blockheaders will change it will affect all tests. need to check that PoS does not break existing tests.

@holiman
Copy link
Contributor

holiman commented Mar 15, 2022

Speculating on some options here:

  1. Add td to input.

With an env like

{
  "currentCoinbase" : "0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba",
  "currentDifficulty" : "0x020000",
  "currentGasLimit" : "0x3b9aca00",
  "currentNumber" : "0x05",
  "currentTimestamp" : "0x03e8",
  "blockHashes" : { "1" : "0xdac58aa524e50956d0c0bae7f3f8bb9d35381365d07804dd5b48a5a297c06af4"}
}

Then when running evm t8n .. --td=0x020000--fork=earlymerge, we could have earlymergedefined as a network with thettdset to0x020000`.
In that case:

  • The terminal total difficulty is hardcoded in fork(s).
  • The total difficulty so far is given as input to the state transition,
  • The difficulty of a block is given in the env input (as before).

One possible drawback is that it won't be possible to know if the ttd was surpassed in this particular block, or an earlier block.

Also, the client should react differently in two cases:

  • If a post-ttd-block arrives through the network, it should be rejected.
  • If a post-ttd-block arrives through the privileged engine API, it should be accepted.

With this schema, we would have to just pretend that the block being tested in t8n arrived via engine API.

@marioevz how does this ^ compare to your suggestion?

@winsvega
Copy link
Contributor Author

winsvega commented Mar 15, 2022

terminal total difficulty is blockchain logic. tool does not do blockchain logic. it is like when running FrontierToHomesteadAt5

all that is required from t8n is access to evm in PoS mode.
so there would be new field currentRandom

@marioevz says there is mode --fakepow to deal with the test blocks. perhaps can be something like --fakepos to accept blocks with difficulty=0 in pos mode

@holiman
Copy link
Contributor

holiman commented Mar 15, 2022

all that is required from t8n is access to evm in PoS mode.
so there would be new field currentRandom

Hm. Yeah I guess you're right!

@marioevz
Copy link
Member

marioevz commented Mar 15, 2022

Regarding the --fakepos, this is related to the hive consensus simulator, which imports the blockchain tests from the test repo, and then it imports each individual block into geth using this:
https://github.com/ethereum/hive/blob/aa4cd345e53a539f814a41315b039d6d26ba57a5/clients/merge-go-ethereum/geth.sh#L115

When we attempt to import a PoS block, it results in Bad Block error:

########## BAD BLOCK #########
Chain config: {ChainID: 1 Homestead: 0 DAO: DAOSupport: true EIP150: 0 EIP155: 0 EIP158: 0 Byzantium: 0 Constantinople: 0 Petersburg: 0 Istanbul: 0, Muir Glacier: , Berlin: 0, London: 0, Arrow Glacier: , MergeFork: , Terminal TD: 0, Engine: ethash}

Number: 1
Hash: 0x8ce374ca14f5c8f25f1a5731cf10e44f8f05ab6968a764fa9d13752e04796c94

Error: invalid difficulty: have 0, want 131072
##############################

The genesis for the test does contain terminalTotalDifficulty==0, but still we don't know if geth should simply accept this block since, in post merge, the execution client would rely on the consensus client to validate this block, but since this is an imported block, there is no consensus client validation involved.

@holiman
Copy link
Contributor

holiman commented Mar 15, 2022

The hive consensus simulator itself will have to be modified -- first importing pow blocks, then importing via some other means -- possibly by feeding the PoS blocks over the rpc engine layer. I don't think solving that is related to the feature here... (?)

@marioevz
Copy link
Member

I think there could be a benefit to test importing PoS blocks by other means other than the Engine API, for example to test fields that the block header has but the Engine API doesn't, like the ommersHash or the nonce.
For the consensus simulator, I like the idea that the Engine API takes over and is used to send the blocks

@marioevz
Copy link
Member

The hive consensus simulator itself will have to be modified -- first importing pow blocks, then importing via some other means -- possibly by feeding the PoS blocks over the rpc engine layer. I don't think solving that is related to the feature here... (?)

Hey @MariusVanDerWijden , regarding this issue, the hive consensus simulator currently tries to import the block using geth import, and it seems like this feature is unaware that the TTD has been reached and still requires the imported blocks to have difficulty > 0.

Do you think it would be worth it to test importing these PoS blocks via the geth import command (Even if we have to send an Engine API forkchoiceUpdated afterwards to make these imported blocks part of the post-merge canonical chain) ?

One upside I see of importing these blocks, using different means other than the Engine API, is that these imported blocks could contain fields that are not available in the Engine API newPayload, such as the ommersHash or the Nonce.

Is this a valid testing scenario or should we simply import everything using the Engine API ?

@MariusVanDerWijden
Copy link
Member

Hmm I think it makes sense for geth import to support post-merge blocks, especially because of EIP-4444 and 4488

@winsvega
Copy link
Contributor Author

Define of terminal total difficulty as 0 in genesis.json should assume that client start in PoS mode since block 1

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

Successfully merging a pull request may close this issue.

6 participants
@ligi @holiman @winsvega @marioevz @MariusVanDerWijden and others