Skip to content

Conversation

@chfast
Copy link
Member

@chfast chfast commented Jan 18, 2023

The t8n is "state transition" CLI interface used to check and generate
JSON tests. It reads and outputs specified JSON files.
See https://ethereum-tests.readthedocs.io/en/develop/t8ntool-ref.html.

@chfast
Copy link
Member Author

chfast commented Jan 18, 2023

First example to run:

  • geth

    retesteth -t 'GeneralStateTests/stExample' -- --clients t8ntool --testpath ~/Projects/ethereum/tests --singletest add11 --singlenet Berlin --verbosity 6
    
  • evmone

    retesteth -t 'GeneralStateTests/stExample' -- --datadir ~/Projects/ethereum/evmone/config/retesteth --clients evmone --testpath ~/Projects/ethereum/tests --singletest add11 --singlenet Berlin --verbosity 6
    

@chfast
Copy link
Member Author

chfast commented Jan 18, 2023

The example isn't working currently because evmone does not produce expected transaction receipts (empty array in the result). The receipt is match with a transaction by transaction hash which evmone is not able to compute yet.

@rodiazet rodiazet force-pushed the t8n branch 2 times, most recently from 766e42a to 0a02e9d Compare January 26, 2023 14:56
@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Merging #552 (993c8e6) into master (cdd82b7) will decrease coverage by 1.00%.
The diff coverage is 44.61%.

❗ Current head 993c8e6 differs from pull request most recent head f657bc2. Consider uploading reports for the commit f657bc2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
- Coverage   97.97%   96.98%   -1.00%     
==========================================
  Files          63       64       +1     
  Lines        6031     6126      +95     
==========================================
+ Hits         5909     5941      +32     
- Misses        122      185      +63     
Flag Coverage Δ
blockchaintests 77.01% <100.00%> (+0.04%) ⬆️
statetests 71.52% <49.53%> (-1.81%) ⬇️
unittests 92.72% <28.68%> (-1.22%) ⬇️

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

Impacted Files Coverage Δ
test/statetest/statetest.hpp 100.00% <ø> (ø)
test/state/errors.hpp 12.50% <12.50%> (ø)
test/statetest/statetest_loader.cpp 65.69% <39.13%> (-20.02%) ⬇️
lib/evmone/baseline.cpp 98.26% <50.00%> (-1.74%) ⬇️
test/state/state.cpp 98.90% <100.00%> (+10.39%) ⬆️
test/state/state.hpp 96.29% <100.00%> (+0.14%) ⬆️
test/statetest/statetest_runner.cpp 94.11% <100.00%> (+0.36%) ⬆️

namespace json = nlohmann;
using namespace evmone;

namespace evmone::state
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this available there or can this be anonymous?

Copy link
Member

@rodiazet rodiazet Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it has to be there. Similar to statetest_runner.cpp. In other case encode template functions instantiation fails.

test/t8n/t8n.cpp Outdated
txs_logs.insert(txs_logs.begin(), (*tx_logs).begin(), (*tx_logs).end());
auto& j_receipt = j_result["receipts"][j_result["receipts"].size()];
j_receipt["transactionHash"] = j_tx["hash"];
j_receipt["gasUsed"] = tu::shortest_hex(uint64_t(gas_used));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if int64_t gas_used is negative (due to a failure)?

j_receipt["contractAddress"] = NULL_HEXSTRING_20;
j_receipt["logsBloom"] = NULL_HEXSTRING_256;
j_receipt["logs"] = json::json::array(); // FIXME: Add to_json<state:Log>
j_receipt["root"] = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really expected to be empty?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a minimal working version. A lot of fields are required by retesteth but never used.

}
} // namespace evmone::state

int main(int argc, const char* argv[])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a unit test for this (mostly for checking the input -> output mapping is unchanged). Question is: is it better to use a CLI test or make this into an internal library first?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the second. @chfast What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will add some unit testing later. I have some ideas for JSON loading.

@axic
Copy link
Member

axic commented Feb 6, 2023

Should remove .config.swp before merging. Also why is there a need to duplicate the config files, aren't they part of reteseth?

@rodiazet
Copy link
Member

rodiazet commented Feb 6, 2023

Should remove .config.swp before merging. Also why is there a need to duplicate the config files, aren't they part of reteseth?

Definitely configs should be moved to retesteth. I will remove them just before merging. They are useful when it's in progress.

evmc_result execute(evmc_vm* c_vm, const evmc_host_interface* host, evmc_host_context* ctx,
evmc_revision rev, const evmc_message* msg, const uint8_t* code, size_t code_size) noexcept
{
if (rev >= EVMC_SHANGHAI && is_eof_code({code, code_size}) &&
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For code equals 0xfe00 (EOF) function read_valid_eof1_header (called by analyze) assumes that there is something else but it's the whole code so it tries to read data from the outside of the code section.

@@ -0,0 +1,80 @@
// evmone: Fast Ethereum Virtual Machine implementation
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should go to test/state.


if (rev >= EVMC_SHANGHAI && !tx.to.has_value() && tx.data.size() > max_initcode_size)
return -1; // initcode size is limited by EIP-3860.
return make_error_code(INIT_CODE_SIZE_LIMIT_EXCEEDED); // initcode size is limited by
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move comment before if.


json::json j_result;
// FIXME: Calculate difficulty properly
j_result["currentDifficulty"] = "0x20000";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right?

}
} // namespace evmone::state

int main(int argc, const char* argv[])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will add some unit testing later. I have some ideas for JSON loading.

"socketType" : "tranition-tool",
"socketAddress" : "start.sh",
"transactionsAsJson" : true,
"checkLogsHash" : false,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to try this?

@chfast chfast changed the title evmone t8n tool Add evmone t8n tool Feb 7, 2023
@chfast chfast marked this pull request as ready for review February 7, 2023 20:40
@chfast chfast force-pushed the t8n branch 2 times, most recently from 3b7a50e to 72db700 Compare February 7, 2023 21:05
@chfast chfast merged commit 404b213 into master Feb 7, 2023
@chfast chfast deleted the t8n branch February 7, 2023 21:35
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.

4 participants