EIP-4844 - pass blockchain tests

[pdobacz]

build/bin/evmone-blockchaintest ~/Downloads/fixtures_test_prague_devnet_5/blockchain_tests/cancun/eip4844_blobs/
...
[==========] 43 tests from 7 test suites ran. (4250 ms total)
[  PASSED  ] 43 tests.

🍏

NOTE: I'll squash the commits before merging.

NOTE2: opening as draft to work through CI issues. Not reviewable yet, but if I'm completely off the scent somewhere, please let know

Ready for review

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.30%. Comparing base (7a96b8e) to head (49b2258).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1077      +/-   ##
==========================================
+ Coverage   94.28%   94.30%   +0.02%     
==========================================
  Files         159      159              
  Lines       17229    17273      +44     
==========================================
+ Hits        16245    16290      +45     
+ Misses        984      983       -1     

Flag	Coverage Δ
eof_execution_spec_tests	23.82% <55.38%> (+0.21%)	⬆️
ethereum_tests	26.90% <69.23%> (+0.27%)	⬆️
ethereum_tests_silkpre	18.94% <22.64%> (+<0.01%)	⬆️
execution_spec_tests	21.30% <69.23%> (+0.33%)	⬆️
unittests	88.97% <69.23%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
test/blockchaintest/blockchaintest.hpp	0.00% <ø> (ø)
test/blockchaintest/blockchaintest_loader.cpp	98.90% <100.00%> (+0.12%)	⬆️
test/blockchaintest/blockchaintest_runner.cpp	80.62% <100.00%> (+2.39%)	⬆️
test/state/block.cpp	100.00% <100.00%> (ø)
test/state/block.hpp	100.00% <ø> (ø)
test/state/host.cpp	100.00% <100.00%> (ø)
test/state/state.cpp	98.41% <100.00%> (+0.01%)	⬆️
test/state/transaction.hpp	100.00% <100.00%> (ø)
test/statetest/statetest_loader.cpp	97.81% <100.00%> (ø)
test/t8n/t8n.cpp	88.16% <100.00%> (+0.07%)	⬆️
... and 4 more

[chfast]

I think we should not compute this each access. Maybe we need two sets of fields in BlockInfo: official/header fields and computed values.

If big deal, we can handle this by separate PR.

[pdobacz]

Oh right, get_tx_context() is called within the instruction, yeah, better to fix right away :(. Alternatively this can be kept in Host? WDYT? if we can avoid mixing header/computed values, maybe it's better? or is Host just inappropriate for this?

[chfast]

Better to put it back to BlockInfo because this is also used in transaction validation and transition. These are also important cases.

[pdobacz]

argh, I've done it and only after I recalled from previous week, that I have already been there. The problem which arises is that it makes compute_blob_gas_price come before block validation, making problematic inputs (excess_blob_gas) come through, which previously wasn't the case. fake_exponential seems to go into infinite loop for such inputs.

I'll need to revisit this and look more closely at what exactly is happening

[chfast]

I imagine we keep the newly added field and also have blob_base_fee "after" them computed as some point...

[pdobacz]

I explored this idea, but I'm struggling to find a good point to compute that. It would sacrifice some consts and make a mess, or force us to introduce more complexity to manage that.

I have a counter-proposal. I dug deeper into fake_exponential, and it seems to me like not only it infinite loops but also easily overflows over 256 bits. If I cap the result at uint256::max on mul overflow (I can post code in a moment), things work OK. I think the tacit assumption in fake_exponential spec is that it should use arbitrary precision, and I peeked at some impls and they actually do that.

So I'd rather fix fake_exponential to not loop/overflow, and rely on current block validation, which would reject the block properly.

I see three options here:

1. switch from uint256 to big int of some sort
2. detect overflow and cap compute_blob_gas_price to uint256::max
3. detect overflow and return nullopt from compute_blob_gas_price

2 is the easiest (will push in a sec), but has the disadvantage of there being a block with potentially incorrect blob_base_fee field, hanging around until we reject it (on validation of excess/used blob gas fields).

3 seems better but not sure if the opts won't proliferate around the code base excessively. Maybe not

[pdobacz]

Trying (2.) first

Copilot wasn't able to review any files in this pull request.

Files not reviewed (14)

• test/blockchaintest/blockchaintest.hpp: Language not supported
• test/blockchaintest/blockchaintest_loader.cpp: Language not supported
• test/blockchaintest/blockchaintest_runner.cpp: Language not supported
• test/state/block.cpp: Language not supported
• test/state/block.hpp: Language not supported
• test/state/host.cpp: Language not supported
• test/state/state.cpp: Language not supported
• test/state/transaction.hpp: Language not supported
• test/statetest/statetest_loader.cpp: Language not supported
• test/t8n/t8n.cpp: Language not supported
• test/unittests/state_transition.cpp: Language not supported
• test/unittests/state_transition_block_test.cpp: Language not supported
• test/unittests/state_transition_tx_test.cpp: Language not supported
• test/unittests/state_tx_test.cpp: Language not supported

[pdobacz]

@chfast I've almost forgotten about this one 😅 . Rebased and tests seem to pass, ready to review

[rodiazet]

OK in general. Some nitpicks added. Most important ones with not throwing access to optional already mentioned by @chfast

[rodiazet]

One last nit.