Normalize usage of LIB #918
Conversation
…[num|id]. Use froot in logs that reference fork_db_root_block_num.
| uint32_t fork_db_root_block_num() const; | ||
| // thread-safe, only valid if fork_db_has_root() | ||
| block_id_type fork_db_root_block_id() const; | ||
| // thread-safe, only valid if fork_db_has_root() | ||
| time_point fork_db_root_block_time() const; |
There was a problem hiding this comment.
I think a better API would be to return a block_handle here, even if it involves a signed_block_ptr copy.
There was a problem hiding this comment.
A signed_block does not provide an efficient .id() call.
There was a problem hiding this comment.
my bad, I misspoke, the block_handle contains the block_state_ptr not signed_block_ptr.
There was a problem hiding this comment.
Oh right. Sure might as well match fork_db_head()
plugins/net_plugin/net_plugin.cpp
Outdated
| block_id_type fork_root_id; | ||
| uint32_t fork_root_num = 0; |
There was a problem hiding this comment.
I was matching the existing fork_head_ members.
plugins/net_plugin/net_plugin.cpp
Outdated
| chain_info_t get_chain_info() const; | ||
| uint32_t get_chain_lib_num() const; | ||
| uint32_t get_fork_root_num() const; |
There was a problem hiding this comment.
froot here to I think for consistency.
There was a problem hiding this comment.
Hmm, we have a get_fork_head_num. Seems better to spell it out.
plugins/net_plugin/net_plugin.cpp
Outdated
| my_impl->connections.for_each_block_connection( [&highest_lib_num]( const auto& cc ) { | ||
| // Closing connection, therefore its view of froot can no longer be considered as we will no longer be connected. | ||
| // Determine current froot of remaining peers as our sync_known_froot_num. | ||
| uint32_t highest_fork_root_num = 0; |
There was a problem hiding this comment.
Matches the handshake fork_root_num.
plugins/net_plugin/net_plugin.cpp
Outdated
| @@ -2049,9 +2049,9 @@ namespace eosio { | |||
|
|
|||
| auto reset_on_failure = [&]() REQUIRES(sync_mtx) { | |||
| sync_source.reset(); | |||
| sync_known_lib_num = chain_info.lib_num; | |||
| sync_known_froot_num = chain_info.fork_root_num; | |||
There was a problem hiding this comment.
froot here and all other fork_root locations.
There was a problem hiding this comment.
I think I would rather go the other direction. Change froot to fork_root except in logging. I do see it is currently inconsistent. I'm happy to go either way with this. Change froot to fork_root in code (leave as froot in logging). Or use froot and also fhead throughout. @linh2931 thoughts?
There was a problem hiding this comment.
whatever we choose let's be consistent throughout in the code.
There was a problem hiding this comment.
I prefer fork_root as it spells out what it really is; it is not long. The f in froot can be anything, it can even be be thought of finality.
There was a problem hiding this comment.
Renamed froot to fork_root.
… fork_db_root_block_time() with fork_db_root()
| auto fork_root = db.fork_db_root(); | ||
| auto fork_head = db.fork_db_head(); |
There was a problem hiding this comment.
Now this seems inconsistent. Why do we have both fork_root and fork_db_root at different spots in the code?
There was a problem hiding this comment.
I don't really like fork_root because it sounds like is the the root of a fork, not of fork_db. If we are going to spell it out, I think it should be fork_db_root everywhere, although I did prefer the shorter froot :-).
|
Another thing I noticed we have |
Maybe another PR to clean up all of those forkdb, fork_db usages, froot, ... |
This is the PR to normalize the usage... I think get it right here. |
|
Note:start |
Most of the changes involve using
frootinstead oflib.Did not change the interface of
testerto avoid uses having to update their tests.get_infowas unchanged; it still reportslast_irreversible_block_numandlast_irreversible_block_idforforkdb_root_block_numandforkdb_root_block_id.Resolves #806