node: fix some clang-tidy warnings (erigontech#1663)

.clang-tidy

@@ -49,7 +49,6 @@ Checks: >
   bugprone-undefined-memory-manipulation,
   bugprone-undelegated-constructor,
   bugprone-unhandled-self-assignment,
-  bugprone-unused-raii,
   bugprone-unused-return-value,
   bugprone-use-after-move,
   bugprone-virtual-near-miss,

silkworm/infra/common/log_test.cpp

@@ -133,7 +133,6 @@ TEST_CASE("LogBuffer", "[silkworm][common][log]") {
 
     SECTION("Settings disable colorized output if log file present") {
         // Default output is colorized
-        // NOLINTNEXTLINE(bugprone-unused-raii)
         LogBuffer_ForTest<Level::kInfo>{"test0", {"key1", "value1", "key2", "value2"}};  // temporary log object, flush on dtor
         const auto cerr_output0{string_cerr.str()};
         CHECK(cerr_output0.find("test0") != std::string::npos);
@@ -152,7 +151,6 @@ TEST_CASE("LogBuffer", "[silkworm][common][log]") {
             .log_file = temp_file.string(),
         };
         init(log_settings1);
-        // NOLINTNEXTLINE(bugprone-unused-raii)
         LogBuffer_ForTest<Level::kInfo>{"test1", {"key1", "value1", "key2", "value2"}};  // temporary log object, flush on dtor
         const auto cerr_output1{string_cerr.str()};
         CHECK(cerr_output1.find("test1") != std::string::npos);
@@ -171,7 +169,6 @@ TEST_CASE("LogBuffer", "[silkworm][common][log]") {
             .log_file = temp_file.string(),
         };
         init(log_settings2);
-        // NOLINTNEXTLINE(bugprone-unused-raii)
         LogBuffer_ForTest<Level::kInfo>{"test2", {"key3", "value3", "key4", "value4"}};  // temporary log object, flush on dtor
         const auto cerr_output2{string_cerr.str()};
         CHECK(cerr_output2.find("test2") != std::string::npos);

silkworm/infra/concurrency/awaitable_future.hpp

@@ -46,6 +46,7 @@ class AwaitableFuture {
 
     Task<T> get_async() {
         try {
+            // NOLINTNEXTLINE(clang-analyzer-core.NullDereference)
             std::optional<T> result = co_await channel_->async_receive(boost::asio::use_awaitable);
             co_return std::move(result.value());
         } catch (const boost::system::system_error& ex) {

silkworm/node/backend/remote/grpc/kv_calls.cpp

@@ -136,7 +136,7 @@ Task<void> TxCall::operator()(const EthereumBackEnd& backend) {
             }
         };
         const auto write = [&]() -> Task<void> {
-            while (co_await write_stream.next()) {
+            while (co_await write_stream.next()) {  // NOLINT(clang-analyzer-core.NullDereference)
             }
         };
         const auto max_idle_timer = [&]() -> Task<void> {
@@ -658,7 +658,7 @@ Task<void> StateChangesCall::operator()(const EthereumBackEnd& backend) {
     auto source = backend.state_change_source();
 
     // Create a never-expiring timer whose cancellation will notify our async waiting is completed
-    auto coroutine_executor = co_await boost::asio::this_coro::executor;
+    auto coroutine_executor = co_await boost::asio::this_coro::executor;  // NOLINT(clang-analyzer-core.CallAndMessage)
     auto notifying_timer = steady_timer{coroutine_executor};
 
     std::optional<remote::StateChangeBatch> incoming_batch;

silkworm/node/db/bitmap.cpp

@@ -296,9 +296,13 @@ std::optional<uint64_t> seek(const roaring::Roaring64Map& bitmap, uint64_t n) {
     return std::nullopt;
 }
 
-roaring::Roaring cut_left(roaring::Roaring& bm, uint64_t size_limit) { return cut_left_impl(bm, size_limit); }
+roaring::Roaring cut_left(roaring::Roaring& bitmap, uint64_t size_limit) {
+    return cut_left_impl(bitmap, size_limit);
+}
 
-roaring::Roaring64Map cut_left(roaring::Roaring64Map& bm, uint64_t size_limit) { return cut_left_impl(bm, size_limit); }
+roaring::Roaring64Map cut_left(roaring::Roaring64Map& bitmap, uint64_t size_limit) {
+    return cut_left_impl(bitmap, size_limit);
+}
 
 template <typename RoaringMap>
 Bytes bitmap_to_bytes(RoaringMap& bitmap) {

silkworm/node/db/bitmap.hpp

@@ -121,7 +121,7 @@ Bytes to_bytes(roaring::Roaring& bitmap);
 roaring::Roaring64Map parse(const mdbx::slice& data);
 
 //! \brief Parse 64-bit roaring bitmap from ByteView
-roaring::Roaring64Map parse(const ByteView data);
+roaring::Roaring64Map parse(ByteView data);
 
 //! \brief Parse 32-bit roaring bitmap from MDBX slice
 roaring::Roaring parse32(const mdbx::slice& data);

silkworm/node/db/buffer.cpp

@@ -408,8 +408,8 @@ void Buffer::insert_call_traces(BlockNum block_number, const CallTraces& traces)
         if (traces.recipients.contains(account)) {
             value[kAddressLength] |= 2;
         }
-        values.insert(std::move(value));
         batch_history_size_ += value.size();
+        values.insert(std::move(value));
     }
     call_traces_.emplace(block_number, values);
 }

silkworm/node/db/eth_status_data_provider.cpp

@@ -42,21 +42,21 @@ void EthStatusDataProvider::HeadInfo::debug_log() const {
         });
 }
 
-EthStatusDataProvider::HeadInfo EthStatusDataProvider::read_head_info(ROTxn& db_tx_) {
+EthStatusDataProvider::HeadInfo EthStatusDataProvider::read_head_info(ROTxn& txn) {
     HeadInfo head_info;
 
-    BlockNum head_height = db::stages::read_stage_progress(db_tx_, db::stages::kBlockBodiesKey);
+    BlockNum head_height = db::stages::read_stage_progress(txn, db::stages::kBlockBodiesKey);
     head_info.block_num = head_height;
 
-    auto head_hash = db::read_canonical_hash(db_tx_, head_height);
+    auto head_hash = db::read_canonical_hash(txn, head_height);
     if (head_hash) {
         head_info.hash = head_hash.value();
     } else {
         log::Warning("EthStatusDataProvider") << "canonical hash at height " << std::to_string(head_height) << " not found in db";
         return head_info;
     }
 
-    auto head_total_difficulty = db::read_total_difficulty(db_tx_, head_height, *head_hash);
+    auto head_total_difficulty = db::read_total_difficulty(txn, head_height, *head_hash);
     if (head_total_difficulty) {
         head_info.total_difficulty = head_total_difficulty.value();
     } else {

silkworm/node/db/mdbx.hpp

@@ -528,36 +528,36 @@ enum class CursorMoveDirection {
 
 //! \brief Executes a function on each record reachable by the provided cursor
 //! \param [in] cursor : A reference to a cursor opened on a map
-//! \param [in] func : A reference to a function with the code to execute on records. Note the return value of the
+//! \param [in] walker : A reference to a function with the code to execute on records. Note the return value of the
 //! function may stop the loop
 //! \param [in] direction : Whether the cursor should navigate records forward (default) or backwards
 //! \return The overall number of processed records
 //! \remarks If the provided cursor is *not* positioned on any record it will be moved to either the beginning or the
 //! end of the table on behalf of the move criteria
-size_t cursor_for_each(ROCursor& cursor, WalkFuncRef func,
+size_t cursor_for_each(ROCursor& cursor, WalkFuncRef walker,
                        CursorMoveDirection direction = CursorMoveDirection::Forward);
 
 //! \brief Executes a function on each record reachable by the provided cursor asserting keys start with provided prefix
 //! \param [in] cursor : A reference to a cursor opened on a map
 //! \param [in] prefix : The prefix each key must start with
-//! \param [in] func : A reference to a function with the code to execute on records. Note the return value of the
+//! \param [in] walker : A reference to a function with the code to execute on records. Note the return value of the
 //! function may stop the loop
 //! \param [in] direction : Whether the cursor should navigate records forward (default) or backwards
 //! \return The overall number of processed records
-size_t cursor_for_prefix(ROCursor& cursor, ByteView prefix, WalkFuncRef func,
+size_t cursor_for_prefix(ROCursor& cursor, ByteView prefix, WalkFuncRef walker,
                          CursorMoveDirection direction = CursorMoveDirection::Forward);
 
 //! \brief Executes a function on each record reachable by the provided cursor up to a max number of iterations
 //! \param [in] cursor : A reference to a cursor opened on a map
-//! \param [in] func : A reference to a function with the code to execute on records. Note the return value of the
+//! \param [in] walker : A reference to a function with the code to execute on records. Note the return value of the
 //! function may stop the loop
 //! \param [in] max_count : Max number of iterations
 //! \param [in] direction : Whether the cursor should navigate records forward (default) or backwards
 //! \return The overall number of processed records. Should it not match the value of max_count it means the cursor has
 //! reached either the end or the beginning of table earlier
 //! \remarks If the provided cursor is *not* positioned on any record it will be moved to either the beginning or the
 //! end of the table on behalf of the move criteria
-size_t cursor_for_count(ROCursor& cursor, WalkFuncRef func, size_t max_count,
+size_t cursor_for_count(ROCursor& cursor, WalkFuncRef walker, size_t max_count,
                         CursorMoveDirection direction = CursorMoveDirection::Forward);
 
 //! \brief Erases map records by cursor until any record is found

silkworm/node/etl/util.cpp

@@ -25,7 +25,7 @@ std::string errno2str(int err_code) {
         (void)strncpy_s(msg, "Unknown error", _TRUNCATE);
     }
 #else
-    if (strerror_r(err_code, msg, sizeof(msg)) != 0) {
+    if (strerror_r(err_code, msg, sizeof(msg))) {
         (void)strncpy(msg, "Unknown error", sizeof(msg));
     }
 #endif

silkworm/node/huffman/decompressor.cpp

@@ -180,12 +180,12 @@ CodeWord* PatternTable::insert_word(CodeWord* codeword) {
     } else {
         codeword->set_next(nullptr);
         if (head_ == nullptr) {
-            codewords_.push_back(std::move(codeword));
+            codewords_.push_back(codeword);
             head_ = codewords_.front();
             inserted = head_;
         } else {
             SILKWORM_ASSERT(!codewords_.empty());
-            codewords_.push_back(std::move(codeword));
+            codewords_.push_back(codeword);
             inserted = codewords_.back();
         }
     }
@@ -297,7 +297,7 @@ std::ostream& operator<<(std::ostream& out, const PositionTable& pt) {
 }
 
 Decompressor::Decompressor(std::filesystem::path compressed_path, std::optional<MemoryMappedRegion> compressed_region)
-    : compressed_path_(std::move(compressed_path)), compressed_region_{std::move(compressed_region)} {}
+    : compressed_path_(std::move(compressed_path)), compressed_region_{compressed_region} {}
 
 Decompressor::~Decompressor() {
     close();

silkworm/node/huffman/decompressor.hpp

@@ -243,7 +243,7 @@ class Decompressor {
 
     [[nodiscard]] const std::filesystem::path& compressed_path() const { return compressed_path_; }
 
-    [[nodiscard]] const std::string compressed_filename() const { return compressed_path_.filename().string(); }
+    [[nodiscard]] std::string compressed_filename() const { return compressed_path_.filename().string(); }
 
     [[nodiscard]] uint64_t words_count() const { return words_count_; }
 

silkworm/node/node.cpp

@@ -80,7 +80,7 @@ class NodeImpl final {
 
 NodeImpl::NodeImpl(Settings& settings, SentryClientPtr sentry_client, mdbx::env chaindata_db)
     : settings_{settings},
-      chaindata_db_{chaindata_db},
+      chaindata_db_{std::move(chaindata_db)},
       snapshot_repository_{settings_.snapshot_settings},
       execution_server_{settings_, db::RWAccess{chaindata_db_}},
       execution_local_client_{execution_server_},
@@ -179,7 +179,7 @@ Task<void> NodeImpl::start_execution_log_timer() {
 }
 
 Node::Node(Settings& settings, SentryClientPtr sentry_client, mdbx::env chaindata_db)
-    : p_impl_(std::make_unique<NodeImpl>(settings, std::move(sentry_client), chaindata_db)) {}
+    : p_impl_(std::make_unique<NodeImpl>(settings, std::move(sentry_client), std::move(chaindata_db))) {}
 
 // Must be here (not in header) because NodeImpl size is necessary for std::unique_ptr in PIMPL idiom
 Node::~Node() = default;

silkworm/node/recsplit/rec_split.hpp

@@ -44,26 +44,21 @@
 
 #pragma once
 
-/* clang-format off */
-#define _USE_MATH_DEFINES
-#include <cmath>
-#if !defined(M_PI) && defined(_MSC_VER)
-#include <corecrt_math_defines.h>
-#endif
-/* clang-format on */
-
 #include <array>
 #include <bit>
 #include <cassert>
 #include <chrono>
+#include <cmath>
 #include <fstream>
 #include <limits>
+#include <numbers>
 #include <random>
 #include <stdexcept>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include <gsl/narrow>
 #include <gsl/util>
 
 #include <silkworm/core/common/assert.hpp>
@@ -233,7 +228,7 @@ class RecSplit {
 
     explicit RecSplit(std::filesystem::path index_path, std::optional<MemoryMappedRegion> index_region = {})
         : index_path_{index_path},
-          encoded_file_{std::make_optional<MemoryMappedFile>(std::move(index_path), std::move(index_region))} {
+          encoded_file_{std::make_optional<MemoryMappedFile>(std::move(index_path), index_region)} {
         SILK_TRACE << "RecSplit encoded file path: " << encoded_file_->path();
         check_minimum_length(kFirstMetadataHeaderLength);
 
@@ -262,9 +257,11 @@ class RecSplit {
         SILKWORM_ASSERT(leaf_size == LEAF_SIZE);
         offset += kLeafSizeLength;
 
-        const uint16_t primary_aggr_bound = leaf_size * succinct::max(2, std::ceil(0.35 * leaf_size + 1. / 2));
+        const uint16_t primary_aggr_bound = leaf_size *
+                                            succinct::max(2, gsl::narrow<int64_t>(std::ceil(0.35 * leaf_size + 0.5)));
         SILKWORM_ASSERT(primary_aggr_bound == kLowerAggregationBound);
-        const uint16_t secondary_aggr_bound = primary_aggr_bound * (leaf_size < 7 ? 2 : ceil(0.21 * leaf_size + 9. / 10));
+        const uint16_t secondary_aggr_bound = primary_aggr_bound *
+                                              (leaf_size < 7 ? 2 : gsl::narrow<uint16_t>(std::ceil(0.21 * leaf_size + 0.9)));
         SILKWORM_ASSERT(secondary_aggr_bound == kUpperAggregationBound);
 
         // Read salt
@@ -277,7 +274,7 @@ class RecSplit {
         offset += kStartSeedSizeLength;
         SILKWORM_ASSERT(start_seed_length == kStartSeed.size());
         check_minimum_length(offset + start_seed_length * sizeof(uint64_t));
-        std::array<uint64_t, kStartSeed.size()> start_seed;
+        std::array<uint64_t, kStartSeed.size()> start_seed{};
         for (std::size_t i{0}; i < start_seed_length; ++i) {
             start_seed[i] = endian::load_big_u64(address + offset);
             offset += sizeof(uint64_t);
@@ -679,8 +676,8 @@ class RecSplit {
             sqrt_prod *= sqrt(k[i]);
         }
 
-        const double p = sqrt(m) / (pow(2 * M_PI, (fanout - 1.) / 2) * sqrt_prod);
-        auto golomb_rice_length = static_cast<uint32_t>(ceil(log2(-std::log((sqrt(5) + 1) / 2) / log1p(-p))));  // log2 Golomb modulus
+        const double p = sqrt(m) / (pow(2 * std::numbers::pi, (fanout - 1.) * 0.5) * sqrt_prod);
+        auto golomb_rice_length = static_cast<uint32_t>(ceil(log2(-std::log((sqrt(5) + 1) * 0.5) / log1p(-p))));  // log2 Golomb modulus
 
         SILKWORM_ASSERT(golomb_rice_length <= 0x1F);  // Golomb-Rice code, stored in the 5 upper bits
         (*memo)[m] = golomb_rice_length << 27;

silkworm/node/snapshot/index.hpp

@@ -31,7 +31,7 @@ class Index {
     static constexpr std::size_t kBucketSize{2'000};
 
     explicit Index(SnapshotPath segment_path, std::optional<MemoryMappedRegion> segment_region = {})
-        : segment_path_(std::move(segment_path)), segment_region_{std::move(segment_region)} {}
+        : segment_path_(std::move(segment_path)), segment_region_{segment_region} {}
     virtual ~Index() = default;
 
     [[nodiscard]] SnapshotPath path() const { return segment_path_.index_file(); }
@@ -48,7 +48,7 @@ class Index {
 class HeaderIndex : public Index {
   public:
     explicit HeaderIndex(SnapshotPath segment_path, std::optional<MemoryMappedRegion> segment_region = {})
-        : Index(std::move(segment_path), std::move(segment_region)) {}
+        : Index(std::move(segment_path), segment_region) {}
 
   protected:
     bool walk(succinct::RecSplit8& rec_split, uint64_t i, uint64_t offset, ByteView word) override;
@@ -57,7 +57,7 @@ class HeaderIndex : public Index {
 class BodyIndex : public Index {
   public:
     explicit BodyIndex(SnapshotPath segment_path, std::optional<MemoryMappedRegion> segment_region = {})
-        : Index(std::move(segment_path), std::move(segment_region)), uint64_buffer_(8, '\0') {}
+        : Index(std::move(segment_path), segment_region), uint64_buffer_(8, '\0') {}
 
   protected:
     bool walk(succinct::RecSplit8& rec_split, uint64_t i, uint64_t offset, ByteView word) override;
@@ -69,7 +69,7 @@ class BodyIndex : public Index {
 class TransactionIndex : public Index {
   public:
     explicit TransactionIndex(SnapshotPath segment_path, std::optional<MemoryMappedRegion> segment_region = {})
-        : Index(std::move(segment_path), std::move(segment_region)) {}
+        : Index(std::move(segment_path), segment_region) {}
 
     void build() override;
 

silkworm/node/stagedsync/execution_pipeline.cpp

@@ -70,11 +70,11 @@ ExecutionPipeline::ExecutionPipeline(silkworm::NodeSettings* node_settings)
     load_stages();
 }
 
-BlockNum ExecutionPipeline::head_header_number() {
+BlockNum ExecutionPipeline::head_header_number() const {
     return head_header_number_;
 }
 
-Hash ExecutionPipeline::head_header_hash() {
+Hash ExecutionPipeline::head_header_hash() const {
     return head_header_hash_;
 }
 
@@ -201,7 +201,7 @@ Stage::Result ExecutionPipeline::forward(db::RWTxn& cycle_txn, BlockNum target_h
             if (start_stage_name) {
                 // Stage is not the start one, skip it and continue
                 if (start_stage_name != stage_id) {
-                    log::Info("Skipping " + std::string(stage_id) + "...", {"START_AT_STAGE", start_stage_name->c_str(), "hit", "true"});
+                    log::Info("Skipping " + std::string(stage_id) + "...", {"START_AT_STAGE", *start_stage_name, "hit", "true"});
                     continue;
                 } else {
                     // Start stage just found, avoid skipping next stages
@@ -211,7 +211,7 @@ Stage::Result ExecutionPipeline::forward(db::RWTxn& cycle_txn, BlockNum target_h
 
             // Check if we have to stop due to environment variable
             if (stop_stage_name && stop_stage_name == stage_id) {
-                log::Warning("Stopping ...", {"STOP_BEFORE_STAGE", stop_stage_name->c_str(), "hit", "true"});
+                log::Warning("Stopping ...", {"STOP_BEFORE_STAGE", *stop_stage_name, "hit", "true"});
                 result = Stage::Result::kStoppedByEnv;
                 break;
             }

silkworm/node/stagedsync/execution_pipeline.hpp

@@ -28,14 +28,14 @@ namespace silkworm::stagedsync {
 class ExecutionPipeline : public Stoppable {
   public:
     explicit ExecutionPipeline(NodeSettings*);
-    ~ExecutionPipeline() = default;
+    ~ExecutionPipeline() override = default;
 
     Stage::Result forward(db::RWTxn&, BlockNum target_height);
     Stage::Result unwind(db::RWTxn&, BlockNum unwind_point);
     Stage::Result prune(db::RWTxn&);
 
-    BlockNum head_header_number();
-    Hash head_header_hash();
+    BlockNum head_header_number() const;
+    Hash head_header_hash() const;
     std::optional<BlockNum> unwind_point();
     std::optional<Hash> bad_block();