From 771736e7d3ef087344d9499c6c7dbb269a114e39 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 16 Sep 2024 16:52:22 -0400 Subject: [PATCH 01/35] Update `fork_db_validated_block_exists` to lookup parents instead of just claimed block. --- libraries/chain/controller.cpp | 20 ++++++++++++------ libraries/chain/fork_database.cpp | 21 +++++++++++++------ .../include/eosio/chain/fork_database.hpp | 2 +- 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index eb60ec697d..e7a18c6956 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1130,9 +1130,16 @@ struct controller_impl { }); } - bool fork_db_validated_block_exists( const block_id_type& id ) const { + bool fork_db_validated_block_exists( const block_id_type& id ) { + return fork_db.apply([&](const auto& forkdb) { + auto bsp = forkdb.get_block(id); + return bsp ? bsp->is_valid() : false; + }); + } + + bool fork_db_validated_block_exists( const block_id_type& id, uint32_t claimed_block_num ) const { return fork_db.apply([&](const auto& forkdb) { - return forkdb.validated_block_exists(id); + return forkdb.validated_block_exists(id, claimed_block_num); }); } @@ -4240,15 +4247,13 @@ struct controller_impl { if (bsp->is_recent() || testing_allow_voting) { if (use_thread_pool == use_thread_pool_t::yes && async_voting == async_t::yes) { boost::asio::post(thread_pool.get_executor(), [this, bsp=bsp]() { - const auto& latest_qc_claim__block_ref = bsp->core.get_block_reference(bsp->core.latest_qc_claim().block_num); - if (fork_db_validated_block_exists(latest_qc_claim__block_ref.block_id)) { + if (fork_db_validated_block_exists(bsp->previous(), bsp->core.latest_qc_claim().block_num)) { create_and_send_vote_msg(bsp); } }); } else { // bsp can be used directly instead of copy needed for post - const auto& latest_qc_claim__block_ref = bsp->core.get_block_reference(bsp->core.latest_qc_claim().block_num); - if (fork_db_validated_block_exists(latest_qc_claim__block_ref.block_id)) { + if (fork_db_validated_block_exists(bsp->previous(), bsp->core.latest_qc_claim().block_num)) { create_and_send_vote_msg(bsp); } } @@ -4275,6 +4280,9 @@ struct controller_impl { }; fork_db.apply(do_accept_block); + + // consider voting again as latest_qc_claim__block_ref may have been validated since the bsp was created in create_block_state_i + consider_voting(bsp, use_thread_pool_t::yes); // must be after bsp is added to fork_db } template diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index 89d8db9ebe..41d0c42961 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -99,7 +99,7 @@ namespace eosio::chain { bsp_t get_block_impl( const block_id_type& id, include_root_t include_root = include_root_t::no ) const; bool block_exists_impl( const block_id_type& id ) const; - bool validated_block_exists_impl( const block_id_type& id ) const; + bool validated_block_exists_impl( const block_id_type& id, uint32_t block_num ) const; void reset_root_impl( const bsp_t& root_bs ); void advance_root_impl( const block_id_type& id ); void remove_impl( const block_id_type& id ); @@ -603,15 +603,24 @@ namespace eosio::chain { } template - bool fork_database_t::validated_block_exists(const block_id_type& id) const { + bool fork_database_t::validated_block_exists(const block_id_type& id, uint32_t block_num) const { std::lock_guard g( my->mtx ); - return my->validated_block_exists_impl(id); + return my->validated_block_exists_impl(id, block_num); } + // returns true if block `id`, or one of its ancestors >= block_num, is found in fork_db and `is_valid()` + // precondition: `id` is already in fork_db, so is a descendent of `root` + // ------------------------------------------------------------------------------------------------------ template - bool fork_database_impl::validated_block_exists_impl(const block_id_type& id) const { - auto itr = index.find( id ); - return itr != index.end() && (*itr)->is_valid(); + bool fork_database_impl::validated_block_exists_impl(const block_id_type& id, uint32_t block_num) const { + assert(root && (root->id() == id || index.find(id) != index.end())); + if (root->block_num() >= block_num) + return true; + for (auto i = index.find(id); i != index.end() && (*i)->block_num() >= block_num; i = index.find((*i)->previous())) { + if ((*i)->is_valid()) + return true; + } + return false; } // ------------------ fork_database ------------------------- diff --git a/libraries/chain/include/eosio/chain/fork_database.hpp b/libraries/chain/include/eosio/chain/fork_database.hpp index daac6f7bd3..c51060de19 100644 --- a/libraries/chain/include/eosio/chain/fork_database.hpp +++ b/libraries/chain/include/eosio/chain/fork_database.hpp @@ -51,7 +51,7 @@ namespace eosio::chain { bsp_t get_block( const block_id_type& id, include_root_t include_root = include_root_t::no ) const; bool block_exists( const block_id_type& id ) const; - bool validated_block_exists( const block_id_type& id ) const; + bool validated_block_exists( const block_id_type& id, uint32_t block_num ) const; /** * Purges any existing blocks from the fork database and resets the root block_header_state to the provided value. From 6efb76d260738db3d582da4a5e1ddd5a8a872e73 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 16 Sep 2024 16:58:08 -0400 Subject: [PATCH 02/35] Add comment. --- libraries/chain/controller.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index e7a18c6956..72db2510e9 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1137,6 +1137,9 @@ struct controller_impl { }); } + // returns true iff block `id`, or one of its ancestors >= block_num, is found in fork_db and `is_valid()` + // precondition: `id` is already in fork_db + // ------------------------------------------------------------------------------------------------------ bool fork_db_validated_block_exists( const block_id_type& id, uint32_t claimed_block_num ) const { return fork_db.apply([&](const auto& forkdb) { return forkdb.validated_block_exists(id, claimed_block_num); From 01bf3ef04a8bda44572ab7ce1bfcace1acf364ed Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 16 Sep 2024 17:00:41 -0400 Subject: [PATCH 03/35] Undo previous change --- libraries/chain/controller.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 72db2510e9..f98f17566c 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -4283,9 +4283,6 @@ struct controller_impl { }; fork_db.apply(do_accept_block); - - // consider voting again as latest_qc_claim__block_ref may have been validated since the bsp was created in create_block_state_i - consider_voting(bsp, use_thread_pool_t::yes); // must be after bsp is added to fork_db } template From c4cd685edc3fd9a604ec2d431313a91aeaaa502b Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 16 Sep 2024 18:39:51 -0400 Subject: [PATCH 04/35] Fix incorrect `root` check in `validated_block_exists_impl` --- libraries/chain/controller.cpp | 2 +- libraries/chain/fork_database.cpp | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index f98f17566c..6c1a61a910 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1130,7 +1130,7 @@ struct controller_impl { }); } - bool fork_db_validated_block_exists( const block_id_type& id ) { + bool fork_db_validated_block_exists( const block_id_type& id ) const { return fork_db.apply([&](const auto& forkdb) { auto bsp = forkdb.get_block(id); return bsp ? bsp->is_valid() : false; diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index 41d0c42961..874be2c8e3 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -614,12 +614,15 @@ namespace eosio::chain { template bool fork_database_impl::validated_block_exists_impl(const block_id_type& id, uint32_t block_num) const { assert(root && (root->id() == id || index.find(id) != index.end())); - if (root->block_num() >= block_num) - return true; + + bsp_t last; for (auto i = index.find(id); i != index.end() && (*i)->block_num() >= block_num; i = index.find((*i)->previous())) { if ((*i)->is_valid()) return true; + last = *i; } + if (last && last->block_num() > block_num && last->previous() == root->id() && root->block_num() >= block_num) + return true; return false; } From 3c417b1f768d390299500a9033f200a3e8ecffaf Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 16 Sep 2024 18:56:35 -0400 Subject: [PATCH 05/35] Make `is_valid()` thread-safe for `block_state_legacy`. --- libraries/chain/include/eosio/chain/block_state_legacy.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/chain/include/eosio/chain/block_state_legacy.hpp b/libraries/chain/include/eosio/chain/block_state_legacy.hpp index 72977f864a..227f5ebc42 100644 --- a/libraries/chain/include/eosio/chain/block_state_legacy.hpp +++ b/libraries/chain/include/eosio/chain/block_state_legacy.hpp @@ -62,8 +62,8 @@ namespace eosio::chain { friend struct block_state; // not thread safe, expected to only be called from main thread - void set_valid(bool v) { validated = v; } - bool is_valid() const { return validated; } + void set_valid(bool v) { validated.store(v); } + bool is_valid() const { return validated.load(); } bool is_pub_keys_recovered()const { return _pub_keys_recovered; } @@ -78,7 +78,7 @@ namespace eosio::chain { _cached_trxs = std::move( trxs_metas ); } - bool validated = false; + copyable_atomic validated{false}; bool _pub_keys_recovered = false; /// this data is redundant with the data stored in block, but facilitates From bc9a5d888fde6617e1aaf96bb6e5e86111edaae8 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 16 Sep 2024 19:30:31 -0400 Subject: [PATCH 06/35] Update `validated_block_exists_impl` to take two `id`s as suggested. --- libraries/chain/controller.cpp | 10 +++++---- libraries/chain/fork_database.cpp | 21 +++++++++---------- .../include/eosio/chain/fork_database.hpp | 2 +- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 6c1a61a910..772f004e18 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1140,9 +1140,9 @@ struct controller_impl { // returns true iff block `id`, or one of its ancestors >= block_num, is found in fork_db and `is_valid()` // precondition: `id` is already in fork_db // ------------------------------------------------------------------------------------------------------ - bool fork_db_validated_block_exists( const block_id_type& id, uint32_t claimed_block_num ) const { + bool fork_db_validated_block_exists( const block_id_type& id, const block_id_type& claimed_id ) const { return fork_db.apply([&](const auto& forkdb) { - return forkdb.validated_block_exists(id, claimed_block_num); + return forkdb.validated_block_exists(id, claimed_id); }); } @@ -4250,13 +4250,15 @@ struct controller_impl { if (bsp->is_recent() || testing_allow_voting) { if (use_thread_pool == use_thread_pool_t::yes && async_voting == async_t::yes) { boost::asio::post(thread_pool.get_executor(), [this, bsp=bsp]() { - if (fork_db_validated_block_exists(bsp->previous(), bsp->core.latest_qc_claim().block_num)) { + const auto& latest_qc_claim__block_ref = bsp->core.get_block_reference(bsp->core.latest_qc_claim().block_num); + if (fork_db_validated_block_exists(bsp->previous(), latest_qc_claim__block_ref.block_id)) { create_and_send_vote_msg(bsp); } }); } else { // bsp can be used directly instead of copy needed for post - if (fork_db_validated_block_exists(bsp->previous(), bsp->core.latest_qc_claim().block_num)) { + const auto& latest_qc_claim__block_ref = bsp->core.get_block_reference(bsp->core.latest_qc_claim().block_num); + if (fork_db_validated_block_exists(bsp->previous(), latest_qc_claim__block_ref.block_id)) { create_and_send_vote_msg(bsp); } } diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index 874be2c8e3..dd24dfb168 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -99,7 +99,7 @@ namespace eosio::chain { bsp_t get_block_impl( const block_id_type& id, include_root_t include_root = include_root_t::no ) const; bool block_exists_impl( const block_id_type& id ) const; - bool validated_block_exists_impl( const block_id_type& id, uint32_t block_num ) const; + bool validated_block_exists_impl( const block_id_type& id, const block_id_type& claimed_id ) const; void reset_root_impl( const bsp_t& root_bs ); void advance_root_impl( const block_id_type& id ); void remove_impl( const block_id_type& id ); @@ -603,27 +603,26 @@ namespace eosio::chain { } template - bool fork_database_t::validated_block_exists(const block_id_type& id, uint32_t block_num) const { + bool fork_database_t::validated_block_exists(const block_id_type& id, const block_id_type& claimed_id) const { std::lock_guard g( my->mtx ); - return my->validated_block_exists_impl(id, block_num); + return my->validated_block_exists_impl(id, claimed_id); } - // returns true if block `id`, or one of its ancestors >= block_num, is found in fork_db and `is_valid()` + // returns true if block `id`, or one of its ancestors not older than claimed_id, is found in fork_db + // and `is_valid()` // precondition: `id` is already in fork_db, so is a descendent of `root` // ------------------------------------------------------------------------------------------------------ template - bool fork_database_impl::validated_block_exists_impl(const block_id_type& id, uint32_t block_num) const { + bool fork_database_impl::validated_block_exists_impl(const block_id_type& id, const block_id_type& claimed_id) const { assert(root && (root->id() == id || index.find(id) != index.end())); - bsp_t last; - for (auto i = index.find(id); i != index.end() && (*i)->block_num() >= block_num; i = index.find((*i)->previous())) { + for (auto i = index.find(id); i != index.end(); i = index.find((*i)->previous())) { if ((*i)->is_valid()) return true; - last = *i; + if ((*i)->id() == claimed_id) + return false; } - if (last && last->block_num() > block_num && last->previous() == root->id() && root->block_num() >= block_num) - return true; - return false; + return true; } // ------------------ fork_database ------------------------- diff --git a/libraries/chain/include/eosio/chain/fork_database.hpp b/libraries/chain/include/eosio/chain/fork_database.hpp index c51060de19..6d9de2754a 100644 --- a/libraries/chain/include/eosio/chain/fork_database.hpp +++ b/libraries/chain/include/eosio/chain/fork_database.hpp @@ -51,7 +51,7 @@ namespace eosio::chain { bsp_t get_block( const block_id_type& id, include_root_t include_root = include_root_t::no ) const; bool block_exists( const block_id_type& id ) const; - bool validated_block_exists( const block_id_type& id, uint32_t block_num ) const; + bool validated_block_exists( const block_id_type& id, const block_id_type& claimed_id ) const; /** * Purges any existing blocks from the fork database and resets the root block_header_state to the provided value. From 54f626f9660596bd96a37cf494db5d3564e78ce1 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 16 Sep 2024 19:32:32 -0400 Subject: [PATCH 07/35] Update comment --- libraries/chain/controller.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index 772f004e18..f8c8f1ab67 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1137,8 +1137,9 @@ struct controller_impl { }); } - // returns true iff block `id`, or one of its ancestors >= block_num, is found in fork_db and `is_valid()` - // precondition: `id` is already in fork_db + // returns true if block `id`, or one of its ancestors not older than claimed_id, is found in fork_db + // and `is_valid()` + // precondition: `id` is already in fork_db, so is a descendent of `root` // ------------------------------------------------------------------------------------------------------ bool fork_db_validated_block_exists( const block_id_type& id, const block_id_type& claimed_id ) const { return fork_db.apply([&](const auto& forkdb) { From 3a133d244e24c14f55b3b5ebdf4d936cabaf3b91 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Tue, 17 Sep 2024 08:13:36 -0400 Subject: [PATCH 08/35] Fix issue in `validated_block_exists_impl` when `id` is not present (assert not always true) --- libraries/chain/controller.cpp | 1 - libraries/chain/fork_database.cpp | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index f8c8f1ab67..f5f638e904 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1139,7 +1139,6 @@ struct controller_impl { // returns true if block `id`, or one of its ancestors not older than claimed_id, is found in fork_db // and `is_valid()` - // precondition: `id` is already in fork_db, so is a descendent of `root` // ------------------------------------------------------------------------------------------------------ bool fork_db_validated_block_exists( const block_id_type& id, const block_id_type& claimed_id ) const { return fork_db.apply([&](const auto& forkdb) { diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index dd24dfb168..fba8af97bb 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -610,19 +610,19 @@ namespace eosio::chain { // returns true if block `id`, or one of its ancestors not older than claimed_id, is found in fork_db // and `is_valid()` - // precondition: `id` is already in fork_db, so is a descendent of `root` // ------------------------------------------------------------------------------------------------------ template bool fork_database_impl::validated_block_exists_impl(const block_id_type& id, const block_id_type& claimed_id) const { - assert(root && (root->id() == id || index.find(id) != index.end())); + bool id_present = false; for (auto i = index.find(id); i != index.end(); i = index.find((*i)->previous())) { + id_present = true; if ((*i)->is_valid()) return true; if ((*i)->id() == claimed_id) return false; } - return true; + return id_present; } // ------------------ fork_database ------------------------- From 311c94dfb0f29d32168111dd88f97918ff760b02 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Tue, 17 Sep 2024 13:46:39 -0400 Subject: [PATCH 09/35] Make `is_valid` and `set_valid` public and remove two `friend` declarations. --- .../chain/include/eosio/chain/block_state_legacy.hpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/libraries/chain/include/eosio/chain/block_state_legacy.hpp b/libraries/chain/include/eosio/chain/block_state_legacy.hpp index 227f5ebc42..4484db593e 100644 --- a/libraries/chain/include/eosio/chain/block_state_legacy.hpp +++ b/libraries/chain/include/eosio/chain/block_state_legacy.hpp @@ -51,20 +51,17 @@ namespace eosio::chain { const producer_authority_schedule* pending_schedule_auth() const { return &block_header_state_legacy::pending_schedule.schedule; } const deque& trxs_metas() const { return _cached_trxs; } - + void set_valid(bool v) { validated.store(v); } + bool is_valid() const { return validated.load(); } + using fork_db_block_state_accessor_t = block_state_legacy_accessor; + private: // internal use only, not thread safe - friend struct block_state_legacy_accessor; friend struct fc::reflector; friend struct controller_impl; - template friend struct fork_database_impl; friend struct completed_block; friend struct block_state; - // not thread safe, expected to only be called from main thread - void set_valid(bool v) { validated.store(v); } - bool is_valid() const { return validated.load(); } - bool is_pub_keys_recovered()const { return _pub_keys_recovered; } deque extract_trxs_metas() { From a471e9596bd135510298665e3ad4467876be3f06 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Tue, 17 Sep 2024 13:54:43 -0400 Subject: [PATCH 10/35] Make `set_valid()` and `is_valid()` public for `block_state` as well. --- libraries/chain/include/eosio/chain/block_state.hpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libraries/chain/include/eosio/chain/block_state.hpp b/libraries/chain/include/eosio/chain/block_state.hpp index d00f037bce..21e825bf1a 100644 --- a/libraries/chain/include/eosio/chain/block_state.hpp +++ b/libraries/chain/include/eosio/chain/block_state.hpp @@ -87,8 +87,6 @@ struct block_state : public block_header_state { // block_header_state provi std::optional base_digest; // For finality_data sent to SHiP, computed on demand in get_finality_data() // ------ private methods ----------------------------------------------------------- - void set_valid(bool v) { validated.store(v); } - bool is_valid() const { return validated.load(); } bool is_pub_keys_recovered() const { return pub_keys_recovered; } deque extract_trxs_metas(); void set_trxs_metas(deque&& trxs_metas, bool keys_recovered); @@ -97,9 +95,9 @@ struct block_state : public block_header_state { // block_header_state provi friend struct test_block_state_accessor; friend struct fc::reflector; friend struct controller_impl; - template friend struct fork_database_impl; friend struct completed_block; friend struct building_block; + public: // ------ functions ----------------------------------------------------------------- const block_id_type& id() const { return block_header_state::id(); } @@ -115,6 +113,9 @@ struct block_state : public block_header_state { // block_header_state provi uint32_t latest_qc_block_num() const { return core.latest_qc_claim().block_num; } block_timestamp_type latest_qc_block_timestamp() const { return core.latest_qc_block_timestamp(); } + void set_valid(bool v) { validated.store(v); } + bool is_valid() const { return validated.load(); } + std::optional get_best_qc() const { return aggregating_qc.get_best_qc(block_num()); } // thread safe bool received_qc_is_strong() const { return aggregating_qc.received_qc_is_strong(); } // thread safe // return true if better qc, thread safe From 8f3610844dfa957f4c8eda31b5c44dfdcf459937 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Tue, 17 Sep 2024 14:11:38 -0400 Subject: [PATCH 11/35] Add unit test for `validated_block_exists` --- unittests/fork_db_tests.cpp | 103 ++++++++++++++++++++++++++---------- 1 file changed, 74 insertions(+), 29 deletions(-) diff --git a/unittests/fork_db_tests.cpp b/unittests/fork_db_tests.cpp index 99851bd667..3344d87f34 100644 --- a/unittests/fork_db_tests.cpp +++ b/unittests/fork_db_tests.cpp @@ -54,33 +54,8 @@ struct test_block_state_accessor { using namespace eosio::chain; -BOOST_AUTO_TEST_SUITE(fork_database_tests) - -BOOST_AUTO_TEST_CASE(add_remove_test) try { - fork_database_if_t forkdb; - - // Setup fork database with blocks based on a root of block 10 - // Add a number of forks in the fork database - auto root = test_block_state_accessor::make_genesis_block_state(); - auto bsp11a = test_block_state_accessor::make_unique_block_state(11, root); - auto bsp12a = test_block_state_accessor::make_unique_block_state(12, bsp11a); - auto bsp13a = test_block_state_accessor::make_unique_block_state(13, bsp12a); - auto bsp11b = test_block_state_accessor::make_unique_block_state(11, root); - auto bsp12b = test_block_state_accessor::make_unique_block_state(12, bsp11b); - auto bsp13b = test_block_state_accessor::make_unique_block_state(13, bsp12b); - auto bsp14b = test_block_state_accessor::make_unique_block_state(14, bsp13b); - auto bsp12bb = test_block_state_accessor::make_unique_block_state(12, bsp11b); - auto bsp13bb = test_block_state_accessor::make_unique_block_state(13, bsp12bb); - auto bsp13bbb = test_block_state_accessor::make_unique_block_state(13, bsp12bb); - auto bsp12bbb = test_block_state_accessor::make_unique_block_state(12, bsp11b); - auto bsp11c = test_block_state_accessor::make_unique_block_state(11, root); - auto bsp12c = test_block_state_accessor::make_unique_block_state(12, bsp11c); - auto bsp13c = test_block_state_accessor::make_unique_block_state(13, bsp12c); - - // keep track of all those added for easy verification - std::vector all { bsp11a, bsp12a, bsp13a, bsp11b, bsp12b, bsp12bb, bsp12bbb, bsp13b, bsp13bb, bsp13bbb, bsp14b, bsp11c, bsp12c, bsp13c }; - - auto reset = [&]() { +struct generate_forkdb_state { + generate_forkdb_state() { forkdb.reset_root(root); forkdb.add(bsp11a, ignore_duplicate_t::no); forkdb.add(bsp11b, ignore_duplicate_t::no); @@ -96,9 +71,37 @@ BOOST_AUTO_TEST_CASE(add_remove_test) try { forkdb.add(bsp13bbb, ignore_duplicate_t::no); forkdb.add(bsp14b, ignore_duplicate_t::no); forkdb.add(bsp13c, ignore_duplicate_t::no); - }; - reset(); + } + fork_database_if_t forkdb; + + // Setup fork database with blocks based on a root of block 10 + // Add a number of forks in the fork database + 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); + block_state_ptr bsp13a = test_block_state_accessor::make_unique_block_state(13, bsp12a); + block_state_ptr bsp11b = test_block_state_accessor::make_unique_block_state(11, root); + block_state_ptr bsp12b = test_block_state_accessor::make_unique_block_state(12, bsp11b); + block_state_ptr bsp13b = test_block_state_accessor::make_unique_block_state(13, bsp12b); + block_state_ptr bsp14b = test_block_state_accessor::make_unique_block_state(14, bsp13b); + block_state_ptr bsp12bb = test_block_state_accessor::make_unique_block_state(12, bsp11b); + block_state_ptr bsp13bb = test_block_state_accessor::make_unique_block_state(13, bsp12bb); + block_state_ptr bsp13bbb = test_block_state_accessor::make_unique_block_state(13, bsp12bb); + block_state_ptr bsp12bbb = test_block_state_accessor::make_unique_block_state(12, bsp11b); + block_state_ptr bsp11c = test_block_state_accessor::make_unique_block_state(11, root); + block_state_ptr bsp12c = test_block_state_accessor::make_unique_block_state(12, bsp11c); + block_state_ptr bsp13c = test_block_state_accessor::make_unique_block_state(13, bsp12c); + + // keep track of all those added for easy verification + std::vector all{bsp11a, bsp12a, bsp13a, bsp11b, bsp12b, bsp12bb, bsp12bbb, + bsp13b, bsp13bb, bsp13bbb, bsp14b, bsp11c, bsp12c, bsp13c}; +}; + + +BOOST_AUTO_TEST_SUITE(fork_database_tests) + +BOOST_FIXTURE_TEST_CASE(add_remove_test, generate_forkdb_state) try { // test get_block for (auto& i : all) { BOOST_TEST(forkdb.get_block(i->id()) == i); @@ -137,4 +140,46 @@ BOOST_AUTO_TEST_CASE(add_remove_test) try { BOOST_TEST(branch[1] == bsp11a); } FC_LOG_AND_RETHROW(); + +// test `fork_database_t::validated_block_exists() const` member +// ------------------------------------------------------------- +BOOST_FIXTURE_TEST_CASE(validated_block_exists, generate_forkdb_state) try { + bsp12b->set_valid(true); + bsp13b->set_valid(true); + bsp14b->set_valid(true); + + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp12b->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); + + bsp14b->set_valid(false); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp12b->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); + + bsp12b->set_valid(false); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp12b->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); + + bsp13b->set_valid(false); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp12b->id())); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); + + bsp11b->set_valid(false); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp12b->id())); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); + + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), root->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), block_id_type{})); + +} FC_LOG_AND_RETHROW(); + BOOST_AUTO_TEST_SUITE_END() From 9bde458c2a7485cee30ff84990bc6053be1891df Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Tue, 17 Sep 2024 17:32:42 -0400 Subject: [PATCH 12/35] Add testcase with incorrect call. --- libraries/chain/controller.cpp | 1 + libraries/chain/fork_database.cpp | 3 ++- unittests/fork_db_tests.cpp | 5 +++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index f5f638e904..d2a1a8adc1 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1137,6 +1137,7 @@ struct controller_impl { }); } + // prerequisite: claimed_id is either id, or an ancestor of id // returns true if block `id`, or one of its ancestors not older than claimed_id, is found in fork_db // and `is_valid()` // ------------------------------------------------------------------------------------------------------ diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index fba8af97bb..82767cc268 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -608,8 +608,9 @@ namespace eosio::chain { return my->validated_block_exists_impl(id, claimed_id); } + // prerequisite: claimed_id is either id, or an ancestor of id // returns true if block `id`, or one of its ancestors not older than claimed_id, is found in fork_db - // and `is_valid()` + // and `is_valid()`. // ------------------------------------------------------------------------------------------------------ template bool fork_database_impl::validated_block_exists_impl(const block_id_type& id, const block_id_type& claimed_id) const { diff --git a/unittests/fork_db_tests.cpp b/unittests/fork_db_tests.cpp index 3344d87f34..ae2e94e431 100644 --- a/unittests/fork_db_tests.cpp +++ b/unittests/fork_db_tests.cpp @@ -147,12 +147,17 @@ BOOST_FIXTURE_TEST_CASE(validated_block_exists, generate_forkdb_state) try { bsp12b->set_valid(true); bsp13b->set_valid(true); bsp14b->set_valid(true); + bsp11a->set_valid(true); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp12b->id())); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); + // incorrect call as bsp13a is not an ancestor of bsp14b. As a result `claimed_id` is + // not found, so we assume it is either the root or an older block which is `valid`. + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp13a->id())); + bsp14b->set_valid(false); BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); From 45e67aa1097e653ad329e466aa9e651b52cedf38 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Wed, 18 Sep 2024 08:40:23 -0400 Subject: [PATCH 13/35] Update test to reflect that if a block is valid in fork_db, its ancestors are as well. --- unittests/fork_db_tests.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/unittests/fork_db_tests.cpp b/unittests/fork_db_tests.cpp index ae2e94e431..cfc504dadc 100644 --- a/unittests/fork_db_tests.cpp +++ b/unittests/fork_db_tests.cpp @@ -144,10 +144,15 @@ BOOST_FIXTURE_TEST_CASE(add_remove_test, generate_forkdb_state) try { // test `fork_database_t::validated_block_exists() const` member // ------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE(validated_block_exists, generate_forkdb_state) try { + + // if a block is valid in fork_db, all its ancestors are necessarily valid. + root->set_valid(true); + bsp11b->set_valid(true); bsp12b->set_valid(true); bsp13b->set_valid(true); bsp14b->set_valid(true); - bsp11a->set_valid(true); + + bsp13a->set_valid(false); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); @@ -164,17 +169,17 @@ BOOST_FIXTURE_TEST_CASE(validated_block_exists, generate_forkdb_state) try { BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp12b->id())); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); - bsp12b->set_valid(false); + bsp13b->set_valid(false); BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); - BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); + BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp12b->id())); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); - bsp13b->set_valid(false); + bsp12b->set_valid(false); BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp12b->id())); - BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); + BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); bsp11b->set_valid(false); BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); From 2cacd2658cab16b4e1c6231f18636c32227011e1 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Wed, 18 Sep 2024 10:02:28 -0400 Subject: [PATCH 14/35] Validate precondition in case we return true (`claimed_id` cannot be in another branch) --- libraries/chain/controller.cpp | 2 +- libraries/chain/fork_database.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/libraries/chain/controller.cpp b/libraries/chain/controller.cpp index d2a1a8adc1..348a580949 100644 --- a/libraries/chain/controller.cpp +++ b/libraries/chain/controller.cpp @@ -1137,7 +1137,7 @@ struct controller_impl { }); } - // prerequisite: claimed_id is either id, or an ancestor of id + // precondition: claimed_id is either id, or an ancestor of id // returns true if block `id`, or one of its ancestors not older than claimed_id, is found in fork_db // and `is_valid()` // ------------------------------------------------------------------------------------------------------ diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index 82767cc268..e52603e055 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -608,7 +608,7 @@ namespace eosio::chain { return my->validated_block_exists_impl(id, claimed_id); } - // prerequisite: claimed_id is either id, or an ancestor of id + // precondition: claimed_id is either id, or an ancestor of id // returns true if block `id`, or one of its ancestors not older than claimed_id, is found in fork_db // and `is_valid()`. // ------------------------------------------------------------------------------------------------------ @@ -623,6 +623,7 @@ namespace eosio::chain { if ((*i)->id() == claimed_id) return false; } + assert(!id_present || block_header::num_from_id(claimed_id) <= block_header::num_from_id(root->id())); // precondition return id_present; } From 15ecf695af05909e82383b1de5c80680a1e27b3e Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Wed, 18 Sep 2024 10:06:01 -0400 Subject: [PATCH 15/35] Add comment --- libraries/chain/fork_database.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index e52603e055..b6ac3f28d2 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -623,7 +623,9 @@ namespace eosio::chain { if ((*i)->id() == claimed_id) return false; } - assert(!id_present || block_header::num_from_id(claimed_id) <= block_header::num_from_id(root->id())); // precondition + + // if we return `true`, let's validate the precondition and make sure claimed_id is not in another branch + assert(!id_present || block_header::num_from_id(claimed_id) <= block_header::num_from_id(root->id())); return id_present; } From 9074babddc656daf37af1fefab803d61f24308fb Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Wed, 18 Sep 2024 10:40:27 -0400 Subject: [PATCH 16/35] Remove unnecessary check. --- unittests/fork_db_tests.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/unittests/fork_db_tests.cpp b/unittests/fork_db_tests.cpp index cfc504dadc..410fe3c63d 100644 --- a/unittests/fork_db_tests.cpp +++ b/unittests/fork_db_tests.cpp @@ -159,10 +159,6 @@ BOOST_FIXTURE_TEST_CASE(validated_block_exists, generate_forkdb_state) try { BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp12b->id())); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp11b->id())); - // incorrect call as bsp13a is not an ancestor of bsp14b. As a result `claimed_id` is - // not found, so we assume it is either the root or an older block which is `valid`. - BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp13a->id())); - bsp14b->set_valid(false); BOOST_REQUIRE_EQUAL(false, forkdb.validated_block_exists(bsp14b->id(), bsp14b->id())); BOOST_REQUIRE_EQUAL(true, forkdb.validated_block_exists(bsp14b->id(), bsp13b->id())); From d2b93dfc45a84f8062cf3d2bf92d555787c701dd Mon Sep 17 00:00:00 2001 From: Matt Witherspoon <32485495+spoonincode@users.noreply.github.com> Date: Thu, 19 Sep 2024 16:11:33 -0400 Subject: [PATCH 17/35] remove 'GS register is not set as expected' warning --- libraries/chain/webassembly/runtimes/eos-vm-oc/executor.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/libraries/chain/webassembly/runtimes/eos-vm-oc/executor.cpp b/libraries/chain/webassembly/runtimes/eos-vm-oc/executor.cpp index fe33ee3d0c..749d752e76 100644 --- a/libraries/chain/webassembly/runtimes/eos-vm-oc/executor.cpp +++ b/libraries/chain/webassembly/runtimes/eos-vm-oc/executor.cpp @@ -139,10 +139,6 @@ executor::executor(const code_cache_base& cc) { //if we're the first executor created, go setup the signal handling. For now we'll just leave this attached forever static executor_signal_init the_executor_signal_init; - uint64_t current_gs; - if(arch_prctl(ARCH_GET_GS, ¤t_gs) || current_gs) - wlog("x86_64 GS register is not set as expected. EOS VM OC may not run correctly on this platform"); - struct stat s; FC_ASSERT(fstat(cc.fd(), &s) == 0, "executor failed to get code cache size"); code_mapping = (uint8_t*)mmap(nullptr, s.st_size, PROT_EXEC|PROT_READ, MAP_SHARED, cc.fd(), 0); From 241b8df24f33aa7c6434663efd1bb32d55ece434 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Wed, 25 Sep 2024 17:41:09 -0400 Subject: [PATCH 18/35] Support setting partitions temporarily using `fc::scoped_set_value` --- unittests/savanna_cluster.hpp | 54 +++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 19 deletions(-) diff --git a/unittests/savanna_cluster.hpp b/unittests/savanna_cluster.hpp index 8c867cbe4c..7e87c3deb3 100644 --- a/unittests/savanna_cluster.hpp +++ b/unittests/savanna_cluster.hpp @@ -125,6 +125,8 @@ namespace savanna_cluster { node_t(node_t&&) = default; + size_t node_idx() const { return _node_idx; } + bool& propagate_votes() { return _propagate_votes; } size_t& vote_delay() { return _vote_delay; } @@ -356,6 +358,8 @@ namespace savanna_cluster { // -------------------------------------------------------------------------------------- class cluster_t { public: + using peers_t = boost::unordered_flat_map>; + explicit cluster_t(cluster_config cfg = {}) : _fin_keys(cfg.num_nodes * 2, cfg.num_nodes) // allow for some spare heys , _num_nodes(cfg.num_nodes) @@ -403,23 +407,8 @@ namespace savanna_cluster { // for nodes (`complement`) form another partition // (within each partition, nodes are fully connected) // ----------------------------------------------------------------------------------------- - void set_partitions(std::initializer_list> l) { - auto inside = [&](size_t node_idx) { - return ranges::any_of(l, [node_idx](const auto& v) { - return ranges::any_of(v, [node_idx](auto i) { return i == node_idx; }); }); - }; - - std::vector complement; - for (size_t i = 0; i < _nodes.size(); ++i) - if (!inside(i)) - complement.push_back(i); - - auto set_peers = [&](const std::vector& v) { for (auto i : v) _peers[i] = v; }; - - _peers.clear(); - for (const auto& v : l) - set_peers(v); - set_peers(complement); + void set_partitions(const std::initializer_list>& l) { + _peers = compute_peers(l); } // this is a convenience function for the most common case where we want to partition @@ -431,6 +420,12 @@ namespace savanna_cluster { set_partitions({indices}); } + peers_t partition(const std::vector& nodes) { + auto view = nodes | std::views::transform([](const node_t* n) { return n->node_idx(); }); + std::vector indices(view.begin(), view.end()); + return compute_peers({indices}); + } + // After creating forks on different nodes on a partitioned network, make sure that, // within any partition, all chain heads of any node are also pushed to all other nodes. // @@ -545,6 +540,8 @@ namespace savanna_cluster { return {}; } + peers_t& peers() { return _peers; } + // debugging utilities // ------------------- void print(const char* name, const signed_block_ptr& b) const { @@ -563,8 +560,6 @@ namespace savanna_cluster { fc::milliseconds(eosio::chain::config::block_interval_ms); private: - using peers_t = boost::unordered_flat_map>; - peers_t _peers; size_t _num_nodes; bool _shutting_down {false}; @@ -572,6 +567,27 @@ namespace savanna_cluster { friend node_t; + peers_t compute_peers(const std::initializer_list>& l) const { + auto inside = [&](size_t node_idx) { + return ranges::any_of(l, [node_idx](const auto& v) { + return ranges::any_of(v, [node_idx](auto i) { return i == node_idx; }); }); + }; + + std::vector complement; + for (size_t i = 0; i < _nodes.size(); ++i) + if (!inside(i)) + complement.push_back(i); + + peers_t peers; + + auto set_peers = [&](const std::vector& v) { for (auto i : v) peers[i] = v; }; + + for (const auto& v : l) + set_peers(v); + set_peers(complement); + return peers; + } + void dispatch_vote_to_peers(size_t node_idx, skip_self_t skip_self, const vote_message_ptr& msg) { for_each_peer(node_idx, skip_self, [&](node_t& n) { if (n.is_open()) From 11ceb73584a72b7353408a6f65f5a4b3f52728b2 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Thu, 26 Sep 2024 11:38:42 -0400 Subject: [PATCH 19/35] Update `set_partition` to take list of nodes instead of indices. --- unittests/forked_tests_savanna.cpp | 27 ++-- unittests/savanna_cluster.hpp | 38 +++--- unittests/savanna_cluster_tests.cpp | 5 +- unittests/savanna_disaster_recovery_tests.cpp | 8 +- unittests/savanna_misc_tests.cpp | 125 +++++++++++++++--- unittests/savanna_proposer_policy_tests.cpp | 17 +-- unittests/savanna_transition_tests.cpp | 16 +-- 7 files changed, 157 insertions(+), 79 deletions(-) diff --git a/unittests/forked_tests_savanna.cpp b/unittests/forked_tests_savanna.cpp index d36a927ed9..9600517241 100644 --- a/unittests/forked_tests_savanna.cpp +++ b/unittests/forked_tests_savanna.cpp @@ -70,6 +70,7 @@ BOOST_AUTO_TEST_SUITE(forked_tests_savanna) // - produce blocks and verify that finality still advances. // --------------------------------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE(fork_with_bad_block_savanna, savanna_cluster::cluster_t) try { + auto& C=_nodes[2]; auto& D=_nodes[3]; struct fork_tracker { vector blocks; }; @@ -86,8 +87,8 @@ BOOST_FIXTURE_TEST_CASE(fork_with_bad_block_savanna, savanna_cluster::cluster_t) // split the network. Finality will stop advancing as // votes and blocks are not propagated. - const std::vector partition {2, 3}; - set_partition(partition); // simulate 2 disconnected partitions: nodes {0, 1} and nodes {2, 3} + + set_partition( {&C, &D} ); // simulate 2 disconnected partitions: nodes {0, 1} and nodes {2, 3} // at this point, each node has a QC to include into // the next block it produces which will advance lib. @@ -204,6 +205,7 @@ BOOST_FIXTURE_TEST_CASE(fork_with_bad_block_savanna, savanna_cluster::cluster_t) // - unsplit the network, produce blocks on _nodes[0] and verify lib advances. // ----------------------------------------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE( forking_savanna, savanna_cluster::cluster_t ) try { + auto& C=_nodes[2]; auto& D=_nodes[3]; _nodes[0].produce_blocks(2); // produce two extra blocks at the beginning so that producer schedules align const vector producers { "dan"_n, "sam"_n, "pam"_n }; @@ -213,8 +215,7 @@ BOOST_FIXTURE_TEST_CASE( forking_savanna, savanna_cluster::cluster_t ) try { auto sb = _nodes[0].produce_block(); BOOST_REQUIRE_EQUAL(sb->producer, producers[prod]); // first block produced by producers[prod] - const std::vector partition {2, 3}; - set_partition(partition); // simulate 2 disconnected partitions: nodes {0, 1} and nodes {2, 3} + set_partition( {&C, &D} ); // simulate 2 disconnected partitions: nodes {0, 1} and nodes {2, 3} // at this point, each node has a QC to include into // the next block it produces which will advance lib. @@ -291,6 +292,7 @@ BOOST_FIXTURE_TEST_CASE( forking_savanna, savanna_cluster::cluster_t ) try { // - Unpartition the network, veerify lib advances. // ---------------------------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE( verify_savanna_fork_choice, savanna_cluster::cluster_t) try { + auto& A=_nodes[0]; const vector producers { "dan"_n, "sam"_n, "pam"_n }; _nodes[0].create_accounts(producers); auto prod = _nodes[0].set_producers(producers); // set new producers and produce blocks until the switch is pending @@ -300,8 +302,7 @@ BOOST_FIXTURE_TEST_CASE( verify_savanna_fork_choice, savanna_cluster::cluster_t) BOOST_REQUIRE_EQUAL(sb_common->producer, producers[prod]); // first block produced by producers[prod] - const std::vector partition {1, 2, 3}; - set_partition(partition); // simulate 2 disconnected partitions: + set_partition( {&A} ); // simulate 2 disconnected partitions: // P0 (node {0}) and P1 (nodes {1, 2, 3}). // At this point, each node has a QC to include into // the next block it produces which will advance lib by one) @@ -396,6 +397,7 @@ BOOST_FIXTURE_TEST_CASE( irreversible_mode_savanna_1, savanna_cluster::cluster_t // advances past the fork block. // --------------------------------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE( irreversible_mode_savanna_2, savanna_cluster::cluster_t ) try { + auto& D=_nodes[3]; const vector producers {"producer1"_n, "producer2"_n}; _nodes[0].create_accounts(producers); _nodes[0].set_producers(producers); // set new producers and produce blocks until the switch is pending @@ -408,8 +410,7 @@ BOOST_FIXTURE_TEST_CASE( irreversible_mode_savanna_2, savanna_cluster::cluster_t // partition node3. lib will not advance on node3 anymore, but will advance on the other 3 nodes - const std::vector partition {3}; - set_partition(partition); // simulate 2 disconnected partitions: nodes {0, 1, 2} and node {3} + set_partition( {&D} ); // simulate 2 disconnected partitions: nodes {0, 1, 2} and node {3} // produce blocks on _nodes[3], creating account "bob"_n. Finality will not advance // -------------------------------------------------------------------------------- @@ -492,6 +493,7 @@ BOOST_FIXTURE_TEST_CASE( irreversible_mode_savanna_2, savanna_cluster::cluster_t // - and restart producing on P1, check that finality advances again // --------------------------------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE( split_and_rejoin, savanna_cluster::cluster_t ) try { + auto& C=_nodes[2]; auto& D=_nodes[3]; const vector producers { "p1"_n, "p2"_n, "p3"_n }; _nodes[0].create_accounts(producers); _nodes[0].set_producers(producers); // set new producers and produce blocks until the switch is pending @@ -501,8 +503,7 @@ BOOST_FIXTURE_TEST_CASE( split_and_rejoin, savanna_cluster::cluster_t ) try { dlog("lib0 = ${lib0}", ("lib0", lib0)); // 45 // split the network - const std::vector partition {2, 3}; - set_partition(partition); // simulate 2 disconnected partitions: nodes {0, 1} and node {2, 3} + set_partition( {&C, &D} ); // simulate 2 disconnected partitions: nodes {0, 1} and node {2, 3} // produce 12 blocks on _nodes[0]'s partition _nodes[0].create_accounts( {"bob"_n} ); @@ -522,7 +523,7 @@ BOOST_FIXTURE_TEST_CASE( split_and_rejoin, savanna_cluster::cluster_t ) try { // update the network split so that {0, 1, 2} are in one partition, enough for finality to start // advancing again - set_partition(std::vector{3}); // simulate 2 disconnected partitions: nodes {0, 1, 2} and node {3} + set_partition( {&D} ); // simulate 2 disconnected partitions: nodes {0, 1, 2} and node {3} propagate_heads(); // otherwise we get unlinkable_block when newly produced blocks are pushed to node2 @@ -539,6 +540,7 @@ BOOST_FIXTURE_TEST_CASE( split_and_rejoin, savanna_cluster::cluster_t ) try { // Verify that a fork switch applies the blocks, and the included transactions in order. // ------------------------------------------------------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE( push_block_returns_forked_transactions_savanna, savanna_cluster::cluster_t ) try { + auto& C=_nodes[2]; auto& D=_nodes[3]; const vector producers { "p1"_n, "p2"_n, "p3"_n }; _nodes[0].create_accounts(producers); _nodes[0].set_producers(producers); // set new producers and produce blocks until the switch is pending @@ -552,8 +554,7 @@ BOOST_FIXTURE_TEST_CASE( push_block_returns_forked_transactions_savanna, savanna signed_block_ptr cb; // split the network - const std::vector partition {2, 3}; - set_partition(partition); // simulate 2 disconnected partitions: nodes {0, 1} and node {2, 3} + set_partition( {&C, &D} ); // simulate 2 disconnected partitions: nodes {0, 1} and node {2, 3} cb = _nodes[0].produce_block(); _nodes[2].produce_block(); diff --git a/unittests/savanna_cluster.hpp b/unittests/savanna_cluster.hpp index 7e87c3deb3..4000937aae 100644 --- a/unittests/savanna_cluster.hpp +++ b/unittests/savanna_cluster.hpp @@ -400,6 +400,23 @@ namespace savanna_cluster { _shutting_down = true; } + peers_t partitions(const std::initializer_list>& l) { + std::vector> indice_vec; + for (const auto& v : l) { + auto view = v | std::views::transform([](const node_t* n) { return n->node_idx(); }); + indice_vec.emplace_back(view.begin(), view.end()); + } + return compute_peers(indice_vec); + } + + peers_t partition(const std::vector& nodes) { + return partitions({nodes}); + } + + void set_partition(const std::vector& nodes) { + _peers = partition(nodes); + } + // `set_partitions` allows to configure logical network connections between nodes. // - an empty list will connect each node of the cluster to every other nodes. // - a non-empty list partitions the network into two or more partitions. @@ -407,23 +424,8 @@ namespace savanna_cluster { // for nodes (`complement`) form another partition // (within each partition, nodes are fully connected) // ----------------------------------------------------------------------------------------- - void set_partitions(const std::initializer_list>& l) { - _peers = compute_peers(l); - } - - // this is a convenience function for the most common case where we want to partition - // the nodes into two separate disconnected partitions. - // Simply provide a set of indices for nodes that need to be logically disconnected - // from the rest of the network. - // ---------------------------------------------------------------------------------- - void set_partition(const std::vector& indices) { - set_partitions({indices}); - } - - peers_t partition(const std::vector& nodes) { - auto view = nodes | std::views::transform([](const node_t* n) { return n->node_idx(); }); - std::vector indices(view.begin(), view.end()); - return compute_peers({indices}); + void set_partitions(const std::initializer_list>& part_vec) { + _peers = partitions(part_vec); } // After creating forks on different nodes on a partitioned network, make sure that, @@ -567,7 +569,7 @@ namespace savanna_cluster { friend node_t; - peers_t compute_peers(const std::initializer_list>& l) const { + peers_t compute_peers(const std::vector>& l) const { auto inside = [&](size_t node_idx) { return ranges::any_of(l, [node_idx](const auto& v) { return ranges::any_of(v, [node_idx](auto i) { return i == node_idx; }); }); diff --git a/unittests/savanna_cluster_tests.cpp b/unittests/savanna_cluster_tests.cpp index 698e276ec0..760ea57a10 100644 --- a/unittests/savanna_cluster_tests.cpp +++ b/unittests/savanna_cluster_tests.cpp @@ -9,6 +9,8 @@ BOOST_AUTO_TEST_SUITE(savanna_cluster_tests) // test set_finalizer host function serialization and tester set_finalizers BOOST_FIXTURE_TEST_CASE(simple_test, savanna_cluster::cluster_t) { try { + auto& C=_nodes[2]; auto& D=_nodes[3]; + auto node3_lib = _nodes[3].lib_num(); // Store initial lib (after Savanna transtion) BOOST_REQUIRE_EQUAL(num_nodes(), num_lib_advancing([&]() { // Check that lib advances on all nodes _nodes[0].produce_block(); // Blocks & votes are propagated to all connected peers. @@ -35,8 +37,7 @@ BOOST_FIXTURE_TEST_CASE(simple_test, savanna_cluster::cluster_t) { try { node3_lib = _nodes[3].lib_num(); BOOST_REQUIRE_EQUAL(node0_lib, node3_lib); - const std::vector partition {2, 3}; - set_partition(partition); // Simulate 2 disconnected partitions: + set_partition( {&C, &D} ); // Simulate 2 disconnected partitions: // nodes {0, 1} and nodes {2, 3} // at this point, each node has a QC to include into the next block it produces which will advance lib. diff --git a/unittests/savanna_disaster_recovery_tests.cpp b/unittests/savanna_disaster_recovery_tests.cpp index 1ce5dc56db..becc997e40 100644 --- a/unittests/savanna_disaster_recovery_tests.cpp +++ b/unittests/savanna_disaster_recovery_tests.cpp @@ -3,7 +3,7 @@ using namespace eosio::chain; using namespace eosio::testing; -BOOST_AUTO_TEST_SUITE(savanna_disaster_recovery) +BOOST_AUTO_TEST_SUITE(savanna_disaster_recovery_tests) // ------------------------------------------------------------------------------------------ // Check that a node can go down cleanly, restart from its existing state, and start voting @@ -213,8 +213,7 @@ BOOST_FIXTURE_TEST_CASE(all_nodes_shutdown_with_reversible_blocks_lost, savanna_ // split network { A, B } and { C, D } // A produces two more blocks, so A and B will vote strong but finality will not advance // ------------------------------------------------------------------------------------- - const std::vector partition {2, 3}; - set_partition(partition); + set_partition( {&C, &D} ); BOOST_REQUIRE_EQUAL(1u, A.lib_advances_by([&]() { A.produce_blocks(2); })); // lib stalls with network partitioned, 1 QC in flight // remove network split @@ -306,8 +305,7 @@ BOOST_FIXTURE_TEST_CASE(restart_from_fork_db_with_only_root_block, savanna_clust BOOST_REQUIRE_EQUAL(2u, C.lib_advances_by([&]() { b1 = C.produce_block(); b2 = C.produce_block(); })); // Partition C by itself, so it doesn't receive b1 and b2 when opebed - const std::vector tmp_partition {2}; - set_partition(tmp_partition); + set_partition( {&C} ); C.close(); C.remove_state(); diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index fff7956d80..73c1c7263a 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -71,8 +71,7 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { // partition D out. D will be used to produce blocks on an alternative fork. // We will have 3 finalizers voting which is enough to reach QCs // ------------------------------------------------------------------------- - const std::vector partition {3}; - set_partition(partition); + set_partition( {&D} ); auto b1 = A.produce_block(); // receives strong votes from 3 finalizers (D partitioned out) print("b1", b1); @@ -82,8 +81,7 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { BOOST_REQUIRE_GT(b2->timestamp.slot, b1->timestamp.slot); - const std::vector tmp_partition {0}; // we temporarily separate A (before pushing b2) - set_partitions({tmp_partition, partition}); // because we don't want A to see the block produced by D (b2) + set_partitions({ {&A}, {&D}}); // because we don't want A to see the block produced by D (b2) // otherwise it will switch forks and build its next block (b3) // on top of it @@ -95,7 +93,7 @@ BOOST_FIXTURE_TEST_CASE(weak_masking_issue, savanna_cluster::cluster_t) try { BOOST_REQUIRE_EQUAL(qc_s(qc(b2)), strong_qc(b0)); // b2 should include a strong qc on b0 - set_partition(partition); // restore our original partition {A, B, C} and {D} + set_partition( {&D} ); // restore our original partition {A, B, C} and {D} signed_block_ptr b3; { @@ -247,14 +245,13 @@ BOOST_FIXTURE_TEST_CASE(gh_534_liveness_issue, savanna_cluster::cluster_t) try { // partition D out. D will be used to produce blocks on an alternative fork. // We will have 3 finalizers voting which is enough to reach QCs // ------------------------------------------------------------------------- - const std::vector partition {3}; - set_partition(partition); + set_partition( {&D} ); auto b3 = D.produce_block(); // produce a block on D print("b3", b3); - const std::vector tmp_partition {0}; // we temporarily separate A (before pushing b3) - set_partition(tmp_partition); // because we don't want A to see the block produced by D (b3) + // we temporarily separate A (before pushing b3) + set_partition( {&A} ); // because we don't want A to see the block produced by D (b3) // otherwise it will switch forks and build its next block (b4) // on top of it @@ -268,7 +265,7 @@ BOOST_FIXTURE_TEST_CASE(gh_534_liveness_issue, savanna_cluster::cluster_t) try { // so it didn't see b3 and its enclosed QC. B.check_fsi({.last_vote = b3, .lock = b2, .other_branch_latest_time = {}}); - set_partition(partition); // restore our original partition {A, B, C} and {D} + set_partition( {&D} ); // restore our original partition {A, B, C} and {D} // from now on, to reproduce the scenario where votes are delayed, so the QC we receive don't // claim the parent block, but an ancestor, we need to artificially delay propagating the votes. @@ -354,8 +351,7 @@ BOOST_FIXTURE_TEST_CASE(validate_qc_after_restart_from_snapshot, savanna_cluster auto b1 = A.produce_block(); // receives strong votes from all finalizers print("b1", b1); - const std::vector partition {0}; // partition A so that B, C and D don't see b2 (yet) - set_partition(partition); + set_partition( {&A} ); // partition A so that B, C and D don't see b2 (yet) auto b2 = A.produce_block(); // receives just 1 strong vote fron A print("b2", b2); @@ -394,7 +390,7 @@ BOOST_FIXTURE_TEST_CASE(validate_qc_after_restart_from_snapshot, savanna_cluster A.remove_state(); A.remove_reversible_data_and_blocks_log(); - set_partition({0}); // partition A so it doesn't receive blocks on `open()` + set_partition( {&A} ); // partition A so it doesn't receive blocks on `open()` A.open_from_snapshot(b3_snapshot); // After starting up from the snapshot, their node receives block b4 from the P2P network. @@ -505,8 +501,8 @@ BOOST_FIXTURE_TEST_CASE(validate_qc_requiring_finalizer_policies, savanna_cluste auto p2 = pending->generation; // and its generation is higher than the active one BOOST_REQUIRE_EQUAL(p2, p1 + 1); // b3 has new pending finalizer policy p2 - const std::vector partition {0}; // partition A so that B, C and D don't see b4 (yet) - set_partition(partition); // and don't vote on it + // partition A so that B, C and D don't see b4 (yet) + set_partition( {&A} ); // and don't vote on it auto b4 = A.produce_block(); print("b4", b4); @@ -581,7 +577,7 @@ BOOST_FIXTURE_TEST_CASE(validate_qc_requiring_finalizer_policies, savanna_cluste A.remove_state(); A.remove_reversible_data_and_blocks_log(); - set_partition({0}); // partition A so it doesn't receive blocks on `open()` + set_partition({&A}); // partition A so it doesn't receive blocks on `open()` A.open_from_snapshot(b6_snapshot); A.push_block(b7); // when pushing b7, if we try to access any block state @@ -632,8 +628,8 @@ BOOST_FIXTURE_TEST_CASE(verify_spring_1_0_block_compatibitity, savanna_cluster:: auto p2 = pending->generation; // and its generation is higher than the active one BOOST_REQUIRE_EQUAL(p2, p1 + 1); // b3 has new pending finalizer policy p2 - const std::vector partition {0}; // partition A so that B, C and D don't see b4 (yet) - set_partition(partition); // and don't vote on it + // partition A so that B, C and D don't see b4 (yet) + set_partition({&A}); // and don't vote on it // push action so that the block is not empty A.push_action(config::system_account_name, updateauth::get_name(), tester_account, @@ -717,7 +713,7 @@ Vote: Strong Strong Strong Weak - because `B2 timestamp < B4 timestamp`. -> safety check fails because `B6` does not extend `B4`. --------------------------------------------------------------------------------------------------------*/ -BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branch, savanna_cluster::cluster_t) try { +BOOST_FIXTURE_TEST_CASE(finalizers_locked_preventing_vote_on_alternate_branch, savanna_cluster::cluster_t) try { using namespace savanna_cluster; auto& A=_nodes[0]; auto& B=_nodes[1]; auto& C=_nodes[2]; auto& D=_nodes[3]; @@ -744,8 +740,8 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc B.propagate_delayed_votes_to(D); // propagate votes on b2 to D, so it can form a QC on b2 C.propagate_delayed_votes_to(D); // which will be included in b6 - const std::vector partition {3}; // partition D so that it doesn't see b3, b4 and b5 - set_partition(partition); // and don't vote on it + // partition D so that it doesn't see b3, b4 and b5 + set_partition({&D}); // and don't vote on it auto b3 = A.produce_block(); print("b3", b3); @@ -791,4 +787,91 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc } FC_LOG_AND_RETHROW() +/* ----------------------------------------------------------------------------------------------------- + Finality advancing past block claimed on alternate branch + ========================================================= +Producer: C C C C C D D D D +Timestamp: t1 t2 t3 t4 t5 t6 t7 t8 t9 +Blocks: + B0 <--- B1 <--- B2 <--- B3 <-|- B4 <--- B5 + | + \----------------- B6 <--- B7 <--- B8 <--- B9 +QC claim: + Strong Strong Strong Strong Strong Strong Strong Weak Strong + B0 B0 B1 B3 B4 B1 B2 B7 B8 + +Votes: + Node A: Strong‡ Strong‡ Strong‡ Strong Weak¹ Weak Strong Strong + Node B: Strong¹ Strong¹ Strong Strong Weak¹ Weak Strong Strong + Node C: Strong Strong Strong Strong Strong‡ Weak¹ Weak¹ Strong¹ Strong + Node D: Strong¹ Strong¹ Strong Strong Strong Strong Strong + + ^ + | + Validating the strong QC on B2 should + not fail for nodes which receive B4 and + B5 prior to B7 despite B5 advancing the + fork DB root to B3. + +Meaning of the superscripts and marks on the votes: +The vote on block b was delayed in reaching the node for the producer p scheduled +for the block at the next time slot t after block b by enough that a block produced on time by +producer p for time slot t could not possibly utilize the vote in any QC the block could claim. +Furthermore, the delay is such that the earliest time slot at which producer p could +produce a block that utilizes the delayed vote is the time slot (t + d) where ... +¹ ... d = 1. +‡ ... d is infinite meaning the vote may never be received by producer p. + +--------------------------------------------------------------------------------------------------------*/ +#if 0 +BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branch, savanna_cluster::cluster_t) try { + using namespace savanna_cluster; + auto& A=_nodes[0]; auto& B=_nodes[1]; auto& C=_nodes[2]; auto& D=_nodes[3]; + + _debug_mode = true; + + auto b0 = A.produce_block(); + print("b0", b0); + + signed_block_ptr b1, b2, b3, b4, b5; + + peers() = partition({ &A }); + + { + fc::scoped_set_value tmp_B(B.vote_delay(), 1); // delay votes from B for 1 slot + fc::scoped_set_value tmp_D(D.vote_delay(), 1); // delay votes from D for 1 slot + + b1 = C.produce_block(); + print("b1", b1); + BOOST_REQUIRE_EQUAL(qc_s(qc(b1)), strong_qc(b0)); // b1 claims a strong QC on b0 + + b2 = C.produce_block(); + print("b2", b2); + BOOST_REQUIRE(!qc(b2)); // b2 should not include a QC (votes on b1 delayed) + BOOST_REQUIRE_EQUAL(qc_claim(b2), qc_claim(b1)); // C didn't form a QC on b1, so b2 should repeat b1's claim + + // D doesn't receive B's vote on b2 yet because it is delayed, or A's vote because it is partitioned out + } + + peers() = partition({ &A, &D }); + + b3 = C.produce_block(); + print("b3", b3); + BOOST_REQUIRE_EQUAL(qc_s(qc(b3)), strong_qc(b1)); // b3 claims a strong QC on b1 (B and D votes delayed by 1) + + + + b4 = C.produce_block(); + print("b4", b4); + BOOST_REQUIRE_EQUAL(qc_s(qc(b4)), strong_qc(b3)); // b4 claims a strong QC on b3 (B and D votes not delayed anymore) + + + + b5 = C.produce_block(); + print("b5", b5); + BOOST_REQUIRE_EQUAL(qc_s(qc(b5)), strong_qc(b4)); // b5 claims a strong QC on b4 + +} FC_LOG_AND_RETHROW() +#endif + BOOST_AUTO_TEST_SUITE_END() diff --git a/unittests/savanna_proposer_policy_tests.cpp b/unittests/savanna_proposer_policy_tests.cpp index 3720aad199..709b58057c 100644 --- a/unittests/savanna_proposer_policy_tests.cpp +++ b/unittests/savanna_proposer_policy_tests.cpp @@ -5,7 +5,7 @@ using namespace eosio::testing; static const uint32_t prod_rep = static_cast(config::producer_repetitions); -BOOST_AUTO_TEST_SUITE(savanna_proposer_policy) +BOOST_AUTO_TEST_SUITE(savanna_proposer_policy_tests) // --------------------------------------------------------------------------------------------------- // Proposer Policy change - check expected delay when policy change initiated on @@ -128,14 +128,13 @@ BOOST_FIXTURE_TEST_CASE(policy_change_last_block_delay_check, savanna_cluster::c // Verify that a proposer policy does not become active when finality has stalled // --------------------------------------------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE(no_proposer_policy_change_without_finality, savanna_cluster::cluster_t) try { - auto& A=_nodes[0]; + auto& A=_nodes[0]; auto& C=_nodes[2]; auto& D=_nodes[3]; // split network { A, B } and { C, D } // Regardless of how many blocks A produces, finality will not advance // by more than one (1 QC in flight) // ------------------------------------------------------------------- - const std::vector partition {2, 3}; - set_partition(partition); + set_partition( {&C, &D} ); auto sb = A.produce_block(); // take care of the in-flight QC const vector producers { "pa"_n, "pb"_n }; @@ -183,14 +182,13 @@ BOOST_FIXTURE_TEST_CASE(no_proposer_policy_change_without_finality, savanna_clus // finality stalled long enough) // --------------------------------------------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE(no_proposer_policy_change_without_finality_2, savanna_cluster::cluster_t) try { - auto& A=_nodes[0]; + auto& A=_nodes[0]; auto& C=_nodes[2]; auto& D=_nodes[3]; // split network { A, B } and { C, D } // Regardless of how many blocks A produces, finality will not advance // by more than one (1 QC in flight) // ------------------------------------------------------------------- - const std::vector partition {2, 3}; - set_partition(partition); + set_partition( {&C, &D} ); auto sb = A.produce_block(); // take care of the in-flight QC const vector producers { "pa"_n, "pb"_n }; @@ -233,7 +231,7 @@ BOOST_FIXTURE_TEST_CASE(no_proposer_policy_change_without_finality_2, savanna_cl // Verify that a proposer policy becomes active when finality has advanced enough to make it pending // --------------------------------------------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE(pending_proposer_policy_becomes_active_without_finality, savanna_cluster::cluster_t) try { - auto& A=_nodes[0]; + auto& A=_nodes[0]; auto& C=_nodes[2]; auto& D=_nodes[3]; static_assert(prod_rep >= 4); auto sb = A.produce_block(); @@ -254,8 +252,7 @@ BOOST_FIXTURE_TEST_CASE(pending_proposer_policy_becomes_active_without_finality, // Regardless of how many blocks A produces, finality will not advance // by more than one (1 QC in flight) // ------------------------------------------------------------------- - const std::vector partition {2, 3}; - set_partition(partition); + set_partition( {&C, &D} ); sb = A.produce_block(); // produce one more block for lib final advance (in-flight QC) diff --git a/unittests/savanna_transition_tests.cpp b/unittests/savanna_transition_tests.cpp index f615a48089..54162c09ac 100644 --- a/unittests/savanna_transition_tests.cpp +++ b/unittests/savanna_transition_tests.cpp @@ -3,7 +3,7 @@ using namespace eosio::chain; using namespace eosio::testing; -BOOST_AUTO_TEST_SUITE(savanna_transition) +BOOST_AUTO_TEST_SUITE(savanna_transition_tests) // --------------------------------------------------------------------------------------------------- // Verify a straightforward transition, with all four nodes healthy and voting @@ -31,7 +31,7 @@ BOOST_FIXTURE_TEST_CASE(straightforward_transition, // --------------------------------------------------------------------------------------------------- BOOST_FIXTURE_TEST_CASE(transition_with_split_network_before_critical_block, savanna_cluster::pre_transition_cluster_t) try { - auto& A=_nodes[0]; + auto& A=_nodes[0]; auto& C=_nodes[2]; auto& D=_nodes[3]; // set two producers, so that we have at least one block between the genesis and critical block. // with one producer the critical block comes right after the genesis block. @@ -62,8 +62,7 @@ BOOST_FIXTURE_TEST_CASE(transition_with_split_network_before_critical_block, // partition network and produce blocks // ---------------------------------------- - const std::vector partition {2, 3}; - set_partition(partition); + set_partition( {&C, &D} ); A.produce_blocks(20); // verify that lib has stalled @@ -131,8 +130,7 @@ BOOST_FIXTURE_TEST_CASE(restart_from_snapshot_at_beginning_of_transition_while_p // partition network and produce blocks // ---------------------------------------- - const std::vector partition {2, 3}; - set_partition(partition); + set_partition( {&C, &D} ); A.produce_blocks(2); auto snapshot_C = C.snapshot(); @@ -230,8 +228,7 @@ BOOST_FIXTURE_TEST_CASE(restart_from_snapshot_at_end_of_transition_while_preserv // partition network and produce a block, which will be the first proper savanna block // ----------------------------------------------------------------------------------- - const std::vector partition {2, 3}; - set_partition(partition); + set_partition( {&C, &D} ); signed_block_ptr b = A.produce_block(); BOOST_REQUIRE(b->is_proper_svnn_block()); @@ -308,8 +305,7 @@ BOOST_FIXTURE_TEST_CASE(restart_from_snapshot_at_beginning_of_transition_with_lo // partition network and produce blocks // ---------------------------------------- - const std::vector partition {2, 3}; - set_partition(partition); + set_partition( {&C, &D} ); A.produce_blocks(2); auto snapshot_C = C.snapshot(); From c64ebf7a2a4bda866ac69e2c5205b2a2510f1841 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Thu, 26 Sep 2024 11:49:13 -0400 Subject: [PATCH 20/35] Store votes from each node so we can simulate slow propagation. --- unittests/savanna_cluster.cpp | 1 + unittests/savanna_cluster.hpp | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/unittests/savanna_cluster.cpp b/unittests/savanna_cluster.cpp index f6a59afaea..783c15a599 100644 --- a/unittests/savanna_cluster.cpp +++ b/unittests/savanna_cluster.cpp @@ -21,6 +21,7 @@ node_t::node_t(size_t node_idx, cluster_t& cluster, setup_policy policy /* = set if (status == vote_result_t::success) { vote_message_ptr vote_msg = std::get<2>(v); _last_vote = vote_t(vote_msg->block_id, vote_msg->strong); + _votes[vote_msg->block_id] = vote_msg; if (_propagate_votes) { if (_vote_delay) diff --git a/unittests/savanna_cluster.hpp b/unittests/savanna_cluster.hpp index 4000937aae..1e530fcc8a 100644 --- a/unittests/savanna_cluster.hpp +++ b/unittests/savanna_cluster.hpp @@ -104,6 +104,8 @@ namespace savanna_cluster { // ---------------------------------------------------------------------------- class node_t : public tester { private: + using votes_map_t = boost::unordered_flat_map; + size_t _node_idx; bool _pushing_a_block{false}; bool _propagate_votes{true}; // if false, votes are dropped @@ -113,6 +115,8 @@ namespace savanna_cluster { size_t _vote_delay{0}; // delay vote propagation by this much std::vector _delayed_votes; + votes_map_t _votes; // all votes from this node + std::function _accepted_block_cb; std::function _voted_block_cb; @@ -135,6 +139,8 @@ namespace savanna_cluster { const vote_t& last_vote() const { return _last_vote; } + vote_message_ptr get_vote(const block_id_type& block_id) const { return _votes.at(block_id); } + void set_node_finalizers(std::span names) { _node_finalizers = std::vector{ names.begin(), names.end() }; if (control) { From 198a35f95883bf079539b111c9eb5f26905ddbdc Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 27 Sep 2024 09:26:03 -0400 Subject: [PATCH 21/35] First part of the new test works. --- unittests/savanna_cluster.cpp | 15 +++++++++------ unittests/savanna_cluster.hpp | 20 +++++++++++++------- unittests/savanna_misc_tests.cpp | 23 +++++++++++++++++------ 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/unittests/savanna_cluster.cpp b/unittests/savanna_cluster.cpp index 783c15a599..ee85b96a28 100644 --- a/unittests/savanna_cluster.cpp +++ b/unittests/savanna_cluster.cpp @@ -29,10 +29,10 @@ node_t::node_t(size_t node_idx, cluster_t& cluster, setup_policy policy /* = set while (_delayed_votes.size() > _vote_delay) { vote_message_ptr vote = _delayed_votes.front(); _delayed_votes.erase(_delayed_votes.cbegin()); - cluster.dispatch_vote_to_peers(node_idx, skip_self_t::yes, vote); + _cluster.dispatch_vote_to_peers(node_idx, skip_self_t::yes, vote); } if (!_vote_delay) - cluster.dispatch_vote_to_peers(node_idx, skip_self_t::yes, vote_msg); + _cluster.dispatch_vote_to_peers(node_idx, skip_self_t::yes, vote_msg); } } }; @@ -42,7 +42,7 @@ node_t::node_t(size_t node_idx, cluster_t& cluster, setup_policy policy /* = set if (!_pushing_a_block) { // we want to propagate only blocks we produce, not the ones we receive from the network auto& b = std::get<0>(p); - cluster.push_block_to_peers(node_idx, skip_self_t::yes, b); + _cluster.push_block_to_peers(node_idx, skip_self_t::yes, b); } }; @@ -59,10 +59,13 @@ node_t::node_t(size_t node_idx, cluster_t& cluster, setup_policy policy /* = set node_t::~node_t() {} -void node_t::propagate_delayed_votes_to(const node_t& to) { +void node_t::propagate_delayed_votes_to(const node_t& n) { for (auto& vote : _delayed_votes) - if (to.is_open()) - to.control->process_vote_message(++_cluster._connection_id, vote); + _cluster.dispatch_vote_to(n, vote); +} + +void node_t::push_vote_to(const node_t& n, const block_id_type& block_id) { + _cluster.dispatch_vote_to(n, get_vote(block_id)); } } diff --git a/unittests/savanna_cluster.hpp b/unittests/savanna_cluster.hpp index 1e530fcc8a..eec01871df 100644 --- a/unittests/savanna_cluster.hpp +++ b/unittests/savanna_cluster.hpp @@ -135,7 +135,7 @@ namespace savanna_cluster { size_t& vote_delay() { return _vote_delay; } - void propagate_delayed_votes_to(const node_t& to); + void propagate_delayed_votes_to(const node_t& n); const vote_t& last_vote() const { return _last_vote; } @@ -231,14 +231,16 @@ namespace savanna_cluster { } template - void push_blocks_to(Node& to, uint32_t block_num_limit = std::numeric_limits::max()) const { + void push_blocks_to(Node& n, uint32_t block_num_limit = std::numeric_limits::max()) const { auto limit = std::min(fork_db_head().block_num(), block_num_limit); - while (to.fork_db_head().block_num() < limit) { - auto sb = control->fetch_block_by_number(to.fork_db_head().block_num() + 1); - to.push_block(sb); + while (n.fork_db_head().block_num() < limit) { + auto sb = control->fetch_block_by_number(n.fork_db_head().block_num() + 1); + n.push_block(sb); } } + void push_vote_to(const node_t& n, const block_id_type& block_id); + bool is_head_missing_finalizer_votes() { if (!control->get_testing_allow_voting_flag()) return false; @@ -596,10 +598,14 @@ namespace savanna_cluster { return peers; } + void dispatch_vote_to(const node_t& n, const vote_message_ptr& msg) { + if (n.is_open()) + n.control->process_vote_message(++_connection_id, msg); + } + void dispatch_vote_to_peers(size_t node_idx, skip_self_t skip_self, const vote_message_ptr& msg) { for_each_peer(node_idx, skip_self, [&](node_t& n) { - if (n.is_open()) - n.control->process_vote_message(++_connection_id, msg); + dispatch_vote_to(n, msg); }); } diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index 73c1c7263a..8c2bf62ca2 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -823,7 +823,6 @@ produce a block that utilizes the delayed vote is the time slot (t + d) where .. ‡ ... d is infinite meaning the vote may never be received by producer p. --------------------------------------------------------------------------------------------------------*/ -#if 0 BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branch, savanna_cluster::cluster_t) try { using namespace savanna_cluster; auto& A=_nodes[0]; auto& B=_nodes[1]; auto& C=_nodes[2]; auto& D=_nodes[3]; @@ -833,9 +832,9 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc auto b0 = A.produce_block(); print("b0", b0); - signed_block_ptr b1, b2, b3, b4, b5; + signed_block_ptr b1, b2, b3, b4, b5, b6; - peers() = partition({ &A }); + set_partition({ &A }); { fc::scoped_set_value tmp_B(B.vote_delay(), 1); // delay votes from B for 1 slot @@ -853,25 +852,37 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc // D doesn't receive B's vote on b2 yet because it is delayed, or A's vote because it is partitioned out } - peers() = partition({ &A, &D }); + set_partitions({{ &A }, { &D }}); // both A and D are isolated by themselves b3 = C.produce_block(); print("b3", b3); BOOST_REQUIRE_EQUAL(qc_s(qc(b3)), strong_qc(b1)); // b3 claims a strong QC on b1 (B and D votes delayed by 1) - + C.push_blocks_to(D); // we want D to receive b3 (so it can build b6 on it), but no votes + D.push_vote_to(C, b3->calculate_id()); // and we want C to get D's vote on b3 so it can form a QC + // this simulates D being isolated just after receiving b3 and voting + // on it, but before receiving B and C votes on b3. b4 = C.produce_block(); print("b4", b4); BOOST_REQUIRE_EQUAL(qc_s(qc(b4)), strong_qc(b3)); // b4 claims a strong QC on b3 (B and D votes not delayed anymore) + b6 = D.produce_block(_block_interval_us * 2); // Node D produces and broadcasts B6 one second early (due + print("b6", b6); // to clock differences). + BOOST_REQUIRE_EQUAL(b6->previous, b3->calculate_id()); // b6 has B3 as its parent block + BOOST_REQUIRE(!qc(b6)); // b6 does not include a new qc (lacking votes on b2 and b3) + BOOST_REQUIRE_EQUAL(qc_claim(b6), qc_claim(b3)); // and repeats b3's strong QC claim on B1. + + set_partition({ &D }); // B and C re-establish connection with A + C.push_blocks_to(A); // we want A to receive b3 and b4, so it can vote on them + A.push_vote_to(C, b4->calculate_id()); b5 = C.produce_block(); print("b5", b5); BOOST_REQUIRE_EQUAL(qc_s(qc(b5)), strong_qc(b4)); // b5 claims a strong QC on b4 } FC_LOG_AND_RETHROW() -#endif + BOOST_AUTO_TEST_SUITE_END() From 6b3997802218269c97d722866176a5a6d74feac3 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 27 Sep 2024 14:15:06 -0400 Subject: [PATCH 22/35] wip --- unittests/savanna_misc_tests.cpp | 39 ++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index 8c2bf62ca2..1eebccbbb9 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -822,6 +822,7 @@ produce a block that utilizes the delayed vote is the time slot (t + d) where .. ¹ ... d = 1. ‡ ... d is infinite meaning the vote may never be received by producer p. +see https://github.com/AntelopeIO/spring/issues/751 --------------------------------------------------------------------------------------------------------*/ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branch, savanna_cluster::cluster_t) try { using namespace savanna_cluster; @@ -832,7 +833,7 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc auto b0 = A.produce_block(); print("b0", b0); - signed_block_ptr b1, b2, b3, b4, b5, b6; + signed_block_ptr b1, b2, b3, b4, b5, b6, b7, b8, b9; set_partition({ &A }); @@ -852,7 +853,7 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc // D doesn't receive B's vote on b2 yet because it is delayed, or A's vote because it is partitioned out } - set_partitions({{ &A }, { &D }}); // both A and D are isolated by themselves + set_partitions({{ &A }, { &D }}); // both A and D are isolated by themselves (step 15) b3 = C.produce_block(); print("b3", b3); @@ -873,15 +874,43 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc BOOST_REQUIRE(!qc(b6)); // b6 does not include a new qc (lacking votes on b2 and b3) BOOST_REQUIRE_EQUAL(qc_claim(b6), qc_claim(b3)); // and repeats b3's strong QC claim on B1. - set_partition({ &D }); // B and C re-establish connection with A + C.push_blocks_to(A); // simulates A and D temporarily reconnecting, D sending the blocks + A.push_vote_to(D, b2->calculate_id()); // produced by C, A voting on them and D receiving these votes - C.push_blocks_to(A); // we want A to receive b3 and b4, so it can vote on them - A.push_vote_to(C, b4->calculate_id()); + set_partition({ &D }); // B and C re-establish connection with A (step 26,27) + C.push_blocks_to(A); // Now that A is reconnected to B and C, it can receive blocks and + A.push_vote_to(C, b4->calculate_id()); // vote on them + + set_partitions({ { &C }, { &D } }); // Node C is isolated from the other nodes (step 30) b5 = C.produce_block(); print("b5", b5); BOOST_REQUIRE_EQUAL(qc_s(qc(b5)), strong_qc(b4)); // b5 claims a strong QC on b4 + b7 = D.produce_block(); // Node D produces b7 + print("b7", b7); + BOOST_REQUIRE_EQUAL(b7->previous, b6->calculate_id()); // b7 has B6 as its parent block + BOOST_REQUIRE_EQUAL(qc_s(qc(b7)), strong_qc(b2)); // b7 claims a strong QC on b2 + + set_partition({}); + + A.push_block(b5); // A receives b5 + BOOST_REQUIRE_EQUAL(A.lib_number, b3->block_num()); // which advances lib to b3 + + B.push_block(b5); // B receives b5 + BOOST_REQUIRE_EQUAL(B.lib_number, b3->block_num()); // which advances lib to b3 + + // critical: Nodes A and B have received b5, which has advanced finality to b3. + // when we push b6 and b7 (produced by D) to these nodes, they will want to verify the QC included in b7 (strong QC on b2). + // If, in order to verify this QC, they attempt to lookup b2 in fork_db, this will fail because lib (and hence fork_db's root) + // has advanced to b3. + // --------------------------------------------------------------------------------------------------------------------------- + A.push_block(b6); // don't use `push_blocks_to` because of fork + A.push_block(b7); + + B.push_block(b6); + B.push_block(b7); + } FC_LOG_AND_RETHROW() From 1ed13f401e715a59c06add0f308435cf3d1e1c5c Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Fri, 27 Sep 2024 15:12:11 -0400 Subject: [PATCH 23/35] Finish test `finality_advancing_past_block_claimed_on_alternate_branch` --- unittests/savanna_cluster.hpp | 7 +++++-- unittests/savanna_misc_tests.cpp | 21 ++++++++++++++++++--- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/unittests/savanna_cluster.hpp b/unittests/savanna_cluster.hpp index eec01871df..feb4da5e7b 100644 --- a/unittests/savanna_cluster.hpp +++ b/unittests/savanna_cluster.hpp @@ -76,10 +76,13 @@ namespace savanna_cluster { struct qc_s { explicit qc_s(uint32_t block_num, bool strong) : block_num(block_num), strong(strong) {} - explicit qc_s(const std::optional& qc) : block_num(qc->block_num), strong(qc->is_strong()) {} + explicit qc_s(const std::optional& qc) : block_num(qc ? qc->block_num : uint32_t(-1)), strong(qc->is_strong()) {} friend std::ostream& operator<<(std::ostream& s, const qc_s& v) { - s << "qc_s(" << v.block_num << ", " << (v.strong ? "strong" : "weak") << ")"; + if (v.block_num == uint32_t(-1)) + s << "no_qc"; + else + s << "qc_s(" << v.block_num << ", " << (v.strong ? "strong" : "weak") << ")"; return s; } bool operator==(const qc_s&) const = default; diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index 1eebccbbb9..77d4fe55d3 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -900,16 +900,31 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc B.push_block(b5); // B receives b5 BOOST_REQUIRE_EQUAL(B.lib_number, b3->block_num()); // which advances lib to b3 - // critical: Nodes A and B have received b5, which has advanced finality to b3. + // Following requires issue #694 fix: + // Nodes A and B have received b5, which has advanced finality to b3. // when we push b6 and b7 (produced by D) to these nodes, they will want to verify the QC included in b7 (strong QC on b2). // If, in order to verify this QC, they attempt to lookup b2 in fork_db, this will fail because lib (and hence fork_db's root) // has advanced to b3. // --------------------------------------------------------------------------------------------------------------------------- A.push_block(b6); // don't use `push_blocks_to` because of fork - A.push_block(b7); + A.push_block(b7); // prior to PR #719 (fixing issue #694), we'd have an exception here B.push_block(b6); - B.push_block(b7); + B.push_block(b7); // prior to PR #719 (fixing issue #694), we'd have an exception here + + // with issue #694 fixed, A and B were able to successfully validate the received block b7 + // However, unless the separate issue #778 is fixed, A and B would still not vote on b7 (which is added to the fork database + // but does not become the new best head since B5 has a later `latest_qc_block_timestamp`). + // --------------------------------------------------------------------------------------------------------------------------- + b8 = D.produce_block(); // Node D produces b8 + print("b8", b8); + BOOST_REQUIRE_EQUAL(b8->previous, b7->calculate_id()); // b8 has B7 as its parent block + BOOST_REQUIRE_EQUAL(qc_s(qc(b8)), weak_qc(b7)); // b8 claims a weak QC on b7 (A, B and C voted weak since locked on b4) + // prior to PR #788 (fixing issue #778), we'd have an test failure here + + b9 = D.produce_block(); // Node D produces b9 + print("b9", b9); + BOOST_REQUIRE_EQUAL(qc_s(qc(b9)), strong_qc(b8)); // b9 claims a strong QC on b8 (all nodes were able to vote strong) } FC_LOG_AND_RETHROW() From b1045ba1536fc4cd71ae186e55270200b3267fde Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Mon, 30 Sep 2024 11:37:51 -0400 Subject: [PATCH 24/35] If previous block is the root it is validated as well. --- libraries/chain/fork_database.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/chain/fork_database.cpp b/libraries/chain/fork_database.cpp index b6ac3f28d2..e6f20d204f 100644 --- a/libraries/chain/fork_database.cpp +++ b/libraries/chain/fork_database.cpp @@ -626,7 +626,7 @@ namespace eosio::chain { // if we return `true`, let's validate the precondition and make sure claimed_id is not in another branch assert(!id_present || block_header::num_from_id(claimed_id) <= block_header::num_from_id(root->id())); - return id_present; + return id_present || id == root->id(); } // ------------------ fork_database ------------------------- From eefa7654bbb48ec138db33e935432ce4cf8a0c70 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Mon, 30 Sep 2024 12:27:55 -0500 Subject: [PATCH 25/35] Revert "[5.0.3] P2P: Resolve on reconnect" --- plugins/net_plugin/net_plugin.cpp | 75 +++++++++++++------------------ 1 file changed, 31 insertions(+), 44 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index 59f3219068..d19674035d 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -348,6 +348,7 @@ namespace eosio { std::string host; connection_ptr c; tcp::endpoint active_ip; + tcp::resolver::results_type ips; }; using connection_details_index = multi_index_container< @@ -415,9 +416,9 @@ namespace eosio { void add(connection_ptr c); string connect(const string& host, const string& p2p_address); - string resolve_and_connect(const string& host, const string& p2p_address, const connection_ptr& c = {}); + string resolve_and_connect(const string& host, const string& p2p_address); void update_connection_endpoint(connection_ptr c, const tcp::endpoint& endpoint); - void reconnect(const connection_ptr& c); + void connect(const connection_ptr& c); string disconnect(const string& host); void close_all(); @@ -911,7 +912,7 @@ namespace eosio { fc::sha256 conn_node_id; string short_conn_node_id; - const string listen_address; // address sent to peer in handshake + string listen_address; // address sent to peer in handshake string log_p2p_address; string log_remote_endpoint_ip; string log_remote_endpoint_port; @@ -2763,10 +2764,6 @@ namespace eosio { fc_dlog( logger, "Skipping connect due to go_away reason ${r}",("r", reason_str( no_retry ))); return false; } - - if (incoming()) - return false; - if( consecutive_immediate_connection_close > def_max_consecutive_immediate_connection_close || no_retry == benign_other ) { fc::microseconds connector_period = my_impl->connections.get_connector_period(); fc::lock_guard g( conn_mtx ); @@ -2774,10 +2771,9 @@ namespace eosio { return true; // true so doesn't remove from valid connections } } - connection_ptr c = shared_from_this(); strand.post([c]() { - my_impl->connections.reconnect(c); + my_impl->connections.connect(c); }); return true; } @@ -4490,48 +4486,39 @@ namespace eosio { return resolve_and_connect( host, p2p_address ); } - string connections_manager::resolve_and_connect( const string& peer_address, const string& listen_address, - const connection_ptr& c ) - { - assert(!c || (c->peer_address() == peer_address && c->listen_address == listen_address)); - + string connections_manager::resolve_and_connect( const string& peer_address, const string& listen_address ) { string::size_type colon = peer_address.find(':'); if (colon == std::string::npos || colon == 0) { fc_elog( logger, "Invalid peer address. must be \"host:port[:|]\": ${p}", ("p", peer_address) ); return "invalid peer address"; } - { - std::lock_guard g(connections_mtx); - if (find_connection_i(peer_address)) - return "already connected"; - } + std::lock_guard g( connections_mtx ); + if( find_connection_i( peer_address ) ) + return "already connected"; auto [host, port, type] = split_host_port_type(peer_address); auto resolver = std::make_shared( my_impl->thread_pool.get_executor() ); resolver->async_resolve(host, port, - [this, resolver, c_org{c}, host, port, peer_address, listen_address]( const boost::system::error_code& err, const tcp::resolver::results_type& results ) { - connection_ptr c = c_org ? c_org : std::make_shared( peer_address, listen_address ); - c->strand.post([this, resolver, c, err, results, host, port, peer_address]() { - c->set_heartbeat_timeout( heartbeat_timeout ); - { - std::lock_guard g( connections_mtx ); - connections.emplace( connection_detail{ - .host = peer_address, - .c = c, - }); - } - if( !err ) { - c->connect( results ); - } else { - fc_wlog( logger, "Unable to resolve ${host}:${port} ${error}", - ("host", host)("port", port)( "error", err.message() ) ); - c->set_state(connection::connection_state::closed); - ++(c->consecutive_immediate_connection_close); - } + [resolver, host = host, port = port, peer_address = peer_address, listen_address = listen_address, this]( const boost::system::error_code& err, const tcp::resolver::results_type& results ) { + connection_ptr c = std::make_shared( peer_address, listen_address ); + c->set_heartbeat_timeout( heartbeat_timeout ); + std::lock_guard g( connections_mtx ); + auto [it, inserted] = connections.emplace( connection_detail{ + .host = peer_address, + .c = std::move(c), + .ips = results }); + if( !err ) { + it->c->connect( results ); + } else { + fc_wlog( logger, "Unable to resolve ${host}:${port} ${error}", + ("host", host)("port", port)( "error", err.message() ) ); + it->c->set_state(connection::connection_state::closed); + ++(it->c->consecutive_immediate_connection_close); + } } ); return "added connection"; @@ -4549,13 +4536,13 @@ namespace eosio { } } - void connections_manager::reconnect(const connection_ptr& c) { - { - std::lock_guard g( connections_mtx ); - auto& index = connections.get(); - index.erase(c); + void connections_manager::connect(const connection_ptr& c) { + std::lock_guard g( connections_mtx ); + const auto& index = connections.get(); + const auto& it = index.find(c); + if( it != index.end() ) { + it->c->connect( it->ips ); } - resolve_and_connect(c->peer_address(), c->listen_address, c); } // called by API From 02f0f3a06883cac42bcc67c1f8252b8a79d8e76d Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Tue, 1 Oct 2024 08:49:48 -0400 Subject: [PATCH 26/35] Update test to match scenarion from issue #751. --- unittests/savanna_misc_tests.cpp | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index 77d4fe55d3..cc525ad438 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -822,7 +822,7 @@ produce a block that utilizes the delayed vote is the time slot (t + d) where .. ¹ ... d = 1. ‡ ... d is infinite meaning the vote may never be received by producer p. -see https://github.com/AntelopeIO/spring/issues/751 +steps mentioned in comments below refer to issue https://github.com/AntelopeIO/spring/issues/751 --------------------------------------------------------------------------------------------------------*/ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branch, savanna_cluster::cluster_t) try { using namespace savanna_cluster; @@ -883,6 +883,8 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc A.push_vote_to(C, b4->calculate_id()); // vote on them set_partitions({ { &C }, { &D } }); // Node C is isolated from the other nodes (step 30) + // so A, B and C get b5 after b6 + b5 = C.produce_block(); print("b5", b5); BOOST_REQUIRE_EQUAL(qc_s(qc(b5)), strong_qc(b4)); // b5 claims a strong QC on b4 @@ -892,7 +894,12 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc BOOST_REQUIRE_EQUAL(b7->previous, b6->calculate_id()); // b7 has B6 as its parent block BOOST_REQUIRE_EQUAL(qc_s(qc(b7)), strong_qc(b2)); // b7 claims a strong QC on b2 - set_partition({}); + set_partition( { &C } ); // step 35 + + A.push_block(b6); // don't use `push_blocks_to` because of fork + B.push_block(b6); // step 36 + + set_partition( {} ); // step 38 A.push_block(b5); // A receives b5 BOOST_REQUIRE_EQUAL(A.lib_number, b3->block_num()); // which advances lib to b3 @@ -906,12 +913,12 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc // If, in order to verify this QC, they attempt to lookup b2 in fork_db, this will fail because lib (and hence fork_db's root) // has advanced to b3. // --------------------------------------------------------------------------------------------------------------------------- - A.push_block(b6); // don't use `push_blocks_to` because of fork A.push_block(b7); // prior to PR #719 (fixing issue #694), we'd have an exception here - - B.push_block(b6); B.push_block(b7); // prior to PR #719 (fixing issue #694), we'd have an exception here + C.push_block(b6); + C.push_block(b7); + // with issue #694 fixed, A and B were able to successfully validate the received block b7 // However, unless the separate issue #778 is fixed, A and B would still not vote on b7 (which is added to the fork database // but does not become the new best head since B5 has a later `latest_qc_block_timestamp`). From c2d54ade6707d4a2eeb5cf1fa6e141010b338826 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 1 Oct 2024 08:02:49 -0500 Subject: [PATCH 27/35] GH-525 Remove dead code --- plugins/net_plugin/net_plugin.cpp | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index 5e19a16f85..f7a90a7a34 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -351,7 +351,6 @@ namespace eosio { struct connection_detail { std::string host; connection_ptr c; - tcp::endpoint active_ip; tcp::resolver::results_type ips; }; @@ -421,7 +420,6 @@ namespace eosio { void add(connection_ptr c); string connect(const string& host, const string& p2p_address); string resolve_and_connect(const string& host, const string& p2p_address); - void update_connection_endpoint(connection_ptr c, const tcp::endpoint& endpoint); void connect(const connection_ptr& c); string disconnect(const string& host); void close_all(); @@ -2844,7 +2842,6 @@ namespace eosio { boost::asio::bind_executor( strand, [c = shared_from_this(), socket=socket]( const boost::system::error_code& err, const tcp::endpoint& endpoint ) { if( !err && socket->is_open() && socket == c->socket ) { - my_impl->connections.update_connection_endpoint(c, endpoint); c->update_endpoints(endpoint); if( c->start_session() ) { c->send_handshake(); @@ -4647,12 +4644,9 @@ namespace eosio { void connections_manager::add( connection_ptr c ) { std::lock_guard g( connections_mtx ); - boost::system::error_code ec; - auto endpoint = c->socket->remote_endpoint(ec); connections.insert( connection_detail{ .host = c->peer_address(), - .c = std::move(c), - .active_ip = endpoint} ); + .c = std::move(c)} ); } // called by API @@ -4701,18 +4695,6 @@ namespace eosio { return "added connection"; } - void connections_manager::update_connection_endpoint(connection_ptr c, - const tcp::endpoint& endpoint) { - std::unique_lock g( connections_mtx ); - auto& index = connections.get(); - const auto& it = index.find(c); - if( it != index.end() ) { - index.modify(it, [endpoint](connection_detail& cd) { - cd.active_ip = endpoint; - }); - } - } - void connections_manager::connect(const connection_ptr& c) { std::lock_guard g( connections_mtx ); const auto& index = connections.get(); From f336eafb19fb52086a73f21b7c09f207af2bf504 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 1 Oct 2024 09:04:24 -0500 Subject: [PATCH 28/35] GH-525 Minimum code changes to resolve on reconnect --- plugins/net_plugin/net_plugin.cpp | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index f7a90a7a34..ab3295eed7 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -419,8 +419,9 @@ namespace eosio { void add(connection_ptr c); string connect(const string& host, const string& p2p_address); - string resolve_and_connect(const string& host, const string& p2p_address); + string resolve_and_connect(const string& host, const string& p2p_address, uint16_t consecutive_immediate_connection_close = 0); void connect(const connection_ptr& c); + void remove(const connection_ptr& c); string disconnect(const string& host); void close_all(); @@ -835,7 +836,7 @@ namespace eosio { public: enum class connection_state { connecting, connected, closing, closed }; - explicit connection( const string& endpoint, const string& listen_address ); + explicit connection( const string& endpoint, const string& listen_address, uint16_t consecutive_immediate_connection_close ); /// @brief ctor /// @param socket created by boost::asio in fc::listener /// @param address identifier of listen socket which accepted this new connection @@ -1264,13 +1265,14 @@ namespace eosio { //--------------------------------------------------------------------------- - connection::connection( const string& endpoint, const string& listen_address ) + connection::connection( const string& endpoint, const string& listen_address, uint16_t consecutive_immediate_connection_close ) : peer_addr( endpoint ), strand( my_impl->thread_pool.get_executor() ), socket( new tcp::socket( my_impl->thread_pool.get_executor() ) ), listen_address( listen_address ), log_p2p_address( endpoint ), connection_id( ++my_impl->current_connection_id ), + consecutive_immediate_connection_close( consecutive_immediate_connection_close ), sync_response_expected_timer( my_impl->thread_pool.get_executor() ), last_handshake_recv(), last_handshake_sent(), @@ -2826,10 +2828,10 @@ namespace eosio { return true; // true so doesn't remove from valid connections } } + connection_ptr c = shared_from_this(); - strand.post([c]() { - my_impl->connections.connect(c); - }); + my_impl->connections.remove(c); + my_impl->connections.resolve_and_connect(c->peer_address(), c->listen_address, c->consecutive_immediate_connection_close.load()); return true; } @@ -4657,7 +4659,7 @@ namespace eosio { return resolve_and_connect( host, p2p_address ); } - string connections_manager::resolve_and_connect( const string& peer_address, const string& listen_address ) { + string connections_manager::resolve_and_connect( const string& peer_address, const string& listen_address, uint16_t consecutive_immediate_connection_close ) { string::size_type colon = peer_address.find(':'); if (colon == std::string::npos || colon == 0) { fc_elog( logger, "Invalid peer address. must be \"host:port[:|]\": ${p}", ("p", peer_address) ); @@ -4673,8 +4675,9 @@ namespace eosio { auto resolver = std::make_shared( my_impl->thread_pool.get_executor() ); resolver->async_resolve(host, port, - [resolver, host = host, port = port, peer_address = peer_address, listen_address = listen_address, this]( const boost::system::error_code& err, const tcp::resolver::results_type& results ) { - connection_ptr c = std::make_shared( peer_address, listen_address ); + [resolver, host, port, peer_address, listen_address, consecutive_immediate_connection_close, this] + ( const boost::system::error_code& err, const tcp::resolver::results_type& results ) { + connection_ptr c = std::make_shared( peer_address, listen_address, consecutive_immediate_connection_close ); c->set_heartbeat_timeout( heartbeat_timeout ); std::lock_guard g( connections_mtx ); auto [it, inserted] = connections.emplace( connection_detail{ @@ -4688,7 +4691,7 @@ namespace eosio { fc_wlog( logger, "Unable to resolve ${host}:${port} ${error}", ("host", host)("port", port)( "error", err.message() ) ); it->c->set_state(connection::connection_state::closed); - ++(it->c->consecutive_immediate_connection_close); + ++it->c->consecutive_immediate_connection_close; } } ); @@ -4704,7 +4707,13 @@ namespace eosio { } } - // called by API + void connections_manager::remove(const connection_ptr& c) { + std::lock_guard g( connections_mtx ); + auto& index = connections.get(); + index.erase(c); + } + +// called by API string connections_manager::disconnect( const string& host ) { std::lock_guard g( connections_mtx ); auto& index = connections.get(); From 3731f83847b3d3b7860b0112705db0141263db90 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Tue, 1 Oct 2024 14:01:45 -0400 Subject: [PATCH 29/35] Add diagram showing timeline for A, B C and D receiving blocks, --- unittests/savanna_misc_tests.cpp | 56 ++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 13 deletions(-) diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index cc525ad438..83851e886e 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -793,12 +793,12 @@ BOOST_FIXTURE_TEST_CASE(finalizers_locked_preventing_vote_on_alternate_branch, s Producer: C C C C C D D D D Timestamp: t1 t2 t3 t4 t5 t6 t7 t8 t9 Blocks: - B0 <--- B1 <--- B2 <--- B3 <-|- B4 <--- B5 + b0 <--- b1 <--- b2 <--- b3 <-|- b4 <--- b5 | - \----------------- B6 <--- B7 <--- B8 <--- B9 + \----------------- b6 <--- b7 <--- b8 <--- b9 QC claim: Strong Strong Strong Strong Strong Strong Strong Weak Strong - B0 B0 B1 B3 B4 B1 B2 B7 B8 + b0 b0 b1 b3 b4 b1 b2 b7 b8 Votes: Node A: Strong‡ Strong‡ Strong‡ Strong Weak¹ Weak Strong Strong @@ -808,10 +808,10 @@ QC claim: ^ | - Validating the strong QC on B2 should - not fail for nodes which receive B4 and - B5 prior to B7 despite B5 advancing the - fork DB root to B3. + Validating the strong QC on b2 should + not fail for nodes which receive b4 and + b5 prior to b7 despite b5 advancing the + fork DB root to b3. Meaning of the superscripts and marks on the votes: The vote on block b was delayed in reaching the node for the producer p scheduled @@ -823,6 +823,36 @@ produce a block that utilizes the delayed vote is the time slot (t + d) where .. ‡ ... d is infinite meaning the vote may never be received by producer p. steps mentioned in comments below refer to issue https://github.com/AntelopeIO/spring/issues/751 + +Diagram below shows the timeline for nodes A, B, C and D receiving blocks b1 through b9. +(x) marks the producer of the block. + +step network partition A B C D +--------------------------------------------------------------- + b1 b1 b1(x) b1 +(3) A / B C D +(4) b2 b2(x) b2 +(9) b3 b3(x) b3 +(15) A / B C / D +(18) b4 b4(x) +(20) b6(x) +(22) A D / B C +(23) b2 +(25) b3 +(26) A B C / D +(28) b4 +(30) A B / C / D +(31) b5(x) +(33) b7(x) +(35) A B D / C +(36) b6 b6 +(38) A B C D +(39) b5 b5 +(40) b6 +(41,43) b7 b7 b7 +(44) b8 b8 b8 b8(x) +(51) b9 b9 b9 b9(x) + --------------------------------------------------------------------------------------------------------*/ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branch, savanna_cluster::cluster_t) try { using namespace savanna_cluster; @@ -868,11 +898,11 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc print("b4", b4); BOOST_REQUIRE_EQUAL(qc_s(qc(b4)), strong_qc(b3)); // b4 claims a strong QC on b3 (B and D votes not delayed anymore) - b6 = D.produce_block(_block_interval_us * 2); // Node D produces and broadcasts B6 one second early (due + b6 = D.produce_block(_block_interval_us * 2); // Node D produces and broadcasts b6 one second early (due print("b6", b6); // to clock differences). - BOOST_REQUIRE_EQUAL(b6->previous, b3->calculate_id()); // b6 has B3 as its parent block + BOOST_REQUIRE_EQUAL(b6->previous, b3->calculate_id()); // b6 has b3 as its parent block BOOST_REQUIRE(!qc(b6)); // b6 does not include a new qc (lacking votes on b2 and b3) - BOOST_REQUIRE_EQUAL(qc_claim(b6), qc_claim(b3)); // and repeats b3's strong QC claim on B1. + BOOST_REQUIRE_EQUAL(qc_claim(b6), qc_claim(b3)); // and repeats b3's strong QC claim on b1. C.push_blocks_to(A); // simulates A and D temporarily reconnecting, D sending the blocks A.push_vote_to(D, b2->calculate_id()); // produced by C, A voting on them and D receiving these votes @@ -891,7 +921,7 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc b7 = D.produce_block(); // Node D produces b7 print("b7", b7); - BOOST_REQUIRE_EQUAL(b7->previous, b6->calculate_id()); // b7 has B6 as its parent block + BOOST_REQUIRE_EQUAL(b7->previous, b6->calculate_id()); // b7 has b6 as its parent block BOOST_REQUIRE_EQUAL(qc_s(qc(b7)), strong_qc(b2)); // b7 claims a strong QC on b2 set_partition( { &C } ); // step 35 @@ -921,11 +951,11 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc // with issue #694 fixed, A and B were able to successfully validate the received block b7 // However, unless the separate issue #778 is fixed, A and B would still not vote on b7 (which is added to the fork database - // but does not become the new best head since B5 has a later `latest_qc_block_timestamp`). + // but does not become the new best head since b5 has a later `latest_qc_block_timestamp`). // --------------------------------------------------------------------------------------------------------------------------- b8 = D.produce_block(); // Node D produces b8 print("b8", b8); - BOOST_REQUIRE_EQUAL(b8->previous, b7->calculate_id()); // b8 has B7 as its parent block + BOOST_REQUIRE_EQUAL(b8->previous, b7->calculate_id()); // b8 has b7 as its parent block BOOST_REQUIRE_EQUAL(qc_s(qc(b8)), weak_qc(b7)); // b8 claims a weak QC on b7 (A, B and C voted weak since locked on b4) // prior to PR #788 (fixing issue #778), we'd have an test failure here From 2abb404f740e6f088008fbe6b2e4ebfb6e9b604d Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Tue, 1 Oct 2024 14:41:22 -0400 Subject: [PATCH 30/35] Alignment in comment --- unittests/savanna_misc_tests.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index 83851e886e..a35345f682 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -842,16 +842,16 @@ step network partition A B C D (26) A B C / D (28) b4 (30) A B / C / D -(31) b5(x) +(31) b5(x) (33) b7(x) (35) A B D / C (36) b6 b6 (38) A B C D (39) b5 b5 -(40) b6 -(41,43) b7 b7 b7 -(44) b8 b8 b8 b8(x) -(51) b9 b9 b9 b9(x) +(40) b6 +(41,43) b7 b7 b7 +(44) b8 b8 b8 b8(x) +(51) b9 b9 b9 b9(x) --------------------------------------------------------------------------------------------------------*/ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branch, savanna_cluster::cluster_t) try { From 6e8767ff482074d89f4515e50a91b3ba975441c8 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 1 Oct 2024 13:44:09 -0500 Subject: [PATCH 31/35] GH-525 Refactor resolve_and_connect so that connection object is reused. --- plugins/net_plugin/net_plugin.cpp | 148 +++++++++++++++--------------- 1 file changed, 73 insertions(+), 75 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index ab3295eed7..326d3f55a3 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -351,7 +351,6 @@ namespace eosio { struct connection_detail { std::string host; connection_ptr c; - tcp::resolver::results_type ips; }; using connection_details_index = multi_index_container< @@ -403,6 +402,8 @@ namespace eosio { boost::asio::steady_timer::duration conn_period, uint32_t maximum_client_count); + std::chrono::milliseconds get_heartbeat_timeout() const { return heartbeat_timeout; } + uint32_t get_max_client_count() const { return max_client_count; } fc::microseconds get_connector_period() const; @@ -419,9 +420,7 @@ namespace eosio { void add(connection_ptr c); string connect(const string& host, const string& p2p_address); - string resolve_and_connect(const string& host, const string& p2p_address, uint16_t consecutive_immediate_connection_close = 0); - void connect(const connection_ptr& c); - void remove(const connection_ptr& c); + string resolve_and_connect(const string& host, const string& p2p_address); string disconnect(const string& host); void close_all(); @@ -836,7 +835,7 @@ namespace eosio { public: enum class connection_state { connecting, connected, closing, closed }; - explicit connection( const string& endpoint, const string& listen_address, uint16_t consecutive_immediate_connection_close ); + explicit connection( const string& endpoint, const string& listen_address ); /// @brief ctor /// @param socket created by boost::asio in fc::listener /// @param address identifier of listen socket which accepted this new connection @@ -1014,7 +1013,7 @@ namespace eosio { bool populate_handshake( handshake_message& hello ) const; - bool reconnect(); + bool resolve_and_connect(); void connect( const tcp::resolver::results_type& endpoints ); void start_read_message(); @@ -1265,21 +1264,20 @@ namespace eosio { //--------------------------------------------------------------------------- - connection::connection( const string& endpoint, const string& listen_address, uint16_t consecutive_immediate_connection_close ) + connection::connection( const string& endpoint, const string& listen_address ) : peer_addr( endpoint ), strand( my_impl->thread_pool.get_executor() ), socket( new tcp::socket( my_impl->thread_pool.get_executor() ) ), listen_address( listen_address ), log_p2p_address( endpoint ), connection_id( ++my_impl->current_connection_id ), - consecutive_immediate_connection_close( consecutive_immediate_connection_close ), sync_response_expected_timer( my_impl->thread_pool.get_executor() ), last_handshake_recv(), last_handshake_sent(), p2p_address( endpoint ) { + set_connection_type( peer_address() ); my_impl->mark_bp_connection(this); - update_endpoints(); fc_ilog( logger, "created connection - ${c} to ${n}", ("c", connection_id)("n", endpoint) ); } @@ -1294,7 +1292,6 @@ namespace eosio { last_handshake_recv(), last_handshake_sent() { - update_endpoints(); fc_dlog( logger, "new connection - ${c} object created for peer ${address}:${port} from listener ${addr}", ("c", connection_id)("address", log_remote_endpoint_ip)("port", log_remote_endpoint_port)("addr", listen_address) ); } @@ -1388,6 +1385,7 @@ namespace eosio { bool connection::start_session() { verify_strand_in_this_thread( strand, __func__, __LINE__ ); + update_endpoints(); boost::asio::ip::tcp::no_delay nodelay( true ); boost::system::error_code ec; socket->set_option( nodelay, ec ); @@ -2810,31 +2808,6 @@ namespace eosio { //------------------------------------------------------------------------ - bool connection::reconnect() { - switch ( no_retry ) { - case no_reason: - case wrong_version: - case benign_other: - case duplicate: // attempt reconnect in case connection has been dropped, should quickly disconnect if duplicate - break; - default: - fc_dlog( logger, "Skipping connect due to go_away reason ${r}",("r", reason_str( no_retry ))); - return false; - } - if( consecutive_immediate_connection_close > def_max_consecutive_immediate_connection_close || no_retry == benign_other ) { - fc::microseconds connector_period = my_impl->connections.get_connector_period(); - fc::lock_guard g( conn_mtx ); - if( last_close == fc::time_point() || last_close > fc::time_point::now() - connector_period ) { - return true; // true so doesn't remove from valid connections - } - } - - connection_ptr c = shared_from_this(); - my_impl->connections.remove(c); - my_impl->connections.resolve_and_connect(c->peer_address(), c->listen_address, c->consecutive_immediate_connection_close.load()); - return true; - } - // called from connection strand void connection::connect( const tcp::resolver::results_type& endpoints ) { set_state(connection_state::connecting); @@ -4635,6 +4608,7 @@ namespace eosio { update_p2p_connection_metrics = std::move(fun); } + // can be called from any thread void connections_manager::connect_supplied_peers(const string& p2p_address) { std::unique_lock g(connections_mtx); chain::flat_set peers = supplied_peers; @@ -4659,61 +4633,85 @@ namespace eosio { return resolve_and_connect( host, p2p_address ); } - string connections_manager::resolve_and_connect( const string& peer_address, const string& listen_address, uint16_t consecutive_immediate_connection_close ) { + string connections_manager::resolve_and_connect( const string& peer_address, const string& listen_address ) { string::size_type colon = peer_address.find(':'); if (colon == std::string::npos || colon == 0) { fc_elog( logger, "Invalid peer address. must be \"host:port[:|]\": ${p}", ("p", peer_address) ); return "invalid peer address"; } - std::lock_guard g( connections_mtx ); - if( find_connection_i( peer_address ) ) - return "already connected"; + { + std::lock_guard g( connections_mtx ); + if( find_connection_i( peer_address ) ) + return "already connected"; + } auto [host, port, type] = split_host_port_type(peer_address); - auto resolver = std::make_shared( my_impl->thread_pool.get_executor() ); - - resolver->async_resolve(host, port, - [resolver, host, port, peer_address, listen_address, consecutive_immediate_connection_close, this] - ( const boost::system::error_code& err, const tcp::resolver::results_type& results ) { - connection_ptr c = std::make_shared( peer_address, listen_address, consecutive_immediate_connection_close ); - c->set_heartbeat_timeout( heartbeat_timeout ); - std::lock_guard g( connections_mtx ); - auto [it, inserted] = connections.emplace( connection_detail{ - .host = peer_address, - .c = std::move(c), - .ips = results - }); - if( !err ) { - it->c->connect( results ); - } else { - fc_wlog( logger, "Unable to resolve ${host}:${port} ${error}", - ("host", host)("port", port)( "error", err.message() ) ); - it->c->set_state(connection::connection_state::closed); - ++it->c->consecutive_immediate_connection_close; - } - } ); + connection_ptr c = std::make_shared( peer_address, listen_address ); + if (c->resolve_and_connect()) { + add(c); + + return "added connection"; + } - return "added connection"; + return "connection failed"; } - void connections_manager::connect(const connection_ptr& c) { - std::lock_guard g( connections_mtx ); - const auto& index = connections.get(); - const auto& it = index.find(c); - if( it != index.end() ) { - it->c->connect( it->ips ); + // called from any thread + bool connection::resolve_and_connect() { + switch ( no_retry ) { + case no_reason: + case wrong_version: + case benign_other: + case duplicate: // attempt reconnect in case connection has been dropped, should quickly disconnect if duplicate + break; + default: + fc_dlog( logger, "Skipping connect due to go_away reason ${r}",("r", reason_str( no_retry ))); + return false; } - } - void connections_manager::remove(const connection_ptr& c) { - std::lock_guard g( connections_mtx ); - auto& index = connections.get(); - index.erase(c); + string::size_type colon = peer_address().find(':'); + if (colon == std::string::npos || colon == 0) { + fc_elog( logger, "Invalid peer address. must be \"host:port[:|]\": ${p}", ("p", peer_address()) ); + return false; + } + + connection_ptr c = shared_from_this(); + + if( consecutive_immediate_connection_close > def_max_consecutive_immediate_connection_close || no_retry == benign_other ) { + fc::microseconds connector_period = my_impl->connections.get_connector_period(); + fc::lock_guard g( conn_mtx ); + if( last_close == fc::time_point() || last_close > fc::time_point::now() - connector_period ) { + return true; // true so doesn't remove from valid connections + } + } + + auto [host, port, type] = split_host_port_type(c->peer_address()); + if (host.empty()) + return false; + + strand.post([c, host, port]() { + auto resolver = std::make_shared( my_impl->thread_pool.get_executor() ); + resolver->async_resolve(host, port, + [resolver, c, host, port] + ( const boost::system::error_code& err, const tcp::resolver::results_type& results ) { + c->set_heartbeat_timeout( my_impl->connections.get_heartbeat_timeout() ); + if( !err ) { + c->connect( results ); + } else { + fc_wlog( logger, "Unable to resolve ${host}:${port} ${error}", + ("host", host)("port", port)( "error", err.message() ) ); + c->set_state(connection::connection_state::closed); + ++c->consecutive_immediate_connection_close; + } + } ); + } ); + + return true; } -// called by API + // called by API string connections_manager::disconnect( const string& host ) { std::lock_guard g( connections_mtx ); auto& index = connections.get(); @@ -4815,7 +4813,7 @@ namespace eosio { auto cleanup = [&num_peers, &num_rm, this](vector&& reconnecting, vector&& removing) { for( auto& c : reconnecting ) { - if (!c->reconnect()) { + if (!c->resolve_and_connect()) { --num_peers; ++num_rm; removing.push_back(c); From 60eab186aa6e1498d6fae68aef2f86d205295ff8 Mon Sep 17 00:00:00 2001 From: greg7mdp Date: Tue, 1 Oct 2024 15:04:03 -0400 Subject: [PATCH 32/35] Comment out `_debug_mode` --- unittests/savanna_misc_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unittests/savanna_misc_tests.cpp b/unittests/savanna_misc_tests.cpp index a35345f682..56e686333a 100644 --- a/unittests/savanna_misc_tests.cpp +++ b/unittests/savanna_misc_tests.cpp @@ -858,7 +858,7 @@ BOOST_FIXTURE_TEST_CASE(finality_advancing_past_block_claimed_on_alternate_branc using namespace savanna_cluster; auto& A=_nodes[0]; auto& B=_nodes[1]; auto& C=_nodes[2]; auto& D=_nodes[3]; - _debug_mode = true; + //_debug_mode = true; auto b0 = A.produce_block(); print("b0", b0); From 709f7c0e8f7ffef8ac5e849d9bff101a7409c317 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 1 Oct 2024 15:36:34 -0500 Subject: [PATCH 33/35] GH-525 Used shared_lock for shared_mutex --- plugins/net_plugin/net_plugin.cpp | 32 ++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index 326d3f55a3..7bff463995 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -4579,7 +4579,7 @@ namespace eosio { //---------------------------------------------------------------------------- size_t connections_manager::number_connections() const { - std::lock_guard g(connections_mtx); + std::shared_lock g(connections_mtx); return connections.size(); } @@ -4610,7 +4610,7 @@ namespace eosio { // can be called from any thread void connections_manager::connect_supplied_peers(const string& p2p_address) { - std::unique_lock g(connections_mtx); + std::shared_lock g(connections_mtx); chain::flat_set peers = supplied_peers; g.unlock(); for (const auto& peer : peers) { @@ -4641,7 +4641,7 @@ namespace eosio { } { - std::lock_guard g( connections_mtx ); + std::shared_lock g( connections_mtx ); if( find_connection_i( peer_address ) ) return "already connected"; } @@ -4737,8 +4737,11 @@ namespace eosio { } std::optional connections_manager::status( const string& host )const { - std::shared_lock g( connections_mtx ); - auto con = find_connection_i( host ); + connection_ptr con; + { + std::shared_lock g( connections_mtx ); + con = find_connection_i( host ); + } if( con ) { return con->get_status(); } @@ -4746,12 +4749,19 @@ namespace eosio { } vector connections_manager::connection_statuses()const { + vector conns; vector result; - std::shared_lock g( connections_mtx ); - auto& index = connections.get(); - result.reserve( index.size() ); - for( const connection_detail& cd : index ) { - result.emplace_back( cd.c->get_status() ); + { + std::shared_lock g( connections_mtx ); + auto& index = connections.get(); + result.reserve( index.size() ); + conns.reserve( index.size() ); + for( const connection_detail& cd : index ) { + conns.emplace_back( cd.c ); + } + } + for (const auto& c : conns) { + result.push_back( c->get_status() ); } return result; } @@ -4879,7 +4889,7 @@ namespace eosio { assert(update_p2p_connection_metrics); auto from = from_connection.lock(); std::shared_lock g(connections_mtx); - auto& index = connections.get(); + const auto& index = connections.get(); size_t num_clients = 0, num_peers = 0, num_bp_peers = 0; net_plugin::p2p_per_connection_metrics per_connection(index.size()); for (auto it = index.begin(); it != index.end(); ++it) { From 3c6363058ac114fad9a257d682332b3ac49ce7f6 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 1 Oct 2024 15:37:24 -0500 Subject: [PATCH 34/35] GH-525 Used std::move --- plugins/net_plugin/net_plugin.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index 7bff463995..e966eb3d7d 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -4650,7 +4650,7 @@ namespace eosio { connection_ptr c = std::make_shared( peer_address, listen_address ); if (c->resolve_and_connect()) { - add(c); + add(std::move(c)); return "added connection"; } From 6c6b22fd36513a8c4639bde40481811deff17712 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 1 Oct 2024 18:42:37 -0500 Subject: [PATCH 35/35] GH-525 Remove duplicate code --- plugins/net_plugin/net_plugin.cpp | 38 ++++++++++++++----------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/plugins/net_plugin/net_plugin.cpp b/plugins/net_plugin/net_plugin.cpp index e966eb3d7d..f5d93f64dc 100644 --- a/plugins/net_plugin/net_plugin.cpp +++ b/plugins/net_plugin/net_plugin.cpp @@ -1190,16 +1190,21 @@ namespace eosio { }; - std::tuple split_host_port_type(const std::string& peer_add) { + std::tuple split_host_port_type(const std::string& peer_add, bool incoming) { // host:port:[|] if (peer_add.empty()) return {}; string::size_type p = peer_add[0] == '[' ? peer_add.find(']') : 0; - if (p == string::npos) { - fc_wlog( logger, "Invalid peer address: ${peer}", ("peer", peer_add) ); + string::size_type colon = p != string::npos ? peer_add.find(':', p) : string::npos; + if (colon == std::string::npos || colon == 0) { + // if incoming then not an error this peer can do anything about + if (incoming) { + fc_dlog( logger, "Invalid peer address. must be \"host:port[:|]\": ${p}", ("p", peer_add) ); + } else { + fc_elog( logger, "Invalid peer address. must be \"host:port[:|]\": ${p}", ("p", peer_add) ); + } return {}; } - string::size_type colon = peer_add.find(':', p); string::size_type colon2 = peer_add.find(':', colon + 1); string::size_type end = colon2 == string::npos ? string::npos : peer_add.find_first_of( " :+=.,<>!$%^&(*)|-#@\t", colon2 + 1 ); // future proof by including most symbols without using regex @@ -1324,7 +1329,7 @@ namespace eosio { // called from connection strand void connection::set_connection_type( const std::string& peer_add ) { - auto [host, port, type] = split_host_port_type(peer_add); + auto [host, port, type] = split_host_port_type(peer_add, false); if( type.empty() ) { fc_dlog( logger, "Setting connection - ${c} type for: ${peer} to both transactions and blocks", ("c", connection_id)("peer", peer_add) ); connection_type = both; @@ -2866,7 +2871,7 @@ namespace eosio { fc_ilog(logger, "Accepted new connection: " + paddr_str); connections.any_of_supplied_peers([&listen_address, &paddr_str, &paddr_desc, &limit](const string& peer_addr) { - auto [host, port, type] = split_host_port_type(peer_addr); + auto [host, port, type] = split_host_port_type(peer_addr, false); if (host == paddr_str) { if (limit > 0) { fc_dlog(logger, "Connection inbound to ${la} from ${a} is a configured p2p-peer-address and will not be throttled", ("la", listen_address)("a", paddr_desc)); @@ -3362,9 +3367,9 @@ namespace eosio { } if( incoming() ) { - auto [host, port, type] = split_host_port_type(msg.p2p_address); + auto [host, port, type] = split_host_port_type(msg.p2p_address, true); if (host.size()) - set_connection_type( msg.p2p_address ); + set_connection_type( msg.p2p_address); peer_dlog( this, "checking for duplicate" ); auto is_duplicate = [&](const connection_ptr& check) { @@ -4634,9 +4639,8 @@ namespace eosio { } string connections_manager::resolve_and_connect( const string& peer_address, const string& listen_address ) { - string::size_type colon = peer_address.find(':'); - if (colon == std::string::npos || colon == 0) { - fc_elog( logger, "Invalid peer address. must be \"host:port[:|]\": ${p}", ("p", peer_address) ); + auto [host, port, type] = split_host_port_type(peer_address, false); + if (host.empty()) { return "invalid peer address"; } @@ -4646,8 +4650,6 @@ namespace eosio { return "already connected"; } - auto [host, port, type] = split_host_port_type(peer_address); - connection_ptr c = std::make_shared( peer_address, listen_address ); if (c->resolve_and_connect()) { add(std::move(c)); @@ -4671,11 +4673,9 @@ namespace eosio { return false; } - string::size_type colon = peer_address().find(':'); - if (colon == std::string::npos || colon == 0) { - fc_elog( logger, "Invalid peer address. must be \"host:port[:|]\": ${p}", ("p", peer_address()) ); + auto [host, port, type] = split_host_port_type(peer_address(), false); + if (host.empty()) return false; - } connection_ptr c = shared_from_this(); @@ -4687,10 +4687,6 @@ namespace eosio { } } - auto [host, port, type] = split_host_port_type(c->peer_address()); - if (host.empty()) - return false; - strand.post([c, host, port]() { auto resolver = std::make_shared( my_impl->thread_pool.get_executor() ); resolver->async_resolve(host, port,