Skip to content
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

IF: Change Fork_db root to store block state #2284

Merged
merged 3 commits into from
Mar 6, 2024
Merged

IF: Change Fork_db root to store block state #2284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a4ff71
Simplify fork_db `get_block` and root access. Simplify `create_block_…
greg7mdp Mar 6, 2024
a469ffd
Finish updating fork_db to store `block_state` as root.
greg7mdp Mar 6, 2024
2697fd0
Use enum instead of `bool` parameter for `get_block`.
greg7mdp Mar 6, 2024
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
48 changes: 14 additions & 34 deletions libraries/chain/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,7 @@ struct controller_impl {
fork_db.apply<void>([&](auto& forkdb) {
std::visit([&](const auto& bsp) {
if constexpr (std::is_same_v<std::decay_t<decltype(bsp)>, std::decay_t<decltype(forkdb.head())>>)
forkdb.reset_root(*bsp);
forkdb.reset_root(bsp);
}, chain_head.internal());
});
}
Expand Down Expand Up @@ -3310,43 +3310,23 @@ struct controller_impl {
}

// thread safe, expected to be called from thread other than the main thread
block_handle create_block_state_i( const block_id_type& id, const signed_block_ptr& b, const block_header_state& prev ) {
// Verify claim made by instant_finality_extension in block header extension and
// quorum_certificate_extension in block extension are valid.
// This is the only place the evaluation is done.
verify_qc_claim(id, b, prev);
template<class BS>
block_handle create_block_state_i( const block_id_type& id, const signed_block_ptr& b, const BS& prev ) {
constexpr bool savanna_mode = std::is_same_v<typename std::decay_t<BS>, block_state>;
if constexpr (savanna_mode) {
// Verify claim made by instant_finality_extension in block header extension and
// quorum_certificate_extension in block extension are valid.
// This is the only place the evaluation is done.
verify_qc_claim(id, b, prev);
}

auto trx_mroot = calculate_trx_merkle( b->transactions, true );
auto trx_mroot = calculate_trx_merkle( b->transactions, savanna_mode );
EOS_ASSERT( b->transaction_mroot == trx_mroot,
block_validate_exception,
"invalid block transaction merkle root ${b} != ${c}", ("b", b->transaction_mroot)("c", trx_mroot) );

const bool skip_validate_signee = false;
auto bsp = std::make_shared<block_state>(
prev,
b,
protocol_features.get_protocol_feature_set(),
[this]( block_timestamp_type timestamp,
const flat_set<digest_type>& cur_features,
const vector<digest_type>& new_features )
{ check_protocol_features( timestamp, cur_features, new_features ); },
skip_validate_signee
);

EOS_ASSERT( id == bsp->id(), block_validate_exception,
"provided id ${id} does not match calculated block id ${bid}", ("id", id)("bid", bsp->id()) );

return block_handle{bsp};
}

// thread safe, expected to be called from thread other than the main thread
block_handle create_block_state_i( const block_id_type& id, const signed_block_ptr& b, const block_header_state_legacy& prev ) {
auto trx_mroot = calculate_trx_merkle( b->transactions, false );
EOS_ASSERT( b->transaction_mroot == trx_mroot, block_validate_exception,
"invalid block transaction merkle root ${b} != ${c}", ("b", b->transaction_mroot)("c", trx_mroot) );

const bool skip_validate_signee = false;
auto bsp = std::make_shared<block_state_legacy>(
auto bsp = std::make_shared<BS>(
prev,
b,
protocol_features.get_protocol_feature_set(),
Expand All @@ -3371,7 +3351,7 @@ struct controller_impl {
auto existing = forkdb.get_block( id );
EOS_ASSERT( !existing, fork_database_exception, "we already know about this block: ${id}", ("id", id) );

auto prev = forkdb.get_block_header( b->previous );
auto prev = forkdb.get_block( b->previous, check_root_t::yes );
EOS_ASSERT( prev, unlinkable_block_exception,
"unlinkable block ${id} previous ${p}", ("id", id)("p", b->previous) );

Expand All @@ -3392,7 +3372,7 @@ struct controller_impl {
EOS_ASSERT( !existing, fork_database_exception, "we already know about this block: ${id}", ("id", id) );

// previous not found could mean that previous block not applied yet
auto prev = forkdb.get_block_header( b->previous );
auto prev = forkdb.get_block( b->previous, check_root_t::yes );
if( !prev ) return {};

return create_block_state_i( id, b, *prev );
Expand Down
58 changes: 22 additions & 36 deletions libraries/chain/fork_database.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ namespace eosio::chain {
/**
* History:
* Version 1: initial version of the new refactored fork database portable format
* Version 2: Savanna version, store either `block_state`, `block_state_legacy` or both versions,
* root is full `block_state`, not just the header.
Copy link
Member

Choose a reason for hiding this comment

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

You didn't update max_supported_version. As implemented, it looks like this implementation can read version 1. However, a version 1 written by this implementation will not be readable by leap v5 or before. Not really sure we really need to support downgrading a node back to pre leap v6. A version dump would allow for a better error message in pre leap v6 (Unsupported version of fork database file).

Copy link
Member

Choose a reason for hiding this comment

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

Note I'm working on #2141 which has the test coverage fork database writing/reading. Would really like this tested with the test cases that are being restored in #2141.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You didn't update max_supported_version. As implemented, it looks like this implementation can read version 1.

The compatibility work will be addressed in a separate issue #2285. The current version (even before this PR) is not compatible with the leap5 format either (for example if starting from a post-transition snapshot).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note I'm working on #2141 which has the test coverage fork database writing/reading. Would really like this tested with the test cases that are being restored in #2141.

This PR does not address the pre-existing file format issue which will be addressed in #2285.

*/

struct block_state_accessor {
Expand Down Expand Up @@ -118,7 +120,7 @@ namespace eosio::chain {

std::mutex mtx;
fork_multi_index_type index;
bsp_t root; // Only uses the block_header_state portion of block_state
bsp_t root;
bsp_t head;
const uint32_t magic_number;

Expand All @@ -128,10 +130,9 @@ namespace eosio::chain {
void close_impl( const std::filesystem::path& fork_db_file );
void add_impl( const bsp_t& n, mark_valid_t mark_valid, ignore_duplicate_t ignore_duplicate, bool validate, validator_t& validator );

bhsp_t get_block_header_impl( const block_id_type& id ) const;
bsp_t get_block_impl( const block_id_type& id ) const;
bsp_t get_block_impl( const block_id_type& id, check_root_t check_root = check_root_t::no ) const;
bool block_exists_impl( const block_id_type& id ) const;
void reset_root_impl( const bhs_t& root_bhs );
void reset_root_impl( const bsp_t& root_bs );
void rollback_head_to_root_impl();
void advance_root_impl( const block_id_type& id );
void remove_impl( const block_id_type& id );
Expand Down Expand Up @@ -189,8 +190,8 @@ namespace eosio::chain {
("max", fork_database::max_supported_version)
);

bhs_t state;
fc::raw::unpack( ds, state );
bsp_t state = std::make_shared<bs_t>();
fc::raw::unpack( ds, *state );
reset_root_impl( state );

unsigned_int size; fc::raw::unpack( ds, size );
Expand Down Expand Up @@ -248,7 +249,7 @@ namespace eosio::chain {
std::ofstream out( fork_db_file.generic_string().c_str(), std::ios::out | std::ios::binary | std::ofstream::trunc );
fc::raw::pack( out, magic_number );
fc::raw::pack( out, fork_database::max_supported_version ); // write out current version which is always max_supported_version
fc::raw::pack( out, *static_cast<bhs_t*>(&*root) );
fc::raw::pack( out, *root );
uint32_t num_blocks_in_fork_db = index.size();
fc::raw::pack( out, unsigned_int{num_blocks_in_fork_db} );

Expand Down Expand Up @@ -298,16 +299,15 @@ namespace eosio::chain {
}

template<class BSP>
void fork_database_t<BSP>::reset_root( const bhs_t& root_bhs ) {
void fork_database_t<BSP>::reset_root( const bsp_t& root_bsp ) {
std::lock_guard g( my->mtx );
my->reset_root_impl(root_bhs);
my->reset_root_impl(root_bsp);
}

template<class BSP>
void fork_database_impl<BSP>::reset_root_impl( const bhs_t& root_bhs ) {
void fork_database_impl<BSP>::reset_root_impl( const bsp_t& root_bsp ) {
index.clear();
root = std::make_shared<bs_t>();
static_cast<bhs_t&>(*root) = root_bhs;
root = root_bsp;
bs_accessor_t::set_valid(*root, true);
Copy link
Member

Choose a reason for hiding this comment

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

Should only set valid if this was a version 1 root_bsp from open_impl. Otherwise, it should already be valid. Could assert it is valid and set the valid in open_impl if a block_header_state was read instead of a block_state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure why, but it appears that validated is not set on root after loading a fork_db when running tests with this PR's code, se the set_valid is necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Set valid is only necessary when loading a version 1 fork database which does not have all the block_state (doesn't read in validated.). I think you should move the set valid into open_impl and assert in reset_root_impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set valid is only necessary when loading a version 1 fork database which does not have all the block_state (doesn't read in validated.). I think you should move the set valid into open_impl and assert in reset_root_impl.

I tried that but many tests fail when I do that.

Copy link
Member

Choose a reason for hiding this comment

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

Glancing at the tests, I think I know why. I can address that in #2141

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm curious to know what the issue could be?

Copy link
Member

Choose a reason for hiding this comment

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

Mock block_state rely on this to set valid. Those need to set valid outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mock block_state rely on this to set valid. Those need to set valid outside.

Thanks, that makes sense.

head = root;
}
Expand Down Expand Up @@ -372,31 +372,12 @@ namespace eosio::chain {
root = new_root;
}

template<class BSP>
fork_database_t<BSP>::bhsp_t fork_database_t<BSP>::get_block_header( const block_id_type& id ) const {
std::lock_guard g( my->mtx );
return my->get_block_header_impl( id );
}

template<class BSP>
fork_database_impl<BSP>::bhsp_t fork_database_impl<BSP>::get_block_header_impl( const block_id_type& id ) const {
if( root->id() == id ) {
return root;
}

auto itr = index.find( id );
if( itr != index.end() )
return *itr;

return bhsp_t();
}

template <class BSP>
void fork_database_impl<BSP>::add_impl(const bsp_t& n, mark_valid_t mark_valid, ignore_duplicate_t ignore_duplicate, bool validate, validator_t& validator) {
EOS_ASSERT( root, fork_database_exception, "root not yet set" );
EOS_ASSERT( n, fork_database_exception, "attempt to add null block state" );

auto prev_bh = get_block_header_impl( n->previous() );
auto prev_bh = get_block_impl( n->previous(), check_root_t::yes );
EOS_ASSERT( prev_bh, unlinkable_block_exception,
"forkdb unlinkable block ${id} previous ${p}", ("id", n->id())("p", n->previous()) );

Expand Down Expand Up @@ -684,13 +665,18 @@ namespace eosio::chain {
}

template<class BSP>
BSP fork_database_t<BSP>::get_block(const block_id_type& id) const {
BSP fork_database_t<BSP>::get_block(const block_id_type& id,
check_root_t check_root /* = check_root_t::no */) const {
std::lock_guard g( my->mtx );
return my->get_block_impl(id);
return my->get_block_impl(id, check_root);
}

template<class BSP>
BSP fork_database_impl<BSP>::get_block_impl(const block_id_type& id) const {
BSP fork_database_impl<BSP>::get_block_impl(const block_id_type& id,
check_root_t check_root /* = check_root_t::no */) const {
if( check_root == check_root_t::yes && root->id() == id ) {
return root;
}
auto itr = index.find( id );
if( itr != index.end() )
return *itr;
Expand Down Expand Up @@ -775,7 +761,7 @@ namespace eosio::chain {
fork_db_s = std::make_unique<fork_database_if_t>(fork_database_if_t::magic_number);
legacy = false;
apply_s<void>([&](auto& forkdb) {
forkdb.reset_root(*new_head);
forkdb.reset_root(new_head);
});
return block_handle{new_head};
}
Expand Down
6 changes: 3 additions & 3 deletions libraries/chain/include/eosio/chain/fork_database.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace eosio::chain {
using block_branch_t = std::vector<signed_block_ptr>;
enum class mark_valid_t { no, yes };
enum class ignore_duplicate_t { no, yes };
enum class check_root_t { no, yes };
Copy link
Member

Choose a reason for hiding this comment

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

Minor thing, but seems like this should be called include_root_t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually the named I used first, but it felt wrong for some reason and I prefer check_root_t. If you want to change it later I'm fine with it.


// Used for logging of comparison values used for best fork determination
std::string log_fork_comparison(const block_state& bs);
Expand Down Expand Up @@ -51,15 +52,14 @@ namespace eosio::chain {
void open( const std::filesystem::path& fork_db_file, validator_t& validator );
void close( const std::filesystem::path& fork_db_file );

bhsp_t get_block_header( const block_id_type& id ) const;
bsp_t get_block( const block_id_type& id ) const;
bsp_t get_block( const block_id_type& id, check_root_t check_root = check_root_t::no ) const;
bool block_exists( const block_id_type& id ) const;

/**
* Purges any existing blocks from the fork database and resets the root block_header_state to the provided value.
* The head will also be reset to point to the root.
*/
void reset_root( const bhs_t& root_bhs );
void reset_root( const bsp_t& root_bhs );

/**
* Removes validated flag from all blocks in fork database and resets head to point to the root.
Expand Down
2 changes: 1 addition & 1 deletion unittests/finalizer_vote_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ struct simulator_t {

auto genesis = make_bsp(proposal_t{0, "n0"}, bsp(), finpol);
bsp_vec.push_back(genesis);
forkdb.reset_root(*genesis);
forkdb.reset_root(genesis);

block_ref genesis_ref(genesis->id(), genesis->timestamp());
my_finalizer.fsi = fsi_t{block_timestamp_type(0), genesis_ref, genesis_ref};
Expand Down
2 changes: 1 addition & 1 deletion unittests/fork_db_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ BOOST_AUTO_TEST_CASE(add_remove_test) try {
// keep track of all those added for easy verification
std::vector<block_state_ptr> all { bsp11a, bsp12a, bsp13a, bsp11b, bsp12b, bsp12bb, bsp12bbb, bsp13b, bsp13bb, bsp13bbb, bsp14b, bsp11c, bsp12c, bsp13c };

forkdb.reset_root(*root);
forkdb.reset_root(root);
forkdb.add(bsp11a, mark_valid_t::no, ignore_duplicate_t::no);
forkdb.add(bsp11b, mark_valid_t::no, ignore_duplicate_t::no);
forkdb.add(bsp11c, mark_valid_t::no, ignore_duplicate_t::no);
Expand Down
Loading