GH-1039 Modify fork_database to return a descriptive enum for add() i…

…nstead of true/false

libraries/chain/controller.cpp

@@ -4232,11 +4232,11 @@ struct controller_impl {
          assert(!verify_qc_future.valid());
       }
 
-      bool best_head = fork_db.add(bsp, ignore_duplicate_t::yes);
+      fork_db_add_t add_result = fork_db.add(bsp, ignore_duplicate_t::yes);
       if constexpr (is_proper_savanna_block)
          vote_processor.notify_new_block(async_aggregation);
 
-      return controller::accepted_block_result{best_head, block_handle{std::move(bsp)}};
+      return controller::accepted_block_result{add_result, block_handle{std::move(bsp)}};
    }
 
    // thread safe, expected to be called from thread other than the main thread

libraries/chain/fork_database.cpp

@@ -94,7 +94,7 @@ namespace eosio::chain {
 
       void             open_impl( const char* desc, const std::filesystem::path& fork_db_file, fc::cfile_datastream& ds, validator_t& validator );
       void             close_impl( std::ofstream& out );
-      bool             add_impl( const bsp_t& n, ignore_duplicate_t ignore_duplicate, bool validate, validator_t& validator );
+      fork_db_add_t    add_impl( const bsp_t& n, ignore_duplicate_t ignore_duplicate, bool validate, validator_t& validator );
       bool             is_valid() const;
 
       bsp_t            get_block_impl( const block_id_type& id, include_root_t include_root = include_root_t::no ) const;
@@ -240,8 +240,8 @@ namespace eosio::chain {
    }
 
    template <class BSP>
-   bool fork_database_impl<BSP>::add_impl(const bsp_t& n, ignore_duplicate_t ignore_duplicate,
-                                          bool validate, validator_t& validator) {
+   fork_db_add_t fork_database_impl<BSP>::add_impl(const bsp_t& n, ignore_duplicate_t ignore_duplicate,
+                                                   bool validate, validator_t& validator) {
       EOS_ASSERT( root, fork_database_exception, "root not yet set" );
       EOS_ASSERT( n, fork_database_exception, "attempt to add null block state" );
 
@@ -277,15 +277,25 @@ namespace eosio::chain {
          EOS_RETHROW_EXCEPTIONS( fork_database_exception, "serialized fork database is incompatible with configured protocol features" )
       }
 
+      auto prev_head = head_impl(include_root_t::yes);
+
       auto inserted = index.insert(n);
       EOS_ASSERT(ignore_duplicate == ignore_duplicate_t::yes || inserted.second, fork_database_exception,
                  "duplicate block added: ${id}", ("id", n->id()));
 
-      return inserted.second && n == head_impl(include_root_t::no);
+      if (!inserted.second)
+         return fork_db_add_t::duplicate;
+      const bool new_head = n == head_impl(include_root_t::no);
+      if (new_head && n->previous() == prev_head->id())
+         return fork_db_add_t::appended_to_head;
+      if (new_head)
+         return fork_db_add_t::fork_switch;
+
+      return fork_db_add_t::added;
    }
 
    template<class BSP>
-   bool fork_database_t<BSP>::add( const bsp_t& n, ignore_duplicate_t ignore_duplicate ) {
+   fork_db_add_t fork_database_t<BSP>::add( const bsp_t& n, ignore_duplicate_t ignore_duplicate ) {
       std::lock_guard g( my->mtx );
       return my->add_impl(n, ignore_duplicate, false,
                           [](block_timestamp_type         timestamp,

libraries/chain/include/eosio/chain/controller.hpp

@@ -96,6 +96,7 @@ namespace eosio::chain {
    using resource_limits::resource_limits_manager;
    using apply_handler = std::function<void(apply_context&)>;
 
+   enum class fork_db_add_t;
    using forked_callback_t = std::function<void(const transaction_metadata_ptr&)>;
 
    // lookup transaction_metadata via supplied function to avoid re-creation
@@ -235,7 +236,7 @@ namespace eosio::chain {
          void set_async_aggregation(async_t val);
 
          struct accepted_block_result {
-            const bool is_new_best_head = false; // true if new best head
+            const fork_db_add_t add_result;
             std::optional<block_handle> block;   // empty optional if block is unlinkable
          };
          // thread-safe

libraries/chain/include/eosio/chain/fork_database.hpp

@@ -12,6 +12,7 @@ namespace eosio::chain {
    using block_branch_t = std::vector<signed_block_ptr>;
    enum class ignore_duplicate_t { no, yes };
    enum class include_root_t { no, yes };
+   enum class fork_db_add_t { duplicate, added, appended_to_head, fork_switch };
 
    // Used for logging of comparison values used for best fork determination
    std::string log_fork_comparison(const block_state& bs);
@@ -67,9 +68,14 @@ namespace eosio::chain {
       /**
        *  Add block state to fork database.
        *  Must link to existing block in fork database or the root.
-       *  @return true if n becomes the new best head (and was not the best head before)
+       *  @returns duplicate - already added and ignore_duplicate=true
+       *           added - inserted into an existing branch or started a new branch, not best branch
+       *           appended_to_head - new best head of current best branch
+       *           fork_switch - new best head of new branch, fork switch to new branch
+       *  @throws unlinkable_block_exception - unlinkable to any branch
+       *  @throws fork_database_exception - no root, n is nullptr, protocol feature error, duplicate when ignore_duplicate=false
        */
-      bool add( const bsp_t& n, ignore_duplicate_t ignore_duplicate );
+      fork_db_add_t add( const bsp_t& n, ignore_duplicate_t ignore_duplicate );
 
       void remove( const block_id_type& id );
 
@@ -306,3 +312,6 @@ namespace eosio::chain {
       static constexpr uint32_t max_supported_version = 3;
    };
 } /// eosio::chain
+
+FC_REFLECT_ENUM( eosio::chain::fork_db_add_t,
+                 (duplicate)(added)(appended_to_head)(fork_switch) )

plugins/net_plugin/net_plugin.cpp

@@ -9,6 +9,7 @@
 #include <eosio/chain/plugin_interface.hpp>
 #include <eosio/chain/thread_utils.hpp>
 #include <eosio/producer_plugin/producer_plugin.hpp>
+#include <eosio/chain/fork_database.hpp>
 
 #include <fc/bitutil.hpp>
 #include <fc/network/message_buffer.hpp>
@@ -3722,7 +3723,7 @@ namespace eosio {
             }
             // this will return empty optional<block_handle> if block is not linkable
             controller::accepted_block_result abh = cc.accept_block( id, ptr );
-            best_head = abh.is_new_best_head;
+            best_head = abh.add_result == fork_db_add_t::appended_to_head || abh.add_result == fork_db_add_t::fork_switch;
             obh = std::move(abh.block);
             unlinkable = !obh;
             close_mode = sync_manager::closing_mode::handshake;

plugins/test_control_plugin/test_control_plugin.cpp

@@ -1,4 +1,5 @@
 #include <eosio/test_control_plugin/test_control_plugin.hpp>
+#include <eosio/chain/fork_database.hpp>
 
 namespace eosio {
 
@@ -160,9 +161,9 @@ void test_control_plugin_impl::swap_action_in_block(const chain::signed_block_pt
    copy_b->producer_signature = _swap_on_options.blk_priv_key.sign(copy_b->calculate_id());
 
    // will be processed on the next start_block if is_new_best_head
-   const auto&[is_new_best_head, bh] = _chain.accept_block(copy_b->calculate_id(), copy_b);
-   ilog("Swapped action ${f} to ${t}, is_new_best_head ${bh}, block ${bn}",
-        ("f", _swap_on_options.from)("t", _swap_on_options.to)("bh", is_new_best_head)("bn", bh ? bh->block_num() : 0));
+   const auto&[add_result, bh] = _chain.accept_block(copy_b->calculate_id(), copy_b);
+   ilog("Swapped action ${f} to ${t}, add_result ${a}, block ${bn}",
+        ("f", _swap_on_options.from)("t", _swap_on_options.to)("a", add_result)("bn", bh ? bh->block_num() : 0));
    reset_swap_action();
 }
 

unittests/fork_db_tests.cpp

@@ -7,8 +7,9 @@
 
 namespace eosio::chain {
 
+uint32_t nonce = 0;
+
 inline block_id_type make_block_id(block_num_type block_num) {
-   static uint32_t nonce = 0;
    ++nonce;
    block_id_type id = fc::sha256::hash(std::to_string(block_num) + "-" + std::to_string(nonce));
    id._hash[0] &= 0xffffffff00000000;
@@ -57,26 +58,38 @@ using namespace eosio::chain;
 struct generate_fork_db_state {
    generate_fork_db_state() {
       fork_db.reset_root(root);
-      fork_db.add(bsp11a, ignore_duplicate_t::no);
-      fork_db.add(bsp11b, ignore_duplicate_t::no);
-      fork_db.add(bsp11c, ignore_duplicate_t::no);
-      fork_db.add(bsp12a, ignore_duplicate_t::no);
-      fork_db.add(bsp13a, ignore_duplicate_t::no);
-      fork_db.add(bsp12b, ignore_duplicate_t::no);
-      fork_db.add(bsp12bb, ignore_duplicate_t::no);
-      fork_db.add(bsp12bbb, ignore_duplicate_t::no);
-      fork_db.add(bsp12c, ignore_duplicate_t::no);
-      fork_db.add(bsp13b, ignore_duplicate_t::no);
-      fork_db.add(bsp13bb, ignore_duplicate_t::no);
-      fork_db.add(bsp13bbb, ignore_duplicate_t::no);
-      fork_db.add(bsp14b, ignore_duplicate_t::no);
-      fork_db.add(bsp13c, ignore_duplicate_t::no);
+      BOOST_TEST((fork_db.add(bsp11a, ignore_duplicate_t::no) == fork_db_add_t::appended_to_head));
+      BOOST_TEST((fork_db.add(bsp11b, ignore_duplicate_t::no) == fork_db_add_t::added));
+      BOOST_TEST((fork_db.add(bsp11c, ignore_duplicate_t::no) == fork_db_add_t::added));
+         BOOST_TEST((fork_db.add(bsp12a, ignore_duplicate_t::no) == fork_db_add_t::appended_to_head));
+            BOOST_TEST((fork_db.add(bsp13a, ignore_duplicate_t::no) == fork_db_add_t::appended_to_head));
+         BOOST_TEST((fork_db.add(bsp12b, ignore_duplicate_t::no) == fork_db_add_t::added));
+         BOOST_TEST((fork_db.add(bsp12bb, ignore_duplicate_t::no) == fork_db_add_t::added));
+         BOOST_TEST((fork_db.add(bsp12bbb, ignore_duplicate_t::no) == fork_db_add_t::added));
+         BOOST_TEST((fork_db.add(bsp12c, ignore_duplicate_t::no) == fork_db_add_t::added));
+            BOOST_TEST((fork_db.add(bsp13b, ignore_duplicate_t::no) == fork_db_add_t::fork_switch));
+
+            // no fork_switch, because id is less
+            BOOST_TEST(bsp13bb->latest_qc_block_timestamp() == bsp13b->latest_qc_block_timestamp());
+            BOOST_TEST(bsp13bb->timestamp() == bsp13b->timestamp());
+            BOOST_TEST(bsp13bb->id() < bsp13b->id());
+            BOOST_TEST((fork_db.add(bsp13bb, ignore_duplicate_t::no) == fork_db_add_t::added));
+
+            // fork_switch by id, everything else is the same
+            BOOST_TEST(bsp13bbb->latest_qc_block_timestamp() == bsp13b->latest_qc_block_timestamp());
+            BOOST_TEST(bsp13bbb->timestamp() == bsp13b->timestamp());
+            BOOST_TEST(bsp13bbb->id() > bsp13b->id());
+            BOOST_TEST((fork_db.add(bsp13bbb, ignore_duplicate_t::no) == fork_db_add_t::fork_switch));
+
+               BOOST_TEST((fork_db.add(bsp14b, ignore_duplicate_t::no) == fork_db_add_t::fork_switch));
+            BOOST_TEST((fork_db.add(bsp13c, ignore_duplicate_t::no) == fork_db_add_t::added));
    }
 
    fork_database_if_t fork_db;
 
    // Setup fork database with blocks based on a root of block 10
    // Add a number of forks in the fork database
+   bool reset_nonce = [&]() { nonce = 0; return true; }();
    block_state_ptr root = test_block_state_accessor::make_genesis_block_state();
    block_state_ptr   bsp11a = test_block_state_accessor::make_unique_block_state(11, root);
    block_state_ptr     bsp12a = test_block_state_accessor::make_unique_block_state(12, bsp11a);
@@ -112,12 +125,12 @@ BOOST_FIXTURE_TEST_CASE(add_remove_test, generate_fork_db_state) try {
    BOOST_TEST(!fork_db.get_block(bsp12b->id()));
    BOOST_TEST(!fork_db.get_block(bsp13b->id()));
    BOOST_TEST(!fork_db.get_block(bsp14b->id()));
-   BOOST_TEST(!fork_db.add(bsp12b, ignore_duplicate_t::no)); // will throw if already exists
+   BOOST_TEST((fork_db.add(bsp12b, ignore_duplicate_t::no) == fork_db_add_t::added)); // will throw if already exists
    // 13b not the best branch because 13c has higher timestamp
-   BOOST_TEST(!fork_db.add(bsp13b, ignore_duplicate_t::no)); // will throw if already exists
+   BOOST_TEST((fork_db.add(bsp13b, ignore_duplicate_t::no) == fork_db_add_t::added)); // will throw if already exists
    // 14b has a higher timestamp than 13c
-   BOOST_TEST(fork_db.add(bsp14b, ignore_duplicate_t::no)); // will throw if already exists
-   BOOST_TEST(!fork_db.add(bsp14b, ignore_duplicate_t::yes));
+   BOOST_TEST((fork_db.add(bsp14b, ignore_duplicate_t::no) == fork_db_add_t::fork_switch)); // will throw if already exists
+   BOOST_TEST((fork_db.add(bsp14b, ignore_duplicate_t::yes) == fork_db_add_t::duplicate));
 
    // test search
    BOOST_TEST(fork_db.search_on_branch( bsp13bb->id(), 11) == bsp11b);
@@ -143,7 +156,7 @@ BOOST_FIXTURE_TEST_CASE(add_remove_test, generate_fork_db_state) try {
    BOOST_TEST(branch[1] == bsp11a);
 
    auto bsp14c = test_block_state_accessor::make_unique_block_state(14, bsp13c); // should be best branch
-   BOOST_TEST(fork_db.add(bsp14c, ignore_duplicate_t::yes));
+   BOOST_TEST((fork_db.add(bsp14c, ignore_duplicate_t::yes) == fork_db_add_t::fork_switch));
 
    // test fetch branch when lib is greater than head
    branch = fork_db.fetch_branch(bsp13b->id(), bsp12a->id());