Merge bitcoin#34488: refactor: Small style and test fixups for bitcoinkernel

fad9dd1 test: kernel test fixups (MarcoFalke)
fabb58d test: Use clang-tidy named args for create_chainman (MarcoFalke)
fa51594 refactor: Small style fixups in src/kernel/bitcoinkernel.cpp (MarcoFalke)

Pull request description:

  Just some small style and test fixups after bitcoin#30595 (review)

ACKs for top commit:
  stickies-v:
    re-ACK fad9dd1
  frankomosh:
    Code Review ACK fad9dd1. All changes are sound refactoring with no functional issues. Nice improvements to readability (named args in create_chainman, span.data(), range checks now properly require non-empty).

Tree-SHA512: 0a92e871b4db75a590acad39672594625e402895bc0d36635d36ec2fe8dce7cc2c5cb6ebf2a92bc14617d94648b84bffb95ff783cea71bd91ac4a9871ef5dbef

Author: fanquake

src/kernel/bitcoinkernel.cpp

@@ -41,7 +41,6 @@
 #include <cstring>
 #include <exception>
 #include <functional>
-#include <iterator>
 #include <list>
 #include <memory>
 #include <span>
@@ -56,7 +55,7 @@ using util::ImmediateTaskRunner;
 
 // Define G_TRANSLATION_FUN symbol in libbitcoinkernel library so users of the
 // library aren't required to export this symbol
-extern const std::function<std::string(const char*)> G_TRANSLATION_FUN{nullptr};
+extern const TranslateFn G_TRANSLATION_FUN{nullptr};
 
 static const kernel::Context btck_context_static{};
 
@@ -84,7 +83,7 @@ class WriterStream
     //
     void write(std::span<const std::byte> src)
     {
-        if (m_writer(std::data(src), src.size(), m_user_data) != 0) {
+        if (m_writer(src.data(), src.size(), m_user_data) != 0) {
             throw std::runtime_error("Failed to write serialization data");
         }
     }
@@ -113,13 +112,13 @@ struct Handle {
     static C* create(Args&&... args)
     {
         auto cpp_obj{std::make_unique<CPP>(std::forward<Args>(args)...)};
-        return reinterpret_cast<C*>(cpp_obj.release());
+        return ref(cpp_obj.release());
     }
 
     static C* copy(const C* ptr)
     {
         auto cpp_obj{std::make_unique<CPP>(get(ptr))};
-        return reinterpret_cast<C*>(cpp_obj.release());
+        return ref(cpp_obj.release());
     }
 
     static const CPP& get(const C* ptr)

src/kernel/bitcoinkernel.h

@@ -82,9 +82,9 @@ extern "C" {
  * @section error Error handling
  *
  * Functions communicate an error through their return types, usually returning
- * a nullptr, 0, or false if an error is encountered. Additionally, verification
- * functions, e.g. for scripts, may communicate more detailed error information
- * through status code out parameters.
+ * a nullptr or a status code as documented by the returning function.
+ * Additionally, verification functions, e.g. for scripts, may communicate more
+ * detailed error information through status code out parameters.
  *
  * Fine-grained validation information is communicated through the validation
  * interface.

src/test/kernel/test_kernel.cpp

@@ -265,9 +265,7 @@ void run_verify_test(
 }
 
 template <typename T>
-concept HasToBytes = requires(T t) {
-    { t.ToBytes() } -> std::convertible_to<std::vector<std::byte>>;
-};
+concept HasToBytes = requires(T t) { t.ToBytes(); };
 
 template <typename T>
 void CheckHandle(T object, T distinct_object)
@@ -277,7 +275,9 @@ void CheckHandle(T object, T distinct_object)
     BOOST_CHECK(object.get() != distinct_object.get());
 
     if constexpr (HasToBytes<T>) {
-        BOOST_CHECK_NE(object.ToBytes().size(), distinct_object.ToBytes().size());
+        const auto object_bytes = object.ToBytes();
+        const auto distinct_bytes = distinct_object.ToBytes();
+        BOOST_CHECK(!std::ranges::equal(object_bytes, distinct_bytes));
     }
 
     // Copy constructor
@@ -321,7 +321,8 @@ void CheckRange(const RangeType& range, size_t expected_size)
     using value_type = std::ranges::range_value_t<RangeType>;
 
     BOOST_CHECK_EQUAL(range.size(), expected_size);
-    BOOST_CHECK_EQUAL(range.empty(), (expected_size == 0));
+    BOOST_REQUIRE(range.size() > 0); // Some checks below assume a non-empty range
+    BOOST_REQUIRE(!range.empty());
 
     BOOST_CHECK(range.begin() != range.end());
     BOOST_CHECK_EQUAL(std::distance(range.begin(), range.end()), static_cast<std::ptrdiff_t>(expected_size));
@@ -332,7 +333,6 @@ void CheckRange(const RangeType& range, size_t expected_size)
         BOOST_CHECK_EQUAL(range[i].get(), (*(range.begin() + i)).get());
     }
 
-    BOOST_CHECK_NE(range.at(0).get(), range.at(expected_size - 1).get());
     BOOST_CHECK_THROW(range.at(expected_size), std::out_of_range);
 
     BOOST_CHECK_EQUAL(range.front().get(), range[0].get());
@@ -792,7 +792,9 @@ void chainman_reindex_test(TestDirectory& test_directory)
 {
     auto notifications{std::make_shared<TestKernelNotifications>()};
     auto context{create_context(notifications, ChainType::MAINNET)};
-    auto chainman{create_chainman(test_directory, true, false, false, false, context)};
+    auto chainman{create_chainman(
+        test_directory, /*reindex=*/true, /*wipe_chainstate=*/false,
+        /*block_tree_db_in_memory=*/false, /*chainstate_db_in_memory=*/false, context)};
 
     std::vector<std::string> import_files;
     BOOST_CHECK(chainman->ImportBlocks(import_files));
@@ -835,7 +837,9 @@ void chainman_reindex_chainstate_test(TestDirectory& test_directory)
 {
     auto notifications{std::make_shared<TestKernelNotifications>()};
     auto context{create_context(notifications, ChainType::MAINNET)};
-    auto chainman{create_chainman(test_directory, false, true, false, false, context)};
+    auto chainman{create_chainman(
+        test_directory, /*reindex=*/false, /*wipe_chainstate=*/true,
+        /*block_tree_db_in_memory=*/false, /*chainstate_db_in_memory=*/false, context)};
 
     std::vector<std::string> import_files;
     import_files.push_back((test_directory.m_directory / "blocks" / "blk00000.dat").string());
@@ -847,7 +851,9 @@ void chainman_mainnet_validation_test(TestDirectory& test_directory)
     auto notifications{std::make_shared<TestKernelNotifications>()};
     auto validation_interface{std::make_shared<TestValidationInterface>()};
     auto context{create_context(notifications, ChainType::MAINNET, validation_interface)};
-    auto chainman{create_chainman(test_directory, false, false, false, false, context)};
+    auto chainman{create_chainman(
+        test_directory, /*reindex=*/false, /*wipe_chainstate=*/false,
+        /*block_tree_db_in_memory=*/false, /*chainstate_db_in_memory=*/false, context)};
 
     // mainnet block 1
     auto raw_block = hex_string_to_byte_vec("010000006fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000982051fd1e4ba744bbbe680e1fee14677ba1a3c3540bf7b1cdb606e857233e0e61bc6649ffff001d01e362990101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff0704ffff001d0104ffffffff0100f2052a0100000043410496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858eeac00000000");
@@ -975,7 +981,9 @@ BOOST_AUTO_TEST_CASE(btck_chainman_in_memory_tests)
 
     auto notifications{std::make_shared<TestKernelNotifications>()};
     auto context{create_context(notifications, ChainType::REGTEST)};
-    auto chainman{create_chainman(in_memory_test_directory, false, false, true, true, context)};
+    auto chainman{create_chainman(
+        in_memory_test_directory, /*reindex=*/false, /*wipe_chainstate=*/false,
+        /*block_tree_db_in_memory=*/true, /*chainstate_db_in_memory=*/true, context)};
 
     for (auto& raw_block : REGTEST_BLOCK_DATA) {
         Block block{hex_string_to_byte_vec(raw_block)};
@@ -999,7 +1007,9 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
     auto context{create_context(notifications, ChainType::REGTEST)};
 
     {
-        auto chainman{create_chainman(test_directory, false, false, false, false, context)};
+        auto chainman{create_chainman(
+            test_directory, /*reindex=*/false, /*wipe_chainstate=*/false,
+            /*block_tree_db_in_memory=*/false, /*chainstate_db_in_memory=*/false, context)};
         for (const auto& data : REGTEST_BLOCK_DATA) {
             Block block{hex_string_to_byte_vec(data)};
             BlockHeader header = block.GetHeader();
@@ -1021,7 +1031,9 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
     const size_t mid{REGTEST_BLOCK_DATA.size() / 2};
 
     {
-        auto chainman{create_chainman(test_directory, false, false, false, false, context)};
+        auto chainman{create_chainman(
+            test_directory, /*reindex=*/false, /*wipe_chainstate=*/false,
+            /*block_tree_db_in_memory=*/false, /*chainstate_db_in_memory=*/false, context)};
         for (size_t i{0}; i < mid; i++) {
             Block block{hex_string_to_byte_vec(REGTEST_BLOCK_DATA[i])};
             bool new_block{false};
@@ -1030,7 +1042,9 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
         }
     }
 
-    auto chainman{create_chainman(test_directory, false, false, false, false, context)};
+    auto chainman{create_chainman(
+        test_directory, /*reindex=*/false, /*wipe_chainstate=*/false,
+        /*block_tree_db_in_memory=*/false, /*chainstate_db_in_memory=*/false, context)};
 
     for (size_t i{mid}; i < REGTEST_BLOCK_DATA.size(); i++) {
         Block block{hex_string_to_byte_vec(REGTEST_BLOCK_DATA[i])};