node: update state batch size for initial=current=empty account

node: fix state batch size computation for account code and storage
node: sort storage locations to insert ordered data into db
node: fix unmanaged r/w txn destruction
add unit tests

Author: canepat

silkworm/api/silkworm_api.cpp

@@ -80,10 +80,12 @@ static log::Args log_args_for_version() {
 }
 
 //! Generate log arguments for execution flush at specified block
-static log::Args log_args_for_exec_flush(const db::Buffer& state_buffer, uint64_t current_block) {
+static log::Args log_args_for_exec_flush(const db::Buffer& state_buffer, uint64_t max_batch_size, uint64_t current_block) {
     return {
-        "state",
+        "batch",
         std::to_string(state_buffer.current_batch_state_size()),
+        "max_batch",
+        std::to_string(max_batch_size),
         "block",
         std::to_string(current_block)};
 }
@@ -469,7 +471,7 @@ int silkworm_execute_blocks(SilkwormHandle* handle, MDBX_txn* mdbx_txn, uint64_t
             // Flush state buffer if we've reached the target batch size
             if (state_buffer.current_batch_state_size() >= max_batch_size) {
                 log::Info{"[4/12 Execution] Flushing state",  // NOLINT(*-unused-raii)
-                          log_args_for_exec_flush(state_buffer, block.header.number)};
+                          log_args_for_exec_flush(state_buffer, max_batch_size, block.header.number)};
                 state_buffer.write_state_to_db();
                 gas_batch_size = 0;
                 StopWatch sw{/*auto_start=*/true};
@@ -496,7 +498,7 @@ int silkworm_execute_blocks(SilkwormHandle* handle, MDBX_txn* mdbx_txn, uint64_t
         }
 
         log::Info{"[4/12 Execution] Flushing state",  // NOLINT(*-unused-raii)
-                  log_args_for_exec_flush(state_buffer, max_block)};
+                  log_args_for_exec_flush(state_buffer, max_batch_size, max_block)};
         state_buffer.write_state_to_db();
         StopWatch sw{/*auto_start=*/true};
         txn.commit_and_renew();

silkworm/core/state/intra_block_state.cpp

@@ -332,12 +332,12 @@ void IntraBlockState::write_to_db(uint64_t block_number) {
 
     for (const auto& [address, obj] : objects_) {
         db_.update_account(address, obj.initial, obj.current);
-        if (!obj.current.has_value()) {
+        if (!obj.current) {
             continue;
         }
         const auto& code_hash{obj.current->code_hash};
         if (code_hash != kEmptyHash &&
-            (!obj.initial.has_value() || obj.initial->incarnation != obj.current->incarnation)) {
+            (!obj.initial || obj.initial->incarnation != obj.current->incarnation)) {
             if (auto it{new_code_.find(code_hash)}; it != new_code_.end()) {
                 ByteView code_view{it->second.data(), it->second.size()};
                 db_.update_account_code(address, obj.current->incarnation, code_hash, code_view);

silkworm/node/db/buffer.cpp

@@ -38,6 +38,13 @@ void Buffer::begin_block(uint64_t block_number) {
 
 void Buffer::update_account(const evmc::address& address, std::optional<Account> initial,
                             std::optional<Account> current) {
+    // Skip update if both initial and final state are non-existent (i.e. contract creation+destruction within the same block)
+    if (!initial && !current) {
+        // Only to perfectly match Erigon state batch size (Erigon does count any account w/ old=new=empty value).
+        batch_state_size_ += kAddressLength;
+        return;
+    }
+
     const bool equal{current == initial};
     const bool account_deleted{!current.has_value()};
 
@@ -83,10 +90,12 @@ void Buffer::update_account(const evmc::address& address, std::optional<Account>
 
 void Buffer::update_account_code(const evmc::address& address, uint64_t incarnation, const evmc::bytes32& code_hash,
                                  ByteView code) {
-    // Don't overwrite already existing code so that views of it
-    // that were previously returned by read_code() are still valid.
-    if (hash_to_code_.try_emplace(code_hash, code).second) {
+    // Don't overwrite existing code so that views of it that were previously returned by read_code are still valid
+    const auto [inserted_or_existing_it, inserted] = hash_to_code_.try_emplace(code_hash, code);
+    if (inserted) {
         batch_state_size_ += kHashLength + code.length();
+    } else {
+        batch_state_size_ += code.length() - inserted_or_existing_it->second.length();
     }
 
     if (storage_prefix_to_code_hash_.insert_or_assign(storage_prefix(address, incarnation), code_hash).second) {
@@ -109,8 +118,14 @@ void Buffer::update_storage(const evmc::address& address, uint64_t incarnation,
         }
     }
 
-    if (storage_[address][incarnation].insert_or_assign(location, current).second) {
-        batch_state_size_ += kPlainStoragePrefixLength + kHashLength + kHashLength;
+    // Iterator in insert_or_assign return value "is pointing at the element that was inserted or updated"
+    // so we cannot use it to determine the old value size: we need to use initial instead
+    const auto [_, inserted] = storage_[address][incarnation].insert_or_assign(location, current);
+    ByteView current_val{zeroless_view(current.bytes)};
+    if (inserted) {
+        batch_state_size_ += kPlainStoragePrefixLength + kHashLength + current_val.length();
+    } else {
+        batch_state_size_ += current_val.length() - zeroless_view(initial.bytes).length();
     }
 }
 
@@ -335,9 +350,17 @@ void Buffer::write_state_to_db() {
         if (auto it{storage_.find(address)}; it != storage_.end()) {
             for (const auto& [incarnation, contract_storage] : it->second) {
                 Bytes prefix{storage_prefix(address, incarnation)};
-                for (const auto& [location, value] : contract_storage) {
-                    upsert_storage_value(*state_table, prefix, location.bytes, value.bytes);
-                    written_size += prefix.length() + kLocationLength + kHashLength;
+                // Extract sorted set of storage locations to insert ordered data into the DB
+                absl::btree_set<evmc::bytes32> storage_locations;
+                for (auto& storage_entry : contract_storage) {
+                    storage_locations.insert(storage_entry.first);
+                }
+                for (const auto& location : storage_locations) {
+                    if (auto storage_it{contract_storage.find(location)}; storage_it != contract_storage.end()) {
+                        const auto& value{storage_it->second};
+                        upsert_storage_value(*state_table, prefix, location.bytes, value.bytes);
+                        written_size += prefix.length() + kLocationLength + zeroless_view(value.bytes).size();
+                    }
                 }
             }
             storage_.erase(it);
@@ -439,7 +462,7 @@ void Buffer::insert_block(const Block& block, const evmc::bytes32& hash) {
     uint64_t block_number{block.header.number};
     Bytes key{block_key(block_number, hash.bytes)};
     headers_[key] = block.header;
-    bodies_[key] = block;
+    bodies_[key] = block;  // NOLINT(cppcoreguidelines-slicing)
 
     if (block_number == 0) {
         difficulty_[key] = 0;

silkworm/node/db/buffer_test.cpp

@@ -177,6 +177,26 @@ TEST_CASE("Account update") {
         REQUIRE(memcmp(data.key.data(), address.bytes, kAddressLength) == 0);
         REQUIRE(endian::load_big_u64(db::from_slice(data.value).data()) == account.incarnation);
     }
+
+    SECTION("Change EOA account w/ new value equal to old one") {
+        const auto address{0xbe00000000000000000000000000000000000000_address};
+        Account initial_account;
+        initial_account.nonce = 2;
+        initial_account.balance = kEther;
+
+        Account current_account;
+        current_account.nonce = 2;
+        current_account.balance = kEther;
+
+        Buffer buffer{txn, 0};
+        buffer.begin_block(1);
+        buffer.update_account(address, /*initial=*/initial_account, current_account);
+        REQUIRE(buffer.account_changes().empty());
+        REQUIRE_NOTHROW(buffer.write_to_db());
+
+        auto account_changeset{db::open_cursor(txn, table::kAccountChangeSet)};
+        REQUIRE(txn->get_map_stat(account_changeset.map()).ms_entries == 0);
+    }
 }
 
 }  // namespace silkworm::db

silkworm/node/db/mdbx.cpp

@@ -268,7 +268,9 @@ void RWTxnManaged::commit_and_stop() {
 }
 
 RWTxnUnmanaged::~RWTxnUnmanaged() {
-    abort();
+    if (handle_) {
+        abort();
+    }
 }
 
 void RWTxnUnmanaged::abort() {
@@ -309,6 +311,7 @@ void RWTxnUnmanaged::commit() {
     if (err.code() != MDBX_SUCCESS) {
         err.throw_exception();
     }
+    SILKWORM_ASSERT(!handle_);
     SILK_TRACE << "Commit latency" << detail::log_args_for_commit_latency(commit_latency);
 }
 

silkworm/node/db/mdbx.hpp

@@ -372,7 +372,7 @@ struct EnvConfig {
     size_t page_size{os::page_size()};  // Mdbx page size
     size_t max_size{3_Tebi};            // Mdbx max map size
     size_t growth_size{2_Gibi};         // Increment size for each extension
-    uint32_t max_tables{128};           // Default max number of named tables
+    uint32_t max_tables{256};           // Default max number of named tables
     uint32_t max_readers{100};          // Default max number of readers
 };
 
@@ -394,12 +394,12 @@ ::mdbx::map_handle open_map(::mdbx::txn& tx, const MapConfig& config);
 //! \return A handle to the opened cursor
 ::mdbx::cursor_managed open_cursor(::mdbx::txn& tx, const MapConfig& config);
 
-//! \brief Computes the max size of value data to fit in a leaf data page
-//! \param [in] page_size : the actually configured MDBX's page size
+//! \brief Computes the max size of single-value data to fit into a leaf data page
+//! \param [in] page_size : the actually configured MDBX page size
 //! \param [in] key_size : the known key size to fit in bundle computed value size
 size_t max_value_size_for_leaf_page(size_t page_size, size_t key_size);
 
-//! \brief Computes the max size of value data to fit in a leaf data page
+//! \brief Computes the max size of single-value data to fit into a leaf data page
 //! \param [in] txn : the transaction used to derive pagesize from
 //! \param [in] key_size : the known key size to fit in bundle computed value size
 size_t max_value_size_for_leaf_page(const ::mdbx::txn& txn, size_t key_size);