New implementation for pre-verified hashes #606
Conversation
| private: | ||
| static std::map<uint64_t, PreverifiedHashes> per_chain; // chain-id based access to the instances |
There was a problem hiding this comment.
I don't get the point of this. Why a map is needed ?
Afaik there is no way we can use preverified hashes from two (or more) different chains
There was a problem hiding this comment.
On a wider perspective I don't get the point of the whole PreverifiedHashes struct.
Why not simply a pair<BlockNum,std::setevmc::bytes32> as const member of WorkingChain which is the only object using it ? Basically preverified hashes should be treated pretty much as ConsensusEngine: once is loaded it stays the same for all the duration of Silkworm instance.
Unless there are specific reasons I'd get rid of this struct
There was a problem hiding this comment.
The PreverifiedHashes struct has 2 fields as std::pair<>, yes, but the first makes the code clearer:
preverified_hashes_->heightis more clear thanpreverified_hashes_.firstpreverified_hashes_->contains(...)is more clear thanpreverified_hashes_.second.find(...) ...
Also it is open to other needs.
There was a problem hiding this comment.
Regarding the map, I managed to remove it maintaining the possibility to use or not pre-verified hashes (e.g. with a command line switch).
| struct PreverifiedHashes { | ||
| std::set<evmc::bytes32> hashes; // Set of hashes of headers that are known to belong to canonical chain | ||
| uint64_t height{0}; // Block height corresponding to the highest preverified header | ||
| uint64_t height{0}; // Block height corresponding to the highest pre-verified header | ||
|
|
||
| [[nodiscard]] bool contains(const evmc::bytes32& hash) const { return hashes.find(hash) != hashes.end(); } | ||
|
|
||
| static PreverifiedHashes none; // A void set of hashes that can be used to turn-off pre-verified hashes usage and |
There was a problem hiding this comment.
Why define a static empty set of itself ?
There was a problem hiding this comment.
The WorkingChain must be able to run without pre-verified hashes. So by default it uses PreverifiedHashes::none that is a clear way to state this, IMO
| const PreverifiedHashes& PreverifiedHashes::load(uint64_t chain_id) { | ||
| PreverifiedHashes& result = per_chain[chain_id]; // insert if not present | ||
|
|
||
| if (chain_id == 1 && result.height == 0) { | ||
| load_preverified_hashes(result, | ||
| preverified_hashes_mainnet_data, | ||
| sizeof_preverified_hashes_mainnet_data, | ||
| preverified_hashes_mainnet_height); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
This does not make much sense to me:
result.height == 0seem to me a guard for chain_id presence inper_chainmap and anot-loadedflag- Hashes should be loaded at struct construction and never reloaded again imho
There was a problem hiding this comment.
The actual code is flexible enough to let us to decide in the future to load or not pre-verified hashes using a command line switch. Also the code make tests simple.
The code in the function is very simple IMO anyway I propose you to focus on the toolbox to find a way to insert the chain_id in the generated files so that here the code becomes generic respect to the chain.
There was a problem hiding this comment.
This was also simplified in the last commit.
Codecov Report
@@ Coverage Diff @@
## master #606 +/- ##
==========================================
- Coverage 81.58% 81.57% -0.01%
==========================================
Files 169 169
Lines 13768 13768
==========================================
- Hits 11232 11231 -1
- Misses 2536 2537 +1
Continue to review full report at Codecov.
|
This PR changes the implementation of pre-verified hashes to allow fast compilation.