From d6d3d8a54cce9deebb6c6a444216eb64c4661b8f Mon Sep 17 00:00:00 2001 From: canepat <16927169+canepat@users.noreply.github.com> Date: Tue, 16 Jan 2024 11:47:08 +0100 Subject: [PATCH] core: amend incarnation logic (#1746) --- silkworm/core/execution/evm_test.cpp | 96 +++++++++++++++++++++-- silkworm/core/state/intra_block_state.cpp | 10 ++- silkworm/core/types/account.hpp | 1 + silkworm/node/db/buffer.cpp | 4 +- silkworm/node/db/buffer_test.cpp | 40 ++++++++-- 5 files changed, 136 insertions(+), 15 deletions(-) diff --git a/silkworm/core/execution/evm_test.cpp b/silkworm/core/execution/evm_test.cpp index 2d0d372ac8..9dfa2f9f08 100644 --- a/silkworm/core/execution/evm_test.cpp +++ b/silkworm/core/execution/evm_test.cpp @@ -67,7 +67,7 @@ TEST_CASE("Value transfer", "[core][execution]") { CHECK(state.touched().count(to) == 1); } -TEST_CASE("Destruct and create", "[core][execution]") { +TEST_CASE("Destruct and recreate", "[core][execution]") { evmc::address to{0x8b299e2b7d7f43c0ce3068263545309ff4ffb521_address}; InMemoryState db; @@ -75,8 +75,10 @@ TEST_CASE("Destruct and create", "[core][execution]") { { IntraBlockState state{db}; - // First create the contract in a "transaction" and push it to the DB + // First, create the contract and set one storage location to non-zero in a block state.clear_journal_and_substate(); + REQUIRE(state.get_original_storage(to, {}) == evmc::bytes32{}); + REQUIRE(state.get_current_storage(to, {}) == evmc::bytes32{}); state.create_contract(to); state.set_storage(to, {}, evmc::bytes32{1}); REQUIRE(state.get_current_storage(to, {}) == evmc::bytes32{1}); @@ -88,10 +90,13 @@ TEST_CASE("Destruct and create", "[core][execution]") { { IntraBlockState state{db}; - // Then destruct it in another "transaction" and "block" + // Then, in another block, destruct it state.clear_journal_and_substate(); - CHECK(state.record_suicide(to)); + REQUIRE(state.get_original_storage(to, {}) == evmc::bytes32{1}); + REQUIRE(state.get_current_storage(to, {}) == evmc::bytes32{1}); + REQUIRE(state.record_suicide(to)); state.destruct_suicides(); + REQUIRE(state.get_current_storage(to, {}) == evmc::bytes32{}); state.finalize_transaction(EVMC_SHANGHAI); // Add some balance to it @@ -99,16 +104,91 @@ TEST_CASE("Destruct and create", "[core][execution]") { state.add_to_balance(to, 1); state.finalize_transaction(EVMC_SHANGHAI); - // Recreate it + // And recreate it: the storage location previously set to non-zero must be zeroed state.clear_journal_and_substate(); + CHECK(state.get_original_storage(to, {}) == evmc::bytes32{}); + CHECK(state.get_current_storage(to, {}) == evmc::bytes32{}); + state.create_contract(to); + CHECK(state.get_current_storage(to, {}) == evmc::bytes32{}); + state.finalize_transaction(EVMC_SHANGHAI); + state.write_to_db(2); + CHECK(db.state_root_hash() == 0x8e723de3b34ef0632b5421f0f8ad8dfa6c981e99009141b5b7130c790f0d38c6_bytes32); + } +} + +TEST_CASE("Create contract, destruct and then recreate", "[core][execution]") { + evmc::address to{0x8b299e2b7d7f43c0ce3068263545309ff4ffb521_address}; + + InMemoryState db; + + { + IntraBlockState state{db}; + + // First, create an empty contract in one block + REQUIRE((state.get_nonce(to) == 0 && state.get_code_hash(to) == kEmptyHash)); state.create_contract(to); - // The following check does not pass, so skipping for now (fix in PR #1553 breaks state root trie) - // CHECK(state.get_current_storage(to, {}) == evmc::bytes32{}); + state.set_code(to, *from_hex("30600155")); + state.finalize_transaction(EVMC_SHANGHAI); + + state.write_to_db(1); + + const auto account{db.read_account(to)}; + CHECK((account && account->incarnation == 1)); + } + + { + IntraBlockState state{db}; + + // Then, in another block, destruct it + state.clear_journal_and_substate(); + REQUIRE(state.record_suicide(to)); + state.destruct_suicides(); + state.finalize_transaction(EVMC_SHANGHAI); + state.write_to_db(2); - CHECK(db.state_root_hash() == 0x73ea1e235dec8e2f576eadd4173322b5ff7a442a1d09ff8da4941d18e03ff071_bytes32); + + CHECK(!db.read_account(to)); + } + + { + IntraBlockState state{db}; + + // Finally, recreate the contract in another block + state.create_contract(to); + state.set_code(to, *from_hex("30600255")); + state.finalize_transaction(EVMC_SHANGHAI); + + state.write_to_db(3); + + const auto account{db.read_account(to)}; + CHECK((account && account->incarnation == 2)); } } +TEST_CASE("Create empty contract and recreate non-empty in same block", "[core][execution]") { + evmc::address to{0x8b299e2b7d7f43c0ce3068263545309ff4ffb521_address}; + + InMemoryState db; + IntraBlockState state{db}; + + // First, create an empty contract in one transaction + REQUIRE((state.get_nonce(to) == 0 && state.get_code_hash(to) == kEmptyHash)); + state.create_contract(to); + state.finalize_transaction(EVMC_SHANGHAI); + + // Then, recreate it adding some code in another transaction + state.clear_journal_and_substate(); + REQUIRE((state.get_nonce(to) == 0 && state.get_code_hash(to) == kEmptyHash)); + state.create_contract(to); + state.set_code(to, *from_hex("30600055")); + state.finalize_transaction(EVMC_SHANGHAI); + + state.write_to_db(1); + + const auto account{db.read_account(to)}; + CHECK((account && account->incarnation == 2)); +} + TEST_CASE("Smart contract with storage", "[core][execution]") { Block block{}; block.header.number = 1; diff --git a/silkworm/core/state/intra_block_state.cpp b/silkworm/core/state/intra_block_state.cpp index 20c30b353f..f9336bac74 100644 --- a/silkworm/core/state/intra_block_state.cpp +++ b/silkworm/core/state/intra_block_state.cpp @@ -85,7 +85,11 @@ void IntraBlockState::create_contract(const evmc::address& address) noexcept { created.initial = prev->initial; if (prev->current) { created.current->balance = prev->current->balance; - prev_incarnation = prev->current->incarnation; + if (prev->initial) { + prev_incarnation = std::max(prev->current->incarnation, prev->initial->incarnation); + } else { + prev_incarnation = prev->current->incarnation; + } } else if (prev->initial) { prev_incarnation = prev->initial->incarnation; } @@ -97,8 +101,12 @@ void IntraBlockState::create_contract(const evmc::address& address) noexcept { if (!prev_incarnation || prev_incarnation == 0) { prev_incarnation = db_.previous_incarnation(address); } + if (prev && prev_incarnation < prev->current->previous_incarnation) { + prev_incarnation = prev->current->previous_incarnation; + } created.current->incarnation = *prev_incarnation + 1; + created.current->previous_incarnation = *prev_incarnation; objects_[address] = created; diff --git a/silkworm/core/types/account.hpp b/silkworm/core/types/account.hpp index 5e2fc7930e..4f43dc3bb4 100644 --- a/silkworm/core/types/account.hpp +++ b/silkworm/core/types/account.hpp @@ -34,6 +34,7 @@ struct Account { intx::uint256 balance; evmc::bytes32 code_hash{kEmptyHash}; uint64_t incarnation{0}; + uint64_t previous_incarnation{0}; //! \remarks Erigon's (*Account)EncodeForStorage [[nodiscard]] Bytes encode_for_storage(bool omit_code_hash = false) const; diff --git a/silkworm/node/db/buffer.cpp b/silkworm/node/db/buffer.cpp index 33496e114e..810fcefb93 100644 --- a/silkworm/node/db/buffer.cpp +++ b/silkworm/node/db/buffer.cpp @@ -74,7 +74,9 @@ void Buffer::update_account(const evmc::address& address, std::optional accounts_[address] = current; } - if (account_deleted && initial->incarnation) { + const bool initial_smart_now_deleted{account_deleted && initial->incarnation}; + const bool initial_smart_now_eoa{!account_deleted && current->incarnation == 0 && initial && initial->incarnation}; + if (initial_smart_now_deleted || initial_smart_now_eoa) { if (incarnations_.insert_or_assign(address, initial->incarnation).second) { batch_state_size_ += kAddressLength + kIncarnationLength; } diff --git a/silkworm/node/db/buffer_test.cpp b/silkworm/node/db/buffer_test.cpp index 15e1f33676..ddd3961c70 100644 --- a/silkworm/node/db/buffer_test.cpp +++ b/silkworm/node/db/buffer_test.cpp @@ -14,6 +14,8 @@ limitations under the License. */ +#include + #include #include @@ -122,7 +124,7 @@ TEST_CASE("Account update") { auto changeset_address{bytes_to_address(data_value_view)}; REQUIRE(changeset_address == address); data_value_view.remove_prefix(kAddressLength); - REQUIRE(data_value_view.length() == 0); + REQUIRE(data_value_view.empty()); } SECTION("Changed EOA account") { @@ -153,13 +155,13 @@ TEST_CASE("Account update") { auto changeset_address{bytes_to_address(data_value_view)}; REQUIRE(changeset_address == address); data_value_view.remove_prefix(kAddressLength); - REQUIRE(data_value_view.length() != 0); + REQUIRE(!data_value_view.empty()); auto previous_account{Account::from_encoded_storage(data_value_view)}; CHECK(previous_account == initial_account); } - SECTION("Delete Contract account") { + SECTION("Delete contract account") { const auto address{0xbe00000000000000000000000000000000000000_address}; Account account; account.incarnation = kDefaultIncarnation; @@ -167,16 +169,44 @@ TEST_CASE("Account update") { Buffer buffer{txn, 0}; buffer.begin_block(1); - buffer.update_account(address, /*initial=*/account, std::nullopt); + buffer.update_account(address, /*initial=*/account, /*current=*/std::nullopt); REQUIRE(buffer.account_changes().empty() == false); REQUIRE_NOTHROW(buffer.write_to_db()); auto incarnations{db::open_cursor(txn, table::kIncarnationMap)}; REQUIRE_NOTHROW(incarnations.to_first()); auto data{incarnations.current()}; - REQUIRE(memcmp(data.key.data(), address.bytes, kAddressLength) == 0); + REQUIRE(std::memcmp(data.key.data(), address.bytes, kAddressLength) == 0); REQUIRE(endian::load_big_u64(db::from_slice(data.value).data()) == account.incarnation); } + + SECTION("Delete contract account and recreate as EOA") { + const auto address{0xbe00000000000000000000000000000000000000_address}; + Account account; + account.incarnation = kDefaultIncarnation; + account.code_hash = to_bytes32(keccak256(address.bytes).bytes); // Just a fake hash + + // Block 1: create contract account + Buffer buffer{txn, 0}; + buffer.begin_block(1); + buffer.update_account(address, /*initial=*/std::nullopt, /*current=*/account); + REQUIRE(!buffer.account_changes().empty()); + REQUIRE_NOTHROW(buffer.write_to_db()); + + // Block 2 : destroy contract and recreate account as EOA + buffer.begin_block(2); + Account eoa; + eoa.balance = kEther; + buffer.update_account(address, /*initial=*/account, /*current=*/eoa); + REQUIRE(!buffer.account_changes().empty()); + REQUIRE_NOTHROW(buffer.write_to_db()); + + auto incarnations{db::open_cursor(txn, table::kIncarnationMap)}; + REQUIRE_NOTHROW(incarnations.to_first()); + auto data{incarnations.current()}; + CHECK(std::memcmp(data.key.data(), address.bytes, kAddressLength) == 0); + CHECK(endian::load_big_u64(db::from_slice(data.value).data()) == account.incarnation); + } } } // namespace silkworm::db