Skip to content

Commit

Permalink
Changes the semantics of election::confirmed to return if the block i…
Browse files Browse the repository at this point in the history 
…s durably confirmed on disk, rather than simply being confirmed in memory. This was causing subtle race conditions or unnecessary extra checking to ensure the confirmation has been committed to disk. (nanocurrency#4200)

Changes the semantics of election::confirmed to return if the block is durably confirmed on disk, rather than simply being confirmed in memory. This was causing subtle race conditions or unnecessary extra checking to ensure the confirmation has been committed to disk.

Modifies tests:
active_transactions.dropped_cleanup
active_transactions.inactive_votes_cache_election_start
active_transactions.republish_winner
confirmation_height.conflict_rollback_cemented
election.quorum_minimum_confirm_success
election.quorum_minimum_update_weight_before_quorum_checks
node.rollback_gap_source
rpc.confirmation_active
vote_processor.invalid_signature

Rewriting test to use a block that is not confirmed on disk.
Cleaning up confirmation_height.conflict_rollback_cemented and reduce complexity.
Cleaning up election.quorum_minimum_confirm_success and fixing race condition where confirmation happens asynchronously.
Fixing race condition in election.quorum_minimum_update_weight_before_quorum_checks when checking asynchronous election confirmation.
Cleaning up and simplifying node.rollback_gap_source
Removing some test calls to block_processor::flush which is being phased out.
  • Loading branch information
@clemahieu
clemahieu committed Mar 28, 2023
1 parent c288bb1 commit fde815f
Show file tree
Hide file tree
Showing 9 changed files with 105 additions and 162 deletions.
26 changes: 12 additions & 14 deletions nano/core_test/active_transactions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,15 +513,11 @@ TEST (active_transactions, inactive_votes_cache_election_start)
ASSERT_TRUE (send4_cache);
ASSERT_EQ (3, send4_cache->voters.size ());
node.process_active (send3);
node.block_processor.flush ();
// An election is started for send6 but does not confirm
ASSERT_TIMELY (5s, 1 == node.active.size ());
node.vote_processor.flush ();
// An election is started for send6 but does not
ASSERT_FALSE (node.block_confirmed_or_being_confirmed (send3->hash ()));
// send7 cannot be voted on but an election should be started from inactive votes
ASSERT_FALSE (node.ledger.dependents_confirmed (node.store.tx_begin_read (), *send4));
node.process_active (send4);
node.block_processor.flush ();
ASSERT_TIMELY (5s, 7 == node.ledger.cache.cemented_count);
}

Expand Down Expand Up @@ -622,26 +618,28 @@ TEST (active_transactions, dropped_cleanup)
nano::node_flags flags;
flags.disable_request_loop = true;
auto & node (*system.add_node (flags));
auto chain = nano::test::setup_chain (system, node, 1, nano::dev::genesis_key, false);
auto hash = chain[0]->hash ();

// Add to network filter to ensure proper cleanup after the election is dropped
std::vector<uint8_t> block_bytes;
{
nano::vectorstream stream (block_bytes);
nano::dev::genesis->serialize (stream);
chain[0]->serialize (stream);
}
ASSERT_FALSE (node.network.publish_filter.apply (block_bytes.data (), block_bytes.size ()));
ASSERT_TRUE (node.network.publish_filter.apply (block_bytes.data (), block_bytes.size ()));

auto election = nano::test::start_election (system, node, nano::dev::genesis->hash ());
auto election = nano::test::start_election (system, node, hash);
ASSERT_NE (nullptr, election);

// Not yet removed
ASSERT_TRUE (node.network.publish_filter.apply (block_bytes.data (), block_bytes.size ()));
ASSERT_TRUE (node.active.active (nano::dev::genesis->hash ()));
ASSERT_TRUE (node.active.active (hash));

// Now simulate dropping the election
ASSERT_FALSE (election->confirmed ());
node.active.erase (*nano::dev::genesis);
node.active.erase (*chain[0]);

// The filter must have been cleared
ASSERT_FALSE (node.network.publish_filter.apply (block_bytes.data (), block_bytes.size ()));
Expand All @@ -650,16 +648,16 @@ TEST (active_transactions, dropped_cleanup)
ASSERT_EQ (1, node.stats.count (nano::stat::type::active_dropped, nano::stat::detail::normal));

// Block cleared from active
ASSERT_FALSE (node.active.active (nano::dev::genesis->hash ()));
ASSERT_FALSE (node.active.active (hash));

// Repeat test for a confirmed election
ASSERT_TRUE (node.network.publish_filter.apply (block_bytes.data (), block_bytes.size ()));

election = nano::test::start_election (system, node, nano::dev::genesis->hash ());
election = nano::test::start_election (system, node, hash);
ASSERT_NE (nullptr, election);
election->force_confirm ();
ASSERT_TRUE (election->confirmed ());
node.active.erase (*nano::dev::genesis);
ASSERT_TIMELY (5s, election->confirmed ());
node.active.erase (*chain[0]);

// The filter should not have been cleared
ASSERT_TRUE (node.network.publish_filter.apply (block_bytes.data (), block_bytes.size ()));
Expand All @@ -668,7 +666,7 @@ TEST (active_transactions, dropped_cleanup)
ASSERT_EQ (1, node.stats.count (nano::stat::type::active_dropped, nano::stat::detail::normal));

// Block cleared from active
ASSERT_FALSE (node.active.active (nano::dev::genesis->hash ()));
ASSERT_FALSE (node.active.active (hash));
}

TEST (active_transactions, republish_winner)
Expand Down
107 changes: 31 additions & 76 deletions nano/core_test/confirmation_height.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1092,98 +1092,53 @@ TEST (confirmation_height, all_block_types)
test_mode (nano::confirmation_height_mode::unbounded);
}

// this test cements a block on one node and another block on another node
// it therefore tests that once a block is confirmed it cannot be rolled back
// and if both nodes have different branches of the fork cemented then it is a permanent fork
// This test ensures a block that's cemented cannot be rolled back by the node
// A block is inserted and confirmed then later a different block is force inserted with a rollback attempt
TEST (confirmation_height, conflict_rollback_cemented)
{
// functor to perform the conflict_rollback_cemented test using a certain mode
auto test_mode = [] (nano::confirmation_height_mode mode_a) {
nano::state_block_builder builder{};
nano::state_block_builder builder;
auto const genesis_hash = nano::dev::genesis->hash ();

nano::test::system system{};
nano::node_flags node_flags{};
nano::test::system system;
nano::node_flags node_flags;
node_flags.confirmation_height_processor_mode = mode_a;

// create node 1 and account key1 (no voting key yet)
auto node1 = system.add_node (node_flags);
nano::keypair key1{};

nano::keypair key1;
// create one side of a forked transaction on node1
auto send1 = builder.make_block ()
.previous (genesis_hash)
.account (nano::dev::genesis_key.pub)
.representative (nano::dev::genesis_key.pub)
.link (key1.pub)
.balance (nano::dev::constants.genesis_amount - 100)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*system.work.generate (genesis_hash))
.build_shared ();
node1->process_active (send1);
ASSERT_TIMELY (5s, node1->active.election (send1->qualified_root ()) != nullptr);
auto fork1a = builder.make_block ()
.previous (genesis_hash)
.account (nano::dev::genesis_key.pub)
.representative (nano::dev::genesis_key.pub)
.link (key1.pub)
.balance (nano::dev::constants.genesis_amount - 100)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*system.work.generate (genesis_hash))
.build_shared ();
ASSERT_EQ (nano::process_result::progress, node1->process (*fork1a).code);
ASSERT_TRUE (nano::test::confirm (*node1, { fork1a }));
ASSERT_TIMELY (5s, nano::test::confirmed (*node1, { fork1a }));

// create the other side of the fork on node2
nano::keypair key2;
auto send2 = builder.make_block ()
.previous (genesis_hash)
.account (nano::dev::genesis_key.pub)
.representative (nano::dev::genesis_key.pub)
.link (key2.pub)
.balance (nano::dev::constants.genesis_amount - 100)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*system.work.generate (genesis_hash))
.build_shared ();

// create node2, with send2 pre-initialised in the ledger so that block send1 cannot possibly get in the ledger first
system.initialization_blocks.push_back (send2);
auto node2 = system.add_node (node_flags);
system.initialization_blocks.clear ();
auto wallet1 = system.wallet (0);
node2->process_active (send2);
ASSERT_TIMELY (5s, node2->active.election (send2->qualified_root ()) != nullptr);

// force confirm send2 on node2
ASSERT_TIMELY (5s, node2->ledger.store.block.get (node2->ledger.store.tx_begin_read (), send2->hash ()));
node2->process_confirmed (nano::election_status{ send2 });
ASSERT_TIMELY (5s, node2->block_confirmed (send2->hash ()));

// make node1 a voting node (it has all the voting weight)
// from now on, node1 can vote for send1 at any time
wallet1->insert_adhoc (nano::dev::genesis_key.prv);

// we expect node1 to vote for one side of the fork only, whichever side
std::shared_ptr<nano::election> election_send1_node1{};
ASSERT_EQ (send1->qualified_root (), send2->qualified_root ());
ASSERT_TIMELY (5s, (election_send1_node1 = node1->active.election (send1->qualified_root ())) != nullptr);
ASSERT_TIMELY (5s, 2 == election_send1_node1->votes ().size ());

// check that the send1 on node1 won the election and got confirmed
// this happens because send1 is seen first by node1, and therefore it already winning and it cannot replaced by send2
ASSERT_TIMELY (5s, election_send1_node1->confirmed ());
auto const winner = election_send1_node1->winner ();
ASSERT_NE (nullptr, winner);
ASSERT_EQ (*winner, *send1);
auto fork1b = builder.make_block ()
.previous (genesis_hash)
.account (nano::dev::genesis_key.pub)
.representative (nano::dev::genesis_key.pub)
.link (key2.pub) // Different destination same 'previous'
.balance (nano::dev::constants.genesis_amount - 100)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.work (*system.work.generate (genesis_hash))
.build_shared ();

node1->block_processor.force (fork1b);
// node2 already has send2 forced confirmed whilst node1 should have confirmed send1 and therefore we have a cemented fork on node2
// and node2 should print an error message on the log that it cannot rollback send2 because it is already cemented
ASSERT_TIMELY (5s, 1 == node2->stats.count (nano::stat::type::ledger, nano::stat::detail::rollback_failed));

// get the tally for election the election on node1
// we expect the winner to be send1 and we expect send1 to have "genesis balance" vote weight
auto const tally = election_send1_node1->tally ();
ASSERT_FALSE (tally.empty ());
auto const & [amount, winner_alias] = *tally.begin ();
ASSERT_EQ (*winner_alias, *send1);
ASSERT_EQ (amount, nano::dev::constants.genesis_amount - 100);

// we expect send1 to exist on node1, is that because send2 is rolled back?
ASSERT_TRUE (node1->ledger.block_or_pruned_exists (send1->hash ()));
ASSERT_FALSE (node1->ledger.block_or_pruned_exists (send2->hash ()));

// we expect only send2 to be existing on node2
ASSERT_TRUE (node2->ledger.block_or_pruned_exists (send2->hash ()));
ASSERT_FALSE (node2->ledger.block_or_pruned_exists (send1->hash ()));
[[maybe_unused]] size_t count = 0;
ASSERT_TIMELY (5s, 1 == (count = node1->stats.count (nano::stat::type::ledger, nano::stat::detail::rollback_failed)));
ASSERT_TRUE (nano::test::confirmed (*node1, { fork1a->hash () })); // fork1a should still remain after the rollback failed event
};

test_mode (nano::confirmation_height_mode::bounded);
Expand Down
25 changes: 13 additions & 12 deletions nano/core_test/election.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <nano/node/election.hpp>
#include <nano/test_common/chains.hpp>
#include <nano/test_common/system.hpp>
#include <nano/test_common/testutil.hpp>

Expand All @@ -19,7 +20,8 @@ TEST (election, construction)
TEST (election, behavior)
{
nano::test::system system (1);
auto election = nano::test::start_election (system, *system.nodes[0], nano::dev::genesis->hash ());
auto chain = nano::test::setup_chain (system, *system.nodes[0], 1, nano::dev::genesis_key, false);
auto election = nano::test::start_election (system, *system.nodes[0], chain[0]->hash ());
ASSERT_NE (nullptr, election);
ASSERT_EQ (nano::election_behavior::normal, election->behavior ());
}
Expand Down Expand Up @@ -125,10 +127,11 @@ TEST (election, quorum_minimum_flip_fail)
ASSERT_FALSE (node.block_confirmed (send2->hash ()));
}

// This test ensures blocks can be confirmed precisely at the quorum minimum
TEST (election, quorum_minimum_confirm_success)
{
nano::test::system system;
nano::node_config node_config (nano::test::get_available_port (), system.logging);
nano::node_config node_config{ nano::test::get_available_port (), system.logging };
node_config.online_weight_minimum = nano::dev::constants.genesis_amount;
node_config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled;
auto & node1 = *system.add_node (node_config);
Expand All @@ -138,24 +141,22 @@ TEST (election, quorum_minimum_confirm_success)
.account (nano::dev::genesis_key.pub)
.previous (nano::dev::genesis->hash ())
.representative (nano::dev::genesis_key.pub)
.balance (node1.online_reps.delta ())
.balance (node1.online_reps.delta ()) // Only minimum quorum remains
.link (key1.pub)
.work (0)
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
.build_shared ();
node1.work_generate_blocking (*send1);
node1.process_active (send1);
node1.block_processor.flush ();
node1.scheduler.activate (nano::dev::genesis_key.pub, node1.store.tx_begin_read ());
ASSERT_TIMELY (5s, node1.active.election (send1->qualified_root ()));
auto election = node1.active.election (send1->qualified_root ());
ASSERT_NE (nullptr, election);
ASSERT_EQ (1, election->blocks ().size ());
auto vote = nano::test::make_final_vote (nano::dev::genesis_key, { send1->hash () });
ASSERT_EQ (nano::vote_code::vote, node1.active.vote (vote));
node1.block_processor.flush ();
ASSERT_NE (nullptr, node1.block (send1->hash ()));
ASSERT_TRUE (election->confirmed ());
ASSERT_TIMELY (5s, election->confirmed ());
}

// checks that block cannot be confirmed if there is no enough votes to reach quorum
Expand Down Expand Up @@ -199,16 +200,16 @@ namespace nano
// FIXME: this test fails on rare occasions. It needs a review.
TEST (election, quorum_minimum_update_weight_before_quorum_checks)
{
nano::test::system system{};
nano::test::system system;

nano::node_config node_config{ nano::test::get_available_port (), system.logging };
node_config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled;

auto & node1 = *system.add_node (node_config);
system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);

nano::keypair key1{};
nano::send_block_builder builder{};
nano::keypair key1;
nano::send_block_builder builder;
auto const amount = ((nano::uint256_t (node_config.online_weight_minimum.number ()) * nano::online_reps::online_weight_quorum) / 100).convert_to<nano::uint128_t> () - 1;

auto const latest = node1.latest (nano::dev::genesis_key.pub);
Expand All @@ -225,7 +226,7 @@ TEST (election, quorum_minimum_update_weight_before_quorum_checks)
auto const open1 = nano::open_block_builder{}.make_block ().account (key1.pub).source (send1->hash ()).representative (key1.pub).sign (key1.prv, key1.pub).work (*system.work.generate (key1.pub)).build_shared ();
ASSERT_EQ (nano::process_result::progress, node1.process (*open1).code);

nano::keypair key2{};
nano::keypair key2;
auto const send2 = builder.make_block ()
.previous (open1->hash ())
.destination (key2.pub)
Expand All @@ -242,7 +243,7 @@ TEST (election, quorum_minimum_update_weight_before_quorum_checks)
system.wallet (1)->insert_adhoc (key1.prv);
ASSERT_TIMELY (10s, node2.ledger.cache.block_count == 4);

std::shared_ptr<nano::election> election{};
std::shared_ptr<nano::election> election;
ASSERT_TIMELY (5s, (election = node1.active.election (send1->qualified_root ())) != nullptr);
ASSERT_EQ (1, election->blocks ().size ());

Expand All @@ -262,7 +263,7 @@ TEST (election, quorum_minimum_update_weight_before_quorum_checks)
node1.online_reps.online_m = node_config.online_weight_minimum.number () + 20;
}
ASSERT_EQ (nano::vote_code::vote, node1.active.vote (vote2));
ASSERT_TRUE (election->confirmed ());
ASSERT_TIMELY (5s, election->confirmed ());
ASSERT_NE (nullptr, node1.block (send1->hash ()));
}
}
Expand Down
Loading

0 comments on commit fde815f

Please sign in to comment.