Skip to content

treat rewriting identical genesis block as no-op in state history log #507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 8, 2024
Merged

treat rewriting identical genesis block as no-op in state history log #507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions libraries/state_history/include/eosio/state_history/log.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,11 +216,8 @@ class state_history_log {
log_header_with_sizes header = {{ship_magic(ship_current_version, 0), id}, 1};
const uint32_t block_num = chain::block_header::num_from_id(header.block_id);

if(!empty()) {
if(!empty())
EOS_ASSERT(block_num <= _end_block, chain::plugin_exception, "block ${b} skips over block ${e} in ${name}", ("b", block_num)("e", _end_block)("name", log.display_path()));
if(_end_block > 2u)
EOS_ASSERT(block_num > 2u, chain::plugin_exception, "existing ship log with ${eb} blocks when starting from genesis block ${b}", ("eb", _end_block-_begin_block)("b", block_num));
}
EOS_ASSERT(block_num >= _index_begin_block, chain::plugin_exception, "block ${b} is before start block ${s} of ${name}", ("b", block_num)("s", _begin_block)("name", log.display_path()));
if(block_num == _end_block) //appending at the end of known blocks; can shortcut some checks since we have last_block_id readily available
EOS_ASSERT(prev_id == last_block_id, chain::plugin_exception, "missed a fork change in ${name}", ("name", log.display_path()));
Expand All @@ -234,6 +231,9 @@ class state_history_log {
//we don't want to re-write blocks that we already have, so check if the existing block_id recorded in the log matches and if so, bail
if(get_block_id(block_num) == id)
return;
//but if it doesn't match, and log isn't empty, ensure not writing a new genesis block to guard against accidental rewinding of the entire ship log
if(!empty())
EOS_ASSERT(block_num > 2u, chain::plugin_exception, "existing ship log with ${eb} blocks when starting from genesis block ${b}", ("eb", _end_block-_begin_block)("b", block_num));
}

ssize_t log_insert_pos = log.size();
Expand Down
22 changes: 9 additions & 13 deletions tests/ship_log.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,12 +265,6 @@ BOOST_DATA_TEST_CASE(basic_prune_test, bdata::xrange(2) * bdata::xrange(2) * bda
return e.to_detail_string().find("missed a fork change") != std::string::npos;
});

//start from genesis not allowed
BOOST_REQUIRE_EXCEPTION(t.add(2, payload_size, 'A', 'A');, eosio::chain::plugin_exception, [](const eosio::chain::plugin_exception& e) {
std::string err = e.to_detail_string();
return err.find("existing ship log") != std::string::npos && err.find("when starting from genesis block") != std::string::npos;
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay to remove this particular check: the test case is actually for pruned logs and pruned logs for this test case won't have the genesis block anyways. The test worked previously because the blocknum==2 check was so early in the logic.

The test case further down exercises the genesis block behavior.

} FC_LOG_AND_RETHROW() }

BOOST_DATA_TEST_CASE(basic_test, bdata::xrange(2) * bdata::xrange(2) * bdata::xrange(2), enable_read, reopen_on_mark, remove_index_on_reopen) { try {
Expand Down Expand Up @@ -537,18 +531,20 @@ BOOST_DATA_TEST_CASE(basic, bdata::make({2u, 333u, 578'000u, 3'123'456'789u}) ^
}

//now we're going to try writing identical blockids to the log. These should be silently swallowed as no-ops
for(unsigned i : {2u, start, start+6, end-5, end-1}) {
//but block 2 is special. If writing block 2 on a non empty log we fail as a safety precaution
for(unsigned i : {start, start+6, end-5, end-1}) {
//but block 2 is special. Writing block 2 on a non empty log will fail if the blockid is different (instead of treated it like a fork), but a
// no-op otherwise. So try a different blockid here to test that
if(i == 2u)
BOOST_REQUIRE_EXCEPTION(lc.pack_and_write_entry(fake_blockid_for_num(i), fake_blockid_for_num(i-1), [&](bio::filtering_ostreambuf& obuf) {
//different blockid
BOOST_REQUIRE_EXCEPTION(lc.pack_and_write_entry(fake_blockid_for_num(i, 0xbeef), fake_blockid_for_num(i-1), [&](bio::filtering_ostreambuf& obuf) {
FC_ASSERT(false, "should not reach here");
}),
plugin_exception,
[](const plugin_exception& e) {return e.to_detail_string().find("when starting from genesis block 2") != std::string::npos;});
else
lc.pack_and_write_entry(fake_blockid_for_num(i), fake_blockid_for_num(i-1), [&](bio::filtering_ostreambuf& obuf) {
FC_ASSERT(false, "should not reach here");
});

lc.pack_and_write_entry(fake_blockid_for_num(i), fake_blockid_for_num(i-1), [&](bio::filtering_ostreambuf& obuf) {
FC_ASSERT(false, "should not reach here");
});
}

BOOST_REQUIRE_EQUAL(lc.block_range().first, start);
Expand Down