From 7a2463e4478cd317f37b0481476a820a078861af Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Tue, 22 Oct 2024 11:47:39 -0400 Subject: [PATCH 1/7] Add a test to reproduce the problem of transaction not found when forking happens --- .../trace_api_plugin/test/test_trace_file.cpp | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/plugins/trace_api_plugin/test/test_trace_file.cpp b/plugins/trace_api_plugin/test/test_trace_file.cpp index 99fbafa598..50441b21f3 100644 --- a/plugins/trace_api_plugin/test/test_trace_file.cpp +++ b/plugins/trace_api_plugin/test/test_trace_file.cpp @@ -1066,5 +1066,106 @@ BOOST_AUTO_TEST_SUITE(slice_tests) BOOST_REQUIRE(!block2); } +// This test verifies the bug reported by https://github.com/AntelopeIO/spring/issues/942 +// is fixed. The bug was if the block containing a transaction forked out, +// get_trx_block_number() always returned the latest block whose block number was +// the same as the initial block's, but this latest block might not include the +// transaction anymore. + BOOST_FIXTURE_TEST_CASE(test_get_trx_block_number_forked, test_fixture) + { + chain::transaction_id_type target_trx_id = "0000000000000000000000000000000000000000000000000000000000000001"_h; + uint32_t initial_block_num = 1; + uint32_t final_block_num = 3; + + transaction_trace_v2 trx_trace1 { + target_trx_id, + actions, + fc::enum_type{chain::transaction_receipt_header::status_enum::executed}, + 10, + 5, + { chain::signature_type() }, + { chain::time_point_sec(), 1, 0, 100, 50, 0 } + }; + + transaction_trace_v2 trx_trace2 { + "0000000000000000000000000000000000000000000000000000000000000002"_h, + actions, + fc::enum_type{chain::transaction_receipt_header::status_enum::executed}, + 10, + 5, + { chain::signature_type() }, + { chain::time_point_sec(), 1, 0, 100, 50, 0 } + }; + + // Initial block including trx_trace1 + block_trace_v2 initial_block_trace { + "b000000000000000000000000000000000000000000000000000000000000001"_h, + initial_block_num, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + chain::block_timestamp_type(0), + "test"_n, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + 0, + std::vector { + trx_trace1 + } + }; + + // Initial block is forked. The original trx_trace1 is forked out and + // replaced by trx_trace2. + block_trace_v2 forked_block_trace = initial_block_trace; + forked_block_trace.transactions = std::vector { trx_trace2 }; + + // Final block including original trx_trace1 + block_trace_v2 final_block_trace { + "b000000000000000000000000000000000000000000000000000000000000003"_h, + final_block_num, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + chain::block_timestamp_type(0), + "test"_n, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + 0, + std::vector { + trx_trace1 + } + }; + + block_trxs_entry initial_block_trxs_entry { + .ids = {target_trx_id}, + .block_num = initial_block_num + }; + + block_trxs_entry final_block_trxs_entry { + .ids = {target_trx_id}, + .block_num = final_block_num + }; + + fc::temp_directory tempdir; + store_provider sp(tempdir.path(), 100, std::optional(), std::optional(), 0); + + // on_accepted_block of the initial block + sp.append(initial_block_trace); + sp.append_trx_ids(initial_block_trxs_entry); + + // initial block forks out + sp.append(forked_block_trace); + + // forked block becomes final + sp.append_lib(initial_block_num); + + // on_accepted_block of the final block + sp.append(final_block_trace); + sp.append_trx_ids(final_block_trxs_entry); + + // final block becomes final + sp.append_lib(final_block_num); + + get_block_n block_num = sp.get_trx_block_number(target_trx_id, {}); + + BOOST_REQUIRE(block_num); + BOOST_REQUIRE_EQUAL(*block_num, final_block_num); // target trx is in final block + } BOOST_AUTO_TEST_SUITE_END() From 0bfc09e0f4d7e2e922d518190b8ec3d5c2015dee Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Tue, 22 Oct 2024 11:48:21 -0400 Subject: [PATCH 2/7] Fix the problem of transaction not found when forking happens --- plugins/trace_api_plugin/store_provider.cpp | 49 ++++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/plugins/trace_api_plugin/store_provider.cpp b/plugins/trace_api_plugin/store_provider.cpp index ae42e843da..4c2085e899 100644 --- a/plugins/trace_api_plugin/store_provider.cpp +++ b/plugins/trace_api_plugin/store_provider.cpp @@ -98,6 +98,43 @@ namespace eosio::trace_api { return std::make_tuple( entry.value(), irreversible ); } + // Returns true if `block` includes the transaction identified by `trx_id` + static bool block_has_trx(const get_block_t& block, const chain::transaction_id_type& trx_id) { + bool has_trx = false; + data_log_entry block_trace = std::get<0>(*block); + + if (std::holds_alternative (block_trace)) { + auto& trace_v0 = std::get(block_trace); + auto itr = std::find_if(trace_v0.transactions.begin(), trace_v0.transactions.end(), + [&](const auto& trx) { return trx.id == trx_id; }); + has_trx = (itr != trace_v0.transactions.end()); + } else if (std::holds_alternative(block_trace)) { + auto& trace_v1 = std::get(block_trace); + auto itr = std::find_if(trace_v1.transactions_v1.begin(), trace_v1.transactions_v1.end(), + [&](const auto& trx) { return trx.id == trx_id; }); + has_trx = (itr != trace_v1.transactions_v1.end()); + } else if (std::holds_alternative(block_trace)) { + auto& trace_v2 = std::get(block_trace); + if (std::holds_alternative>(trace_v2.transactions)) { + auto& trxs = std::get>(trace_v2.transactions); + auto itr = std::find_if(trxs.begin(), trxs.end(), + [&](const auto& trx) { return trx.id == trx_id; }); + has_trx = (itr != trxs.end()); + } else if (std::holds_alternative>(trace_v2.transactions)) { + auto& trxs = std::get>(trace_v2.transactions); + auto itr = std::find_if(trxs.begin(), trxs.end(), + [&](const auto& trx) { return trx.id == trx_id; }); + has_trx = (itr != trxs.end()); + } else { + FC_ASSERT( false, "block_trace_v2 should contain transaction_trace_v2 or transaction_trace_v3 transactions" );; + } + } else { + FC_ASSERT( false, "block_trace should be a block_trace_v0, block_trace_v1, or a block_trace_v3" ); + } + + return has_trx; + } + get_block_n store_provider::get_trx_block_number(const chain::transaction_id_type& trx_id, std::optional minimum_irreversible_history_blocks, const yield_function& yield) { fc::cfile trx_id_file; int32_t slice_number; @@ -125,8 +162,16 @@ namespace eosio::trace_api { const auto& trxs_entry = std::get(entry); for (auto i = 0U; i < trxs_entry.ids.size(); ++i) { if (trxs_entry.ids[i] == trx_id) { - trx_entries++; - trx_block_num = trxs_entry.block_num; + // Update trx_block_num only if the block contains the transaction. + // This is to prevent the problem where the initial block containing + // the transaction forks out and the transaction is not in the latest + // block which has the same block number as the initial block. + + get_block_t block = get_block(trxs_entry.block_num); + if (block && block_has_trx(block, trx_id)) { + trx_entries++; + trx_block_num = trxs_entry.block_num; + } } } } else if (std::holds_alternative(entry)) { From 2ee402b4df67bbf6ea1391c24dd0b8e523a91560 Mon Sep 17 00:00:00 2001 From: Kevin Heifner Date: Tue, 22 Oct 2024 15:10:54 -0500 Subject: [PATCH 3/7] Alternative implementation of handling forking --- plugins/trace_api_plugin/store_provider.cpp | 28 ++++++++----------- .../trace_api_plugin/test/test_trace_file.cpp | 20 ++++++++----- 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/plugins/trace_api_plugin/store_provider.cpp b/plugins/trace_api_plugin/store_provider.cpp index 4c2085e899..2998bac277 100644 --- a/plugins/trace_api_plugin/store_provider.cpp +++ b/plugins/trace_api_plugin/store_provider.cpp @@ -144,8 +144,7 @@ namespace eosio::trace_api { slice_number = 0; } - uint32_t trx_block_num = 0; // number of the block that contains the target trx - uint32_t trx_entries = 0; // number of entries that contain the target trx + std::set trx_block_nums; while (true){ const bool found = _slice_directory.find_trx_id_slice(slice_number, open_state::read, trx_id_file); if( !found ) @@ -160,24 +159,21 @@ namespace eosio::trace_api { fc::raw::unpack(ds, entry); if (std::holds_alternative(entry)) { const auto& trxs_entry = std::get(entry); + bool found_in_block = false; for (auto i = 0U; i < trxs_entry.ids.size(); ++i) { if (trxs_entry.ids[i] == trx_id) { - // Update trx_block_num only if the block contains the transaction. - // This is to prevent the problem where the initial block containing - // the transaction forks out and the transaction is not in the latest - // block which has the same block number as the initial block. - - get_block_t block = get_block(trxs_entry.block_num); - if (block && block_has_trx(block, trx_id)) { - trx_entries++; - trx_block_num = trxs_entry.block_num; - } + trx_block_nums.insert(trxs_entry.block_num); + found_in_block = true; + break; } } + // block can be seen again when a fork happens, if not in the new block remove it from blocks that have the trx + if (!found_in_block) + trx_block_nums.erase(trxs_entry.block_num); } else if (std::holds_alternative(entry)) { auto lib = std::get(entry).lib; - if (trx_entries > 0 && lib >= trx_block_num) { - return trx_block_num; + if (!trx_block_nums.empty() && lib >= *(--trx_block_nums.end())) { + return *(--trx_block_nums.end()); } } else { FC_ASSERT( false, "unpacked data should be a block_trxs_entry or a lib_entry_v0" );; @@ -188,8 +184,8 @@ namespace eosio::trace_api { } // transaction's block is not irreversible - if (trx_entries > 0) - return trx_block_num; + if (!trx_block_nums.empty()) + return *(--trx_block_nums.end()); return get_block_n{}; } diff --git a/plugins/trace_api_plugin/test/test_trace_file.cpp b/plugins/trace_api_plugin/test/test_trace_file.cpp index 50441b21f3..e1b8f1fa30 100644 --- a/plugins/trace_api_plugin/test/test_trace_file.cpp +++ b/plugins/trace_api_plugin/test/test_trace_file.cpp @@ -1137,6 +1137,11 @@ BOOST_AUTO_TEST_SUITE(slice_tests) .block_num = initial_block_num }; + block_trxs_entry forked_block_trxs_entry { + .ids = {trx_trace2.id}, + .block_num = initial_block_num + }; + block_trxs_entry final_block_trxs_entry { .ids = {target_trx_id}, .block_num = final_block_num @@ -1146,21 +1151,22 @@ BOOST_AUTO_TEST_SUITE(slice_tests) store_provider sp(tempdir.path(), 100, std::optional(), std::optional(), 0); // on_accepted_block of the initial block - sp.append(initial_block_trace); - sp.append_trx_ids(initial_block_trxs_entry); + sp.append(initial_block_trace); // block 1 + sp.append_trx_ids(initial_block_trxs_entry); // block 1 // initial block forks out - sp.append(forked_block_trace); + sp.append(forked_block_trace); // block 1 + sp.append_trx_ids(forked_block_trxs_entry); // block 1 // forked block becomes final - sp.append_lib(initial_block_num); + sp.append_lib(initial_block_num); // block 1 // on_accepted_block of the final block - sp.append(final_block_trace); - sp.append_trx_ids(final_block_trxs_entry); + sp.append(final_block_trace); // block 3 + sp.append_trx_ids(final_block_trxs_entry); // block 3 // final block becomes final - sp.append_lib(final_block_num); + sp.append_lib(final_block_num); // block 3 get_block_n block_num = sp.get_trx_block_number(target_trx_id, {}); From 9bfa82e20e466bc70023c0d8d7f09c98bb35a803 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Tue, 22 Oct 2024 16:46:44 -0400 Subject: [PATCH 4/7] remove unneeded block_has_trx() --- plugins/trace_api_plugin/store_provider.cpp | 37 --------------------- 1 file changed, 37 deletions(-) diff --git a/plugins/trace_api_plugin/store_provider.cpp b/plugins/trace_api_plugin/store_provider.cpp index 2998bac277..b3f0ea2c99 100644 --- a/plugins/trace_api_plugin/store_provider.cpp +++ b/plugins/trace_api_plugin/store_provider.cpp @@ -98,43 +98,6 @@ namespace eosio::trace_api { return std::make_tuple( entry.value(), irreversible ); } - // Returns true if `block` includes the transaction identified by `trx_id` - static bool block_has_trx(const get_block_t& block, const chain::transaction_id_type& trx_id) { - bool has_trx = false; - data_log_entry block_trace = std::get<0>(*block); - - if (std::holds_alternative (block_trace)) { - auto& trace_v0 = std::get(block_trace); - auto itr = std::find_if(trace_v0.transactions.begin(), trace_v0.transactions.end(), - [&](const auto& trx) { return trx.id == trx_id; }); - has_trx = (itr != trace_v0.transactions.end()); - } else if (std::holds_alternative(block_trace)) { - auto& trace_v1 = std::get(block_trace); - auto itr = std::find_if(trace_v1.transactions_v1.begin(), trace_v1.transactions_v1.end(), - [&](const auto& trx) { return trx.id == trx_id; }); - has_trx = (itr != trace_v1.transactions_v1.end()); - } else if (std::holds_alternative(block_trace)) { - auto& trace_v2 = std::get(block_trace); - if (std::holds_alternative>(trace_v2.transactions)) { - auto& trxs = std::get>(trace_v2.transactions); - auto itr = std::find_if(trxs.begin(), trxs.end(), - [&](const auto& trx) { return trx.id == trx_id; }); - has_trx = (itr != trxs.end()); - } else if (std::holds_alternative>(trace_v2.transactions)) { - auto& trxs = std::get>(trace_v2.transactions); - auto itr = std::find_if(trxs.begin(), trxs.end(), - [&](const auto& trx) { return trx.id == trx_id; }); - has_trx = (itr != trxs.end()); - } else { - FC_ASSERT( false, "block_trace_v2 should contain transaction_trace_v2 or transaction_trace_v3 transactions" );; - } - } else { - FC_ASSERT( false, "block_trace should be a block_trace_v0, block_trace_v1, or a block_trace_v3" ); - } - - return has_trx; - } - get_block_n store_provider::get_trx_block_number(const chain::transaction_id_type& trx_id, std::optional minimum_irreversible_history_blocks, const yield_function& yield) { fc::cfile trx_id_file; int32_t slice_number; From c0f88375c1c5c4d56ff308162e34ec050ecd9131 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Tue, 22 Oct 2024 16:50:19 -0400 Subject: [PATCH 5/7] Add a comment about highest block number --- plugins/trace_api_plugin/store_provider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/trace_api_plugin/store_provider.cpp b/plugins/trace_api_plugin/store_provider.cpp index b3f0ea2c99..a924180c59 100644 --- a/plugins/trace_api_plugin/store_provider.cpp +++ b/plugins/trace_api_plugin/store_provider.cpp @@ -136,7 +136,7 @@ namespace eosio::trace_api { } else if (std::holds_alternative(entry)) { auto lib = std::get(entry).lib; if (!trx_block_nums.empty() && lib >= *(--trx_block_nums.end())) { - return *(--trx_block_nums.end()); + return *(--trx_block_nums.end()); // *(--trx_block_nums.end()) is the block with highest block number which is final } } else { FC_ASSERT( false, "unpacked data should be a block_trxs_entry or a lib_entry_v0" );; From 158338725151ef6a17af38524e1656c4bab041c2 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Tue, 22 Oct 2024 17:17:26 -0400 Subject: [PATCH 6/7] Verify get_trx_block_number() result after every major step --- plugins/trace_api_plugin/test/test_trace_file.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/plugins/trace_api_plugin/test/test_trace_file.cpp b/plugins/trace_api_plugin/test/test_trace_file.cpp index e1b8f1fa30..4637085be2 100644 --- a/plugins/trace_api_plugin/test/test_trace_file.cpp +++ b/plugins/trace_api_plugin/test/test_trace_file.cpp @@ -1154,6 +1154,11 @@ BOOST_AUTO_TEST_SUITE(slice_tests) sp.append(initial_block_trace); // block 1 sp.append_trx_ids(initial_block_trxs_entry); // block 1 + // target trx is in the first block (is still reversible) + get_block_n block_num = sp.get_trx_block_number(target_trx_id, {}); + BOOST_REQUIRE(block_num); + BOOST_REQUIRE_EQUAL(*block_num, initial_block_num); + // initial block forks out sp.append(forked_block_trace); // block 1 sp.append_trx_ids(forked_block_trxs_entry); // block 1 @@ -1161,6 +1166,12 @@ BOOST_AUTO_TEST_SUITE(slice_tests) // forked block becomes final sp.append_lib(initial_block_num); // block 1 + // target trx is forked out. block 1 does not include + // target_trx (trx_trace1) but trx_trace2; + // therefore no block is found for target_trx_id. + block_num = sp.get_trx_block_number(target_trx_id, {}); + BOOST_REQUIRE(!block_num); + // on_accepted_block of the final block sp.append(final_block_trace); // block 3 sp.append_trx_ids(final_block_trxs_entry); // block 3 @@ -1168,8 +1179,7 @@ BOOST_AUTO_TEST_SUITE(slice_tests) // final block becomes final sp.append_lib(final_block_num); // block 3 - get_block_n block_num = sp.get_trx_block_number(target_trx_id, {}); - + block_num = sp.get_trx_block_number(target_trx_id, {}); BOOST_REQUIRE(block_num); BOOST_REQUIRE_EQUAL(*block_num, final_block_num); // target trx is in final block } From e850b108c467baf855822451597e5ab9703f31a2 Mon Sep 17 00:00:00 2001 From: Lin Huang Date: Tue, 22 Oct 2024 18:01:40 -0400 Subject: [PATCH 7/7] Add test_get_trx_block_number_basic to verify normal flow --- .../trace_api_plugin/test/test_trace_file.cpp | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/plugins/trace_api_plugin/test/test_trace_file.cpp b/plugins/trace_api_plugin/test/test_trace_file.cpp index 4637085be2..64f3690916 100644 --- a/plugins/trace_api_plugin/test/test_trace_file.cpp +++ b/plugins/trace_api_plugin/test/test_trace_file.cpp @@ -1066,6 +1066,118 @@ BOOST_AUTO_TEST_SUITE(slice_tests) BOOST_REQUIRE(!block2); } +// Verify basics of get_trx_block_number() + BOOST_FIXTURE_TEST_CASE(test_get_trx_block_number_basic, test_fixture) + { + chain::transaction_id_type trx_id1 = "0000000000000000000000000000000000000000000000000000000000000001"_h; + chain::transaction_id_type trx_id2 = "0000000000000000000000000000000000000000000000000000000000000002"_h; + uint32_t block_num1 = 1; + uint32_t block_num2 = 2; + + transaction_trace_v2 trx_trace1 { + trx_id1, + actions, + fc::enum_type{chain::transaction_receipt_header::status_enum::executed}, + 10, + 5, + { chain::signature_type() }, + { chain::time_point_sec(), 1, 0, 100, 50, 0 } + }; + + transaction_trace_v2 trx_trace2 { + trx_id2, + actions, + fc::enum_type{chain::transaction_receipt_header::status_enum::executed}, + 10, + 5, + { chain::signature_type() }, + { chain::time_point_sec(), 1, 0, 100, 50, 0 } + }; + + // block 1 includes trx_trace1 + block_trace_v2 block_trace1 { + "b000000000000000000000000000000000000000000000000000000000000001"_h, + block_num1, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + chain::block_timestamp_type(0), + "test"_n, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + 0, + std::vector { + trx_trace1 + } + }; + + // block 2 includes trx_trace2 + block_trace_v2 block_trace2 { + "b000000000000000000000000000000000000000000000000000000000000003"_h, + block_num2, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + chain::block_timestamp_type(0), + "test"_n, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + "0000000000000000000000000000000000000000000000000000000000000000"_h, + 0, + std::vector { + trx_trace2 + } + }; + + block_trxs_entry block_trxs_entry1 { + .ids = {trx_id1}, + .block_num = block_num1 + }; + + block_trxs_entry block_trxs_entry2 { + .ids = {trx_id2}, + .block_num = block_num2 + }; + + fc::temp_directory tempdir; + store_provider sp(tempdir.path(), 100, std::optional(), std::optional(), 0); + + // on_accepted_block of block 1 + sp.append(block_trace1); + sp.append_trx_ids(block_trxs_entry1); + + // block 1 is reversible and get_trx_block_number should find trx_id1 in block 1 + get_block_n block_num = sp.get_trx_block_number(trx_id1, {}); + BOOST_REQUIRE(block_num); + BOOST_REQUIRE_EQUAL(*block_num, block_num1); + + // block 1 becomes final + sp.append_lib(block_num1); + + // get_trx_block_number should find trx_id1 in block 1 + block_num = sp.get_trx_block_number(trx_id1, {}); + BOOST_REQUIRE(block_num); + BOOST_REQUIRE_EQUAL(*block_num, block_num1); + + // on_accepted_block of block 2 + sp.append(block_trace2); + sp.append_trx_ids(block_trxs_entry2); + + // get_trx_block_number should find both trx_id1 and trx_id2 + block_num = sp.get_trx_block_number(trx_id1, {}); + BOOST_REQUIRE(block_num); + BOOST_REQUIRE_EQUAL(*block_num, block_num1); + block_num = sp.get_trx_block_number(trx_id2, {}); + BOOST_REQUIRE(block_num); + BOOST_REQUIRE_EQUAL(*block_num, block_num2); + + // block 2 becomes final + sp.append_lib(block_num2); + + // get_trx_block_number should still find both trx_id1 and trx_id2 + block_num = sp.get_trx_block_number(trx_id1, {}); + BOOST_REQUIRE(block_num); + BOOST_REQUIRE_EQUAL(*block_num, block_num1); + block_num = sp.get_trx_block_number(trx_id2, {}); + BOOST_REQUIRE(block_num); + BOOST_REQUIRE_EQUAL(*block_num, block_num2); + } + // This test verifies the bug reported by https://github.com/AntelopeIO/spring/issues/942 // is fixed. The bug was if the block containing a transaction forked out, // get_trx_block_number() always returned the latest block whose block number was