IF: Change Fork_db root to store block state #2284
Conversation
| @@ -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. | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Set valid is only necessary when loading a version 1 fork database which does not have all the
block_state(doesn't read invalidated.). I think you should move the set valid intoopen_impland assert inreset_root_impl.
I tried that but many tests fail when I do that.
There was a problem hiding this comment.
Glancing at the tests, I think I know why. I can address that in #2141
There was a problem hiding this comment.
I'm curious to know what the issue could be?
There was a problem hiding this comment.
Mock block_state rely on this to set valid. Those need to set valid outside.
There was a problem hiding this comment.
Mock block_state rely on this to set valid. Those need to set valid outside.
Thanks, that makes sense.
|
Note:start |
| @@ -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 }; | |||
There was a problem hiding this comment.
Minor thing, but seems like this should be called include_root_t
There was a problem hiding this comment.
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.
Resolves #2283 .
Removes
fork_database::get_block_headerand simplifiescreate_block_state_i.The only difference between
fork_database::get_block_headerandfork_database::get_blockwas that the former checked theroot. It was also relying on the fact thatblock_stateispublic block_header_stateto return the same pointer downcasted.I have added an optional
check_rootparameter tofork_database::get_blockinstead.This allowed to make
create_block_state_ia template instead of having two separate implementations.New disk format and compatibility mode will be addressed in #2285 .