fix(lib/trie): Check for root in EncodeAndHash

[kishansagathiya]

Changes

• In EncodeAndHash, If a node is root, we return blake2b hash of its encoding as hash
  regardless of whether length of encoding is smaller or greater than 32

Tests

Issues

• Check for root in EncodeAndHash #2357

Primary Reviewer

• @qdm12

[qdm12]

Nice, just a few nits.

Also, would it be possible to instead change the database store/fetch code to be able to understand smaller than 32 bytes values instead of hash for the root node? I guess not, just asking this out of curiosity.

[qdm12]

nit for extreme readability

Suggested change

	return find(t.root, key, recorder, true)
	const isRoot = true
	return find(t.root, key, recorder, isRoot)

[qdm12]

Suggested change

	return find(b.Children[key[length]], key[length+1:], recorder, false)
	const isChildRoot = false
	return find(b.Children[key[length]], key[length+1:], recorder, isChildRoot)

[EclesioMeloJunior]

the isChildRoot variable does not make much sense, the root node already has been processed and the other nodes are not the root, so it is explicit that for other find calls the false argument means the nodes are not the root

[qdm12]

Yeah rename as you wish. I just like setting constants inline to avoid looking up the argument names in the function. Lazy me I guess 😄

[codecov]

Codecov Report

Merging #2359 (32f4e17) into development (8c07aec) will increase coverage by 0.03%.
The diff coverage is 89.18%.

❗ Current head 32f4e17 differs from pull request most recent head 7446b9c. Consider uploading reports for the commit 7446b9c to get more accurate results

@@               Coverage Diff               @@
##           development    #2359      +/-   ##
===============================================
+ Coverage        58.01%   58.04%   +0.03%     
===============================================
  Files              213      213              
  Lines            28014    28008       -6     
===============================================
+ Hits             16251    16257       +6     
+ Misses           10120    10111       -9     
+ Partials          1643     1640       -3     

Flag	Coverage Δ
unit-tests	58.04% <89.18%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dot/state/test_helpers.go	49.76% <ø> (-0.24%)	⬇️
lib/trie/database.go	48.39% <75.00%> (+0.85%)	⬆️
dot/state/block_finalisation.go	47.25% <83.33%> (+2.60%)	⬆️
dot/state/tries.go	100.00% <100.00%> (+7.31%)	⬆️
internal/trie/node/hash.go	84.21% <100.00%> (ø)
lib/trie/lookup.go	76.00% <100.00%> (ø)
dot/network/connmgr.go	89.04% <0.00%> (-1.37%)	⬇️
lib/runtime/wasmer/imports.go	48.46% <0.00%> (-0.26%)	⬇️
dot/network/notifications.go	69.59% <0.00%> (+1.35%)	⬆️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ea0c65...7446b9c. Read the comment docs.

[kishansagathiya]

Also, would it be possible to instead change the database store/fetch code to be able to understand smaller than 32 bytes values instead of hash for the root node?

This could be possible. I prefer the current approach however. I prefer function's behaviour to be obvious based on its signature. If I edit key inside say db.Put(key, value), that is not an obvious behaviour. A behaviour like that could lead to bugs!

[qdm12]

If I edit key inside say db.Put(key, value), that is not an obvious behaviour. A behaviour like that could lead to bugs!

Definitely. But can't we write keys as byte slices (not fixed size arrays) to database? That way there might not be any custom code for the root, and we can know if it's a node value or the node encoding hash digest depending on the database retrieved byte slice length? 🤔 Maybe I'm wrong though..

[kishansagathiya]

But can't we write keys as byte slices (not fixed size arrays) to database?

That is what we do. the signature is db.Put(key []byte, value []byte)

That way there might not be any custom code for the root, and we can know if it's a node value or the node encoding hash digest depending on the database retrieved byte slice length?

We can't differentiate that way. hash of root node would always be 32 bytes. but hash of other nodes could be 32 or less than that. Also, we would need root hash at other place than just db related tasks.

Also, the approach works :)