From 960ac727242c64627fb782ce0b545490400ac4c4 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Tue, 31 Jan 2023 04:09:28 +0000 Subject: [PATCH 1/4] Tidy up test case --- nano/core_test/node.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 738f6cb358..e42b81bb52 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -623,9 +623,12 @@ TEST (node, fork_publish) TEST (node, DISABLED_fork_publish_inactive) { nano::test::system system (1); + auto & node = *system.nodes[0]; nano::keypair key1; nano::keypair key2; + nano::send_block_builder builder; + auto send1 = builder.make_block () .previous (nano::dev::genesis->hash ()) .destination (key1.pub) @@ -633,6 +636,7 @@ TEST (node, DISABLED_fork_publish_inactive) .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) .work (*system.work.generate (nano::dev::genesis->hash ())) .build_shared (); + auto send2 = builder.make_block () .previous (nano::dev::genesis->hash ()) .destination (key2.pub) @@ -640,9 +644,10 @@ TEST (node, DISABLED_fork_publish_inactive) .sign (nano::dev::genesis_key.prv, nano::dev::genesis_key.pub) .work (send1->block_work ()) .build_shared (); - auto & node = *system.nodes[0]; + node.process_active (send1); - ASSERT_TIMELY (3s, nullptr != node.block (send1->hash ())); + ASSERT_TIMELY (5s, node.block (send1->hash ())); + ASSERT_EQ (nano::process_result::fork, node.process_local (send2).code); auto election = node.active.election (send1->qualified_root ()); ASSERT_NE (election, nullptr); From d0a29ed04e8d915fa544a1fda5544ef11cc2fb39 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Tue, 31 Jan 2023 04:11:56 +0000 Subject: [PATCH 2/4] Ensure the election is started before the forked block is processed If the election is not started, there is no where to save the forked block --- nano/core_test/node.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index e42b81bb52..94c74d4cd8 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -648,10 +648,13 @@ TEST (node, DISABLED_fork_publish_inactive) node.process_active (send1); ASSERT_TIMELY (5s, node.block (send1->hash ())); + std::shared_ptr election; + ASSERT_TIMELY (5s, election = node.active.election (send1->qualified_root ())); + ASSERT_EQ (nano::process_result::fork, node.process_local (send2).code); - auto election = node.active.election (send1->qualified_root ()); - ASSERT_NE (election, nullptr); + auto blocks = election->blocks (); + ASSERT_TIMELY_EQ (5s, blocks.size (), 2); ASSERT_NE (blocks.end (), blocks.find (send1->hash ())); ASSERT_NE (blocks.end (), blocks.find (send2->hash ())); ASSERT_EQ (election->winner ()->hash (), send1->hash ()); From 41f9706b493504fb1d3ae3ae4fa0e8fd5cedb73e Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Tue, 31 Jan 2023 04:12:43 +0000 Subject: [PATCH 3/4] Re-enable test case --- nano/core_test/node.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 94c74d4cd8..ee49e4d64c 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -617,10 +617,7 @@ TEST (node, fork_publish) ASSERT_TRUE (node0.expired ()); } -// Test disabled because it's failing intermittently. -// PR in which it got disabled: https://github.com/nanocurrency/nano-node/pull/3611 -// Issue for investigating it: https://github.com/nanocurrency/nano-node/issues/3614 -TEST (node, DISABLED_fork_publish_inactive) +TEST (node, fork_publish_inactive) { nano::test::system system (1); auto & node = *system.nodes[0]; From a187ca80454a495aa0e13b5f7bc97b072f061c6e Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Wed, 8 Feb 2023 18:04:02 +0000 Subject: [PATCH 4/4] Add comment with concern --- nano/core_test/node.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index ee49e4d64c..0d877d110c 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -617,6 +617,14 @@ TEST (node, fork_publish) ASSERT_TRUE (node0.expired ()); } +// In test case there used to be a race condition, it was worked around in:. +// https://github.com/nanocurrency/nano-node/pull/4091 +// The election and the processing of block send2 happen in parallel. +// Usually the election happens first and the send2 block is added to the election. +// However, if the send2 block is processed before the election is started then +// there is a race somewhere and the election might not notice the send2 block. +// The test case can be made to pass by ensuring the election is started before the send2 is processed. +// However, is this a problem with the test case or this is a problem with the node handling of forks? TEST (node, fork_publish_inactive) { nano::test::system system (1);