Blockchain tests support: Add nonce value verification #685

rodiazet · 2023-08-09T14:22:40Z

No description provided.

chfast

All of this looks to be done in a wrong place. We should check it in validate_transaction() where we inspect sender's nonce already.

codecov · 2023-08-10T09:04:08Z

Codecov Report

Merging #685 (39419a7) into master (1d0c994) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
+ Coverage   97.48%   97.52%   +0.04%     
==========================================
  Files          85       86       +1     
  Lines        8182     8208      +26     
==========================================
+ Hits         7976     8005      +29     
+ Misses        206      203       -3

Flag	Coverage Δ
blockchaintests	`62.71% <ø> (ø)`
statetests	`74.12% <66.66%> (-0.10%)`	⬇️
statetests-silkpre	`23.23% <34.78%> (-0.02%)`	⬇️
unittests	`95.11% <100.00%> (+0.10%)`	⬆️

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

Files Changed	Coverage Δ
test/state/state.hpp	`100.00% <ø> (ø)`
test/unittests/state_transition.hpp	`0.00% <ø> (ø)`
test/state/errors.hpp	`30.55% <100.00%> (+18.05%)`	⬆️
test/state/state.cpp	`98.59% <100.00%> (+0.04%)`	⬆️
test/statetest/statetest_loader.cpp	`90.17% <100.00%> (+0.04%)`	⬆️
test/unittests/state_tx_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/statetest_loader_test.cpp	`100.00% <100.00%> (ø)`

test/state/state.cpp

chfast

Why are many state tests failing?

rodiazet · 2023-08-17T11:58:40Z

Why are many state tests failing?

Because tx which is built by statetest_runner has nonce == 0 when the sender nonce == 1. It's because we have a bug in static void from_json(const json::json& j, TestMultiTransaction& o) . It does not read nonce from json test file.

Fixed with additional commit.

chfast · 2023-08-17T13:10:26Z

test/statetest/statetest_loader.cpp

@@ -251,6 +251,9 @@ static void from_json_tx_common(const json::json& j, state::Transaction& o)
 {
    o.sender = from_json<evmc::address>(j.at("sender"));

+    if (const auto nonce_it = j.find("nonce"); nonce_it != j.end())


Isn't the nonce always expected there?

Our load_minimal_test test does not define nonce. So we can add that it's mandatory or we load it only when it's defined.

Should be mandatory

chfast · 2023-08-17T19:16:39Z

test/unittests/state_validate_transaction_test.cpp

+
+using namespace evmone::state;
+
+TEST(transaction, validate_transaction)


Suggested change

TEST(transaction, validate_transaction)

TEST(state_tx, validate_nonce)

Also change file name to state_tx_test.cpp

chfast · 2023-08-17T19:17:43Z

test/unittests/state_validate_transaction_test.cpp

+
+    {
+        const auto res = validate_transaction(acc, bi, tx, EVMC_BERLIN);
+        ASSERT_TRUE(holds_alternative<int64_t>(res)) << std::get<std::error_code>(res).message();


Suggested change

ASSERT_TRUE(holds_alternative<int64_t>(res)) << std::get<std::error_code>(res).message();

EXPECT_FALSE(holds_alternative<std::error_code>(res)) << std::get<std::error_code>(res).message();

chfast · 2023-08-17T19:18:36Z

test/unittests/state_validate_transaction_test.cpp

+    {
+        const auto res = validate_transaction(acc, bi, tx, EVMC_BERLIN);
+        ASSERT_TRUE(holds_alternative<std::error_code>(res))
+            << make_error_code(ErrorCode::NONCE_TOO_LOW).message();


Why do you need this additional message?

No we don't. I will add also checking the error code returned.

chfast · 2023-08-18T11:09:17Z

test/unittests/state_tx_test.cpp

+
+    const BlockInfo bi{.gas_limit = 0x989680,
+        .coinbase = 0x01_address,
+        .prev_randao = {},


Suggested change

.prev_randao = {},

I think you can skip default initialized fields.

No I cannot. CI complains during build.

rodiazet requested review from chfast and gumb0 August 9, 2023 14:22

chfast reviewed Aug 9, 2023

View reviewed changes

rodiazet force-pushed the verify-nonce branch from 6376694 to 05319d9 Compare August 16, 2023 17:38

chfast reviewed Aug 16, 2023

View reviewed changes

test/state/state.cpp Show resolved Hide resolved

rodiazet changed the title ~~Blockchain tests fixing: Add nonce value verification~~ Blockchain tests support: Add nonce value verification Aug 17, 2023

chfast requested changes Aug 17, 2023

View reviewed changes

rodiazet force-pushed the verify-nonce branch from c004a2e to d46c47c Compare August 17, 2023 12:38

chfast reviewed Aug 17, 2023

View reviewed changes

rodiazet force-pushed the verify-nonce branch 2 times, most recently from 6eaedc2 to 601416c Compare August 17, 2023 15:46

chfast requested changes Aug 17, 2023

View reviewed changes

rodiazet force-pushed the verify-nonce branch 16 times, most recently from 7c5d040 to 32c1434 Compare August 18, 2023 10:56

rodiazet requested a review from chfast August 18, 2023 11:08

chfast approved these changes Aug 18, 2023

View reviewed changes

rodiazet added 3 commits August 18, 2023 13:29

state: Add nonce value verification

83097bd

state: Add loading nonce of tx for state tests

5e77e97

state: Add unit tests for validate_transaction

39419a7

rodiazet force-pushed the verify-nonce branch from 32c1434 to 39419a7 Compare August 18, 2023 11:29

rodiazet merged commit 7cef356 into master Aug 18, 2023
3 checks passed

rodiazet deleted the verify-nonce branch August 18, 2023 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blockchain tests support: Add nonce value verification #685

Blockchain tests support: Add nonce value verification #685

rodiazet commented Aug 9, 2023

chfast left a comment

codecov bot commented Aug 10, 2023 •

edited

Loading

chfast left a comment

rodiazet commented Aug 17, 2023 •

edited

Loading

chfast Aug 17, 2023

rodiazet Aug 17, 2023

chfast Aug 17, 2023

chfast Aug 17, 2023

chfast Aug 17, 2023

chfast Aug 17, 2023

rodiazet Aug 18, 2023

chfast Aug 18, 2023

rodiazet Aug 18, 2023


		using namespace evmone::state;

		TEST(transaction, validate_transaction)

	TEST(transaction, validate_transaction)
	TEST(state_tx, validate_nonce)

	ASSERT_TRUE(holds_alternative<int64_t>(res)) << std::get<std::error_code>(res).message();
	EXPECT_FALSE(holds_alternative<std::error_code>(res)) << std::get<std::error_code>(res).message();

Blockchain tests support: Add nonce value verification #685

Blockchain tests support: Add nonce value verification #685

Conversation

rodiazet commented Aug 9, 2023

chfast left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2023 • edited Loading

Codecov Report

chfast left a comment

Choose a reason for hiding this comment

rodiazet commented Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2023 •

edited

Loading

rodiazet commented Aug 17, 2023 •

edited

Loading