IF: Support Leap 5 Fork Database disk format and new Savanna disk format. #2306
Conversation
Now `ship_streamer_if_test` test passes.
| @@ -157,16 +161,18 @@ namespace eosio::chain { | |||
| // see fork_database_t::fetch_branch(forkdb->head()->id()) | |||
| block_branch_t fetch_branch_from_head() const; | |||
|
|
|||
| void reset_root(const block_handle::block_handle_variant_t& v); | |||
There was a problem hiding this comment.
I really dislike that this exposes block_handle::block_handle_variant_t. I would prefer that type to not escape controller.cpp
There was a problem hiding this comment.
I defined block_state_variant_t in block_state.hpp.
There was a problem hiding this comment.
In my mind this is even worse. I would say we either go all the way and provide every method that takes a variant or we continue to use apply/etc in controller. I say we keep with using apply in controller.
There was a problem hiding this comment.
Hum, I don't think it is worse.
I get that block_handle is its own class providing a unified view of a block_state and that we don't want other things to depend on its internals.
However, it seems that there are some cases where we need the new block_state_variant_t type, like when we want to reset the root of our external fork_database which contains two fork_db pointers. I don't see why it is bad to define this type?
My intent in doing this was to try to help with the transition case where we may have two simultaneous fork_dbs. It may not be the best solution for your actual needs. If you know at this point what the interface should be, I'm happy to update this PR.
Did you mean in your comment that I should replace all instances of block_state_variant_t with std::variant<block_state_legacy_ptr, block_state_ptr>?
There was a problem hiding this comment.
Did you mean in your comment that I should replace all instances of block_state_variant_t with std::variant<block_state_legacy_ptr, block_state_ptr>?
No. We use apply for this type of thing elsewhere, just seem to me to be consisted to keep using apply. In my PR the creation of the new_head is in controller.
There was a problem hiding this comment.
You can leave as is, it will go away with my PR anyway.
There was a problem hiding this comment.
Sounds good, I'll leave as-is, feel free to update as you see fit.
I don't think we can use controller's apply always because what we want to do may depend of in_use inside fork_db.
| return; | ||
| } | ||
|
|
||
| std::ofstream out( fork_db_file.generic_string().c_str(), std::ios::out | std::ios::binary | std::ofstream::trunc ); |
There was a problem hiding this comment.
Seems a bit odd to use cfile to read and std::ofstream to write. Might be better to use cfile.
There was a problem hiding this comment.
It was that way before my change. But yes I agree it is a little bit inconsistent. I can change it if you think it is worthwhile.
| @@ -1694,12 +1689,12 @@ struct controller_impl { | |||
| // If we start at a block during or after the IF transition, we need to provide this information | |||
| // at startup. | |||
| // --------------------------------------------------------------------------------------------- | |||
| if (fork_db.fork_db_if_present()) { | |||
| if (auto in_use = fork_db.version_in_use(); in_use == fork_database::in_use_t::both || in_use == fork_database::in_use_t::savanna) { | |||
There was a problem hiding this comment.
Maybe you can have a method version_is_savanna() and version_is_legacy() to avoid this long expression. But no need to change now.
| @@ -3025,7 +3020,7 @@ struct controller_impl { | |||
|
|
|||
| chain_head = block_handle{std::move(new_head)}; | |||
| if (s != controller::block_status::irreversible) { | |||
| fork_db.switch_from_legacy(chain_head); | |||
| fork_db.switch_from_legacy(chain_head.internal()); | |||
There was a problem hiding this comment.
Not your PR. This internal() implies details exposed.
|
Note:start |
Resolves #2285.
This adds support for a new
fork_databasepersistent file version (v2), which can store two versions of thefork_database(thelegacyversion storingblock_state_legacypointers, and theSavannaversion storingblock_statepointers.In addition, the
in_usevariable indicating which version is currently in use is stored as well.Old version (
v1) which stores only thelegacyversion offork_database, can still be read.Re-enabled
ship_streamer_if_testwhich now passes.