Skip to content

Commit fde815f

Browse files
clemahieuclemahieu
authored
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. (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.
1 parent c288bb1 commit fde815f

9 files changed

Lines changed: 105 additions & 162 deletions

File tree

‎nano/core_test/active_transactions.cpp‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -513,15 +513,11 @@ TEST (active_transactions, inactive_votes_cache_election_start)
513513
ASSERT_TRUE (send4_cache);
514514
ASSERT_EQ (3, send4_cache->voters.size ());
515515
node.process_active (send3);
516-
node.block_processor.flush ();
517-
// An election is started for send6 but does not confirm
518-
ASSERT_TIMELY (5s, 1 == node.active.size ());
519-
node.vote_processor.flush ();
516+
// An election is started for send6 but does not
520517
ASSERT_FALSE (node.block_confirmed_or_being_confirmed (send3->hash ()));
521518
// send7 cannot be voted on but an election should be started from inactive votes
522519
ASSERT_FALSE (node.ledger.dependents_confirmed (node.store.tx_begin_read (), *send4));
523520
node.process_active (send4);
524-
node.block_processor.flush ();
525521
ASSERT_TIMELY (5s, 7 == node.ledger.cache.cemented_count);
526522
}
527523

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

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

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

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

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

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

652650
// Block cleared from active
653-
ASSERT_FALSE (node.active.active (nano::dev::genesis->hash ()));
651+
ASSERT_FALSE (node.active.active (hash));
654652

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

658-
election = nano::test::start_election (system, node, nano::dev::genesis->hash ());
656+
election = nano::test::start_election (system, node, hash);
659657
ASSERT_NE (nullptr, election);
660658
election->force_confirm ();
661-
ASSERT_TRUE (election->confirmed ());
662-
node.active.erase (*nano::dev::genesis);
659+
ASSERT_TIMELY (5s, election->confirmed ());
660+
node.active.erase (*chain[0]);
663661

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

670668
// Block cleared from active
671-
ASSERT_FALSE (node.active.active (nano::dev::genesis->hash ()));
669+
ASSERT_FALSE (node.active.active (hash));
672670
}
673671

674672
TEST (active_transactions, republish_winner)

‎nano/core_test/confirmation_height.cpp‎

Lines changed: 31 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -1092,98 +1092,53 @@ TEST (confirmation_height, all_block_types)
10921092
test_mode (nano::confirmation_height_mode::unbounded);
10931093
}
10941094

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

1105-
nano::test::system system{};
1106-
nano::node_flags node_flags{};
1104+
nano::test::system system;
1105+
nano::node_flags node_flags;
11071106
node_flags.confirmation_height_processor_mode = mode_a;
1108-
1109-
// create node 1 and account key1 (no voting key yet)
11101107
auto node1 = system.add_node (node_flags);
1111-
nano::keypair key1{};
11121108

1109+
nano::keypair key1;
11131110
// create one side of a forked transaction on node1
1114-
auto send1 = builder.make_block ()
1115-
.previous (genesis_hash)
1116-
.account (nano::dev::genesis_key.pub)
1117-
.representative (nano::dev::genesis_key.pub)
1118-
.link (key1.pub)
1119-
.balance (nano::dev::constants.genesis_amount - 100)
1120-
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
1121-
.work (*system.work.generate (genesis_hash))
1122-
.build_shared ();
1123-
node1->process_active (send1);
1124-
ASSERT_TIMELY (5s, node1->active.election (send1->qualified_root ()) != nullptr);
1111+
auto fork1a = builder.make_block ()
1112+
.previous (genesis_hash)
1113+
.account (nano::dev::genesis_key.pub)
1114+
.representative (nano::dev::genesis_key.pub)
1115+
.link (key1.pub)
1116+
.balance (nano::dev::constants.genesis_amount - 100)
1117+
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
1118+
.work (*system.work.generate (genesis_hash))
1119+
.build_shared ();
1120+
ASSERT_EQ (nano::process_result::progress, node1->process (*fork1a).code);
1121+
ASSERT_TRUE (nano::test::confirm (*node1, { fork1a }));
1122+
ASSERT_TIMELY (5s, nano::test::confirmed (*node1, { fork1a }));
11251123

11261124
// create the other side of the fork on node2
11271125
nano::keypair key2;
1128-
auto send2 = builder.make_block ()
1129-
.previous (genesis_hash)
1130-
.account (nano::dev::genesis_key.pub)
1131-
.representative (nano::dev::genesis_key.pub)
1132-
.link (key2.pub)
1133-
.balance (nano::dev::constants.genesis_amount - 100)
1134-
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
1135-
.work (*system.work.generate (genesis_hash))
1136-
.build_shared ();
1137-
1138-
// create node2, with send2 pre-initialised in the ledger so that block send1 cannot possibly get in the ledger first
1139-
system.initialization_blocks.push_back (send2);
1140-
auto node2 = system.add_node (node_flags);
1141-
system.initialization_blocks.clear ();
1142-
auto wallet1 = system.wallet (0);
1143-
node2->process_active (send2);
1144-
ASSERT_TIMELY (5s, node2->active.election (send2->qualified_root ()) != nullptr);
1145-
1146-
// force confirm send2 on node2
1147-
ASSERT_TIMELY (5s, node2->ledger.store.block.get (node2->ledger.store.tx_begin_read (), send2->hash ()));
1148-
node2->process_confirmed (nano::election_status{ send2 });
1149-
ASSERT_TIMELY (5s, node2->block_confirmed (send2->hash ()));
1150-
1151-
// make node1 a voting node (it has all the voting weight)
1152-
// from now on, node1 can vote for send1 at any time
1153-
wallet1->insert_adhoc (nano::dev::genesis_key.prv);
1154-
1155-
// we expect node1 to vote for one side of the fork only, whichever side
1156-
std::shared_ptr<nano::election> election_send1_node1{};
1157-
ASSERT_EQ (send1->qualified_root (), send2->qualified_root ());
1158-
ASSERT_TIMELY (5s, (election_send1_node1 = node1->active.election (send1->qualified_root ())) != nullptr);
1159-
ASSERT_TIMELY (5s, 2 == election_send1_node1->votes ().size ());
1160-
1161-
// check that the send1 on node1 won the election and got confirmed
1162-
// this happens because send1 is seen first by node1, and therefore it already winning and it cannot replaced by send2
1163-
ASSERT_TIMELY (5s, election_send1_node1->confirmed ());
1164-
auto const winner = election_send1_node1->winner ();
1165-
ASSERT_NE (nullptr, winner);
1166-
ASSERT_EQ (*winner, *send1);
1126+
auto fork1b = builder.make_block ()
1127+
.previous (genesis_hash)
1128+
.account (nano::dev::genesis_key.pub)
1129+
.representative (nano::dev::genesis_key.pub)
1130+
.link (key2.pub) // Different destination same 'previous'
1131+
.balance (nano::dev::constants.genesis_amount - 100)
1132+
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
1133+
.work (*system.work.generate (genesis_hash))
1134+
.build_shared ();
11671135

1136+
node1->block_processor.force (fork1b);
11681137
// node2 already has send2 forced confirmed whilst node1 should have confirmed send1 and therefore we have a cemented fork on node2
11691138
// and node2 should print an error message on the log that it cannot rollback send2 because it is already cemented
1170-
ASSERT_TIMELY (5s, 1 == node2->stats.count (nano::stat::type::ledger, nano::stat::detail::rollback_failed));
1171-
1172-
// get the tally for election the election on node1
1173-
// we expect the winner to be send1 and we expect send1 to have "genesis balance" vote weight
1174-
auto const tally = election_send1_node1->tally ();
1175-
ASSERT_FALSE (tally.empty ());
1176-
auto const & [amount, winner_alias] = *tally.begin ();
1177-
ASSERT_EQ (*winner_alias, *send1);
1178-
ASSERT_EQ (amount, nano::dev::constants.genesis_amount - 100);
1179-
1180-
// we expect send1 to exist on node1, is that because send2 is rolled back?
1181-
ASSERT_TRUE (node1->ledger.block_or_pruned_exists (send1->hash ()));
1182-
ASSERT_FALSE (node1->ledger.block_or_pruned_exists (send2->hash ()));
1183-
1184-
// we expect only send2 to be existing on node2
1185-
ASSERT_TRUE (node2->ledger.block_or_pruned_exists (send2->hash ()));
1186-
ASSERT_FALSE (node2->ledger.block_or_pruned_exists (send1->hash ()));
1139+
[[maybe_unused]] size_t count = 0;
1140+
ASSERT_TIMELY (5s, 1 == (count = node1->stats.count (nano::stat::type::ledger, nano::stat::detail::rollback_failed)));
1141+
ASSERT_TRUE (nano::test::confirmed (*node1, { fork1a->hash () })); // fork1a should still remain after the rollback failed event
11871142
};
11881143

11891144
test_mode (nano::confirmation_height_mode::bounded);

‎nano/core_test/election.cpp‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include <nano/node/election.hpp>
2+
#include <nano/test_common/chains.hpp>
23
#include <nano/test_common/system.hpp>
34
#include <nano/test_common/testutil.hpp>
45

@@ -19,7 +20,8 @@ TEST (election, construction)
1920
TEST (election, behavior)
2021
{
2122
nano::test::system system (1);
22-
auto election = nano::test::start_election (system, *system.nodes[0], nano::dev::genesis->hash ());
23+
auto chain = nano::test::setup_chain (system, *system.nodes[0], 1, nano::dev::genesis_key, false);
24+
auto election = nano::test::start_election (system, *system.nodes[0], chain[0]->hash ());
2325
ASSERT_NE (nullptr, election);
2426
ASSERT_EQ (nano::election_behavior::normal, election->behavior ());
2527
}
@@ -125,10 +127,11 @@ TEST (election, quorum_minimum_flip_fail)
125127
ASSERT_FALSE (node.block_confirmed (send2->hash ()));
126128
}
127129

130+
// This test ensures blocks can be confirmed precisely at the quorum minimum
128131
TEST (election, quorum_minimum_confirm_success)
129132
{
130133
nano::test::system system;
131-
nano::node_config node_config (nano::test::get_available_port (), system.logging);
134+
nano::node_config node_config{ nano::test::get_available_port (), system.logging };
132135
node_config.online_weight_minimum = nano::dev::constants.genesis_amount;
133136
node_config.frontiers_confirmation = nano::frontiers_confirmation_mode::disabled;
134137
auto & node1 = *system.add_node (node_config);
@@ -138,24 +141,22 @@ TEST (election, quorum_minimum_confirm_success)
138141
.account (nano::dev::genesis_key.pub)
139142
.previous (nano::dev::genesis->hash ())
140143
.representative (nano::dev::genesis_key.pub)
141-
.balance (node1.online_reps.delta ())
144+
.balance (node1.online_reps.delta ()) // Only minimum quorum remains
142145
.link (key1.pub)
143146
.work (0)
144147
.sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub)
145148
.build_shared ();
146149
node1.work_generate_blocking (*send1);
147150
node1.process_active (send1);
148-
node1.block_processor.flush ();
149151
node1.scheduler.activate (nano::dev::genesis_key.pub, node1.store.tx_begin_read ());
150152
ASSERT_TIMELY (5s, node1.active.election (send1->qualified_root ()));
151153
auto election = node1.active.election (send1->qualified_root ());
152154
ASSERT_NE (nullptr, election);
153155
ASSERT_EQ (1, election->blocks ().size ());
154156
auto vote = nano::test::make_final_vote (nano::dev::genesis_key, { send1->hash () });
155157
ASSERT_EQ (nano::vote_code::vote, node1.active.vote (vote));
156-
node1.block_processor.flush ();
157158
ASSERT_NE (nullptr, node1.block (send1->hash ()));
158-
ASSERT_TRUE (election->confirmed ());
159+
ASSERT_TIMELY (5s, election->confirmed ());
159160
}
160161

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

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

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

210-
nano::keypair key1{};
211-
nano::send_block_builder builder{};
211+
nano::keypair key1;
212+
nano::send_block_builder builder;
212213
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;
213214

214215
auto const latest = node1.latest (nano::dev::genesis_key.pub);
@@ -225,7 +226,7 @@ TEST (election, quorum_minimum_update_weight_before_quorum_checks)
225226
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 ();
226227
ASSERT_EQ (nano::process_result::progress, node1.process (*open1).code);
227228

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

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

@@ -262,7 +263,7 @@ TEST (election, quorum_minimum_update_weight_before_quorum_checks)
262263
node1.online_reps.online_m = node_config.online_weight_minimum.number () + 20;
263264
}
264265
ASSERT_EQ (nano::vote_code::vote, node1.active.vote (vote2));
265-
ASSERT_TRUE (election->confirmed ());
266+
ASSERT_TIMELY (5s, election->confirmed ());
266267
ASSERT_NE (nullptr, node1.block (send1->hash ()));
267268
}
268269
}

0 commit comments

Comments
 (0)