From 5b5a861d5db5220fb56b5be84fa4c3ebe93ec9b0 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Thu, 18 Apr 2024 12:20:09 +0900 Subject: [PATCH 1/6] Fix bootstrap_processor.lazy_pruning_missing_block --- nano/core_test/bootstrap.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nano/core_test/bootstrap.cpp b/nano/core_test/bootstrap.cpp index 7ff12f9f3f..d343681853 100644 --- a/nano/core_test/bootstrap.cpp +++ b/nano/core_test/bootstrap.cpp @@ -1306,6 +1306,9 @@ TEST (bootstrap_processor, lazy_pruning_missing_block) config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled; config.enable_voting = false; // Remove after allowing pruned voting nano::node_flags node_flags; + // there is a race condition between starting the second node and publishing of blocks + // disabling the request loop, which is not needed for this, makes the problem go away + node_flags.disable_request_loop = true; node_flags.disable_bootstrap_bulk_push_client = true; node_flags.disable_legacy_bootstrap = true; node_flags.disable_ascending_bootstrap = true; From 5f5b2685e4e0890ce5b308be3d9ddda216e50ed5 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Thu, 18 Apr 2024 13:20:36 +0900 Subject: [PATCH 2/6] More accurate explanation and readability improvements --- nano/core_test/bootstrap.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/nano/core_test/bootstrap.cpp b/nano/core_test/bootstrap.cpp index d343681853..1f845b26a5 100644 --- a/nano/core_test/bootstrap.cpp +++ b/nano/core_test/bootstrap.cpp @@ -1306,8 +1306,11 @@ TEST (bootstrap_processor, lazy_pruning_missing_block) config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled; config.enable_voting = false; // Remove after allowing pruned voting nano::node_flags node_flags; - // there is a race condition between starting the second node and publishing of blocks - // disabling the request loop, which is not needed for this, makes the problem go away + // It looks like force confirmed elections remain in the AEC and + // they keep publishing blocks, disabling the request loop stops this. + // A better fix would be to create a way to add and cement the blocks + // without involving an election at all or have a way to delete an + // election from the AEC. node_flags.disable_request_loop = true; node_flags.disable_bootstrap_bulk_push_client = true; node_flags.disable_legacy_bootstrap = true; @@ -1330,7 +1333,6 @@ TEST (bootstrap_processor, lazy_pruning_missing_block) .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) .work (*system.work.generate (nano::dev::genesis->hash ())) .build (); - node1->process_active (send1); // send from genesis to key2 auto send2 = builder @@ -1343,7 +1345,6 @@ TEST (bootstrap_processor, lazy_pruning_missing_block) .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) .work (*system.work.generate (send1->hash ())) .build (); - node1->process_active (send2); // open account key1 auto open = builder @@ -1354,7 +1355,6 @@ TEST (bootstrap_processor, lazy_pruning_missing_block) .sign (key1.prv, key1.pub) .work (*system.work.generate (key1.pub)) .build (); - node1->process_active (open); // open account key2 auto state_open = builder @@ -1368,11 +1368,9 @@ TEST (bootstrap_processor, lazy_pruning_missing_block) .work (*system.work.generate (key2.pub)) .build (); - node1->process_active (state_open); - ASSERT_TIMELY (5s, node1->block (state_open->hash ()) != nullptr); - // Confirm last block to prune previous + ASSERT_TRUE (nano::test::process (*node1, { send1, send2, open, state_open })); ASSERT_TRUE (nano::test::start_elections (system, *node1, { send1, send2, open, state_open }, true)); - ASSERT_TIMELY (5s, nano::test::confirmed (*node1, { send2, open, state_open })); + ASSERT_TIMELY (5s, nano::test::confirmed (*node1, { send1, send2, open, state_open })); ASSERT_EQ (5, node1->ledger.block_count ()); ASSERT_EQ (5, node1->ledger.cemented_count ()); From 0c7b1b90fd5112f08bc89a04fe382a4c906bdd1b Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Thu, 18 Apr 2024 22:16:16 +0900 Subject: [PATCH 3/6] Introduce test function nano::test::confirm() --- nano/secure/ledger.hpp | 2 ++ nano/test_common/testutil.cpp | 8 ++++++++ nano/test_common/testutil.hpp | 5 +++++ 3 files changed, 15 insertions(+) diff --git a/nano/secure/ledger.hpp b/nano/secure/ledger.hpp index 465b029eac..a309f6e5a8 100644 --- a/nano/secure/ledger.hpp +++ b/nano/secure/ledger.hpp @@ -116,6 +116,8 @@ class ledger final // Returns the next receivable entry equal or greater than 'key' std::optional> receivable_lower_bound (secure::transaction const & tx, nano::account const & account, nano::block_hash const & hash) const; void initialize (nano::generate_cache_flags const &); + +public: // for unit tests only void confirm (secure::write_transaction const & transaction, nano::block const & block); }; } diff --git a/nano/test_common/testutil.cpp b/nano/test_common/testutil.cpp index 8ff84dd5f5..3fd3bbf07a 100644 --- a/nano/test_common/testutil.cpp +++ b/nano/test_common/testutil.cpp @@ -121,6 +121,14 @@ bool nano::test::exists (nano::node & node, std::vector> blocks) +{ + for (auto block : blocks) + { + ledger.confirm (ledger.tx_begin_write (), *block); + } +} + bool nano::test::block_or_pruned_all_exists (nano::node & node, std::vector hashes) { auto transaction = node.ledger.tx_begin_read (); diff --git a/nano/test_common/testutil.hpp b/nano/test_common/testutil.hpp index 89155614d1..ce1ee83806 100644 --- a/nano/test_common/testutil.hpp +++ b/nano/test_common/testutil.hpp @@ -321,6 +321,11 @@ namespace test * @return true if all blocks are fully processed and inserted in the ledger, false otherwise */ bool exists (nano::node & node, std::vector> blocks); + /* + * Convenience function to confirm/cement a block in the ledger by setting the confirmation + * height of the account to be the height of the block. This could cement more than the block given. + */ + void confirm (nano::ledger & ledger, std::vector> blocks); /* * Convenience function to check whether *all* of the hashes exists in node ledger or in the pruned table. * @return true if all blocks are fully processed and inserted in the ledger, false otherwise From 485a104a30e1be447063ed4fbfb64136a012a00a Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Thu, 18 Apr 2024 22:18:11 +0900 Subject: [PATCH 4/6] Do the test without using elections --- nano/core_test/bootstrap.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/nano/core_test/bootstrap.cpp b/nano/core_test/bootstrap.cpp index 1f845b26a5..5bd6fc051d 100644 --- a/nano/core_test/bootstrap.cpp +++ b/nano/core_test/bootstrap.cpp @@ -1306,12 +1306,6 @@ TEST (bootstrap_processor, lazy_pruning_missing_block) config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled; config.enable_voting = false; // Remove after allowing pruned voting nano::node_flags node_flags; - // It looks like force confirmed elections remain in the AEC and - // they keep publishing blocks, disabling the request loop stops this. - // A better fix would be to create a way to add and cement the blocks - // without involving an election at all or have a way to delete an - // election from the AEC. - node_flags.disable_request_loop = true; node_flags.disable_bootstrap_bulk_push_client = true; node_flags.disable_legacy_bootstrap = true; node_flags.disable_ascending_bootstrap = true; @@ -1368,8 +1362,11 @@ TEST (bootstrap_processor, lazy_pruning_missing_block) .work (*system.work.generate (key2.pub)) .build (); + // add the blocks without starting elections because elections publish blocks + // and the publishing would interefere with the testing ASSERT_TRUE (nano::test::process (*node1, { send1, send2, open, state_open })); - ASSERT_TRUE (nano::test::start_elections (system, *node1, { send1, send2, open, state_open }, true)); + ASSERT_TIMELY (5s, nano::test::exists (*node1, { send1, send2, open, state_open })); + nano::test::confirm (node1->ledger, { send1, send2, open, state_open }); ASSERT_TIMELY (5s, nano::test::confirmed (*node1, { send1, send2, open, state_open })); ASSERT_EQ (5, node1->ledger.block_count ()); ASSERT_EQ (5, node1->ledger.cemented_count ()); From 1ac5ac0cb6d9d315675ad0215b52d860ab2be2c1 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Mon, 22 Apr 2024 16:39:02 +0100 Subject: [PATCH 5/6] fix comment --- nano/secure/ledger.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nano/secure/ledger.hpp b/nano/secure/ledger.hpp index a309f6e5a8..dfa0591235 100644 --- a/nano/secure/ledger.hpp +++ b/nano/secure/ledger.hpp @@ -117,7 +117,7 @@ class ledger final std::optional> receivable_lower_bound (secure::transaction const & tx, nano::account const & account, nano::block_hash const & hash) const; void initialize (nano::generate_cache_flags const &); -public: // for unit tests only +public: // making this function public so that it is accessible from unit tests void confirm (secure::write_transaction const & transaction, nano::block const & block); }; } From d886a19e2ed8fedbc070c2744a77567d7d1737cb Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Tue, 23 Apr 2024 13:11:51 +0100 Subject: [PATCH 6/6] Improvements after review --- nano/core_test/bootstrap.cpp | 9 +++++---- nano/secure/ledger.cpp | 6 ++++++ nano/secure/ledger.hpp | 5 +++-- nano/test_common/testutil.cpp | 6 +++--- nano/test_common/testutil.hpp | 5 +++-- 5 files changed, 20 insertions(+), 11 deletions(-) diff --git a/nano/core_test/bootstrap.cpp b/nano/core_test/bootstrap.cpp index 5bd6fc051d..444f5a1650 100644 --- a/nano/core_test/bootstrap.cpp +++ b/nano/core_test/bootstrap.cpp @@ -1364,10 +1364,11 @@ TEST (bootstrap_processor, lazy_pruning_missing_block) // add the blocks without starting elections because elections publish blocks // and the publishing would interefere with the testing - ASSERT_TRUE (nano::test::process (*node1, { send1, send2, open, state_open })); - ASSERT_TIMELY (5s, nano::test::exists (*node1, { send1, send2, open, state_open })); - nano::test::confirm (node1->ledger, { send1, send2, open, state_open }); - ASSERT_TIMELY (5s, nano::test::confirmed (*node1, { send1, send2, open, state_open })); + std::vector> const blocks{ send1, send2, open, state_open }; + ASSERT_TRUE (nano::test::process (*node1, blocks)); + ASSERT_TIMELY (5s, nano::test::exists (*node1, blocks)); + nano::test::force_confirm (node1->ledger, blocks); + ASSERT_TIMELY (5s, nano::test::confirmed (*node1, blocks)); ASSERT_EQ (5, node1->ledger.block_count ()); ASSERT_EQ (5, node1->ledger.cemented_count ()); diff --git a/nano/secure/ledger.cpp b/nano/secure/ledger.cpp index 9be416b241..004c872b3e 100644 --- a/nano/secure/ledger.cpp +++ b/nano/secure/ledger.cpp @@ -1594,3 +1594,9 @@ std::unique_ptr nano::ledger::collect_container_ composite->add_component (cache.rep_weights.collect_container_info ("rep_weights")); return composite; } + +void nano::ledger::force_confirm (secure::write_transaction const & transaction, nano::block const & block) +{ + release_assert (*constants.genesis == *constants.nano_dev_genesis); + confirm (transaction, block); +} \ No newline at end of file diff --git a/nano/secure/ledger.hpp b/nano/secure/ledger.hpp index dfa0591235..db7a7a6ae1 100644 --- a/nano/secure/ledger.hpp +++ b/nano/secure/ledger.hpp @@ -116,8 +116,9 @@ class ledger final // Returns the next receivable entry equal or greater than 'key' std::optional> receivable_lower_bound (secure::transaction const & tx, nano::account const & account, nano::block_hash const & hash) const; void initialize (nano::generate_cache_flags const &); - -public: // making this function public so that it is accessible from unit tests void confirm (secure::write_transaction const & transaction, nano::block const & block); + +public: // Only used in tests + void force_confirm (secure::write_transaction const & transaction, nano::block const & block); }; } diff --git a/nano/test_common/testutil.cpp b/nano/test_common/testutil.cpp index 3fd3bbf07a..1862ea1eff 100644 --- a/nano/test_common/testutil.cpp +++ b/nano/test_common/testutil.cpp @@ -121,11 +121,11 @@ bool nano::test::exists (nano::node & node, std::vector> blocks) +void nano::test::force_confirm (nano::ledger & ledger, std::vector> const blocks) { - for (auto block : blocks) + for (auto const block : blocks) { - ledger.confirm (ledger.tx_begin_write (), *block); + ledger.force_confirm (ledger.tx_begin_write (), *block); } } diff --git a/nano/test_common/testutil.hpp b/nano/test_common/testutil.hpp index ce1ee83806..f23e1007a4 100644 --- a/nano/test_common/testutil.hpp +++ b/nano/test_common/testutil.hpp @@ -323,9 +323,10 @@ namespace test bool exists (nano::node & node, std::vector> blocks); /* * Convenience function to confirm/cement a block in the ledger by setting the confirmation - * height of the account to be the height of the block. This could cement more than the block given. + * height of the account to be the height of the block. + * The blocks are confirmed in the order that they are given. */ - void confirm (nano::ledger & ledger, std::vector> blocks); + void force_confirm (nano::ledger & ledger, std::vector> const blocks); /* * Convenience function to check whether *all* of the hashes exists in node ledger or in the pruned table. * @return true if all blocks are fully processed and inserted in the ledger, false otherwise