state: Add insert-only MPT implementation #477
Conversation
Codecov Report
@@ Coverage Diff @@
## master #477 +/- ##
==========================================
- Coverage 99.61% 99.55% -0.07%
==========================================
Files 42 44 +2
Lines 4702 4942 +240
==========================================
+ Hits 4684 4920 +236
- Misses 18 22 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
chfast
force-pushed
the
state_rlp
branch
2 times, most recently
from
7b1099c
to
db90da9
Compare
|
typo in commit message: MTP => MPT |
chfast
force-pushed
the
state_rlp
branch
2 times, most recently
from
0f0f383
to
9fd19c4
Compare
test/state/mpt.cpp
Outdated
|
|
||
| [[nodiscard]] Path head(size_t size) const noexcept | ||
| { | ||
| assert(size < length); |
There was a problem hiding this comment.
tail(0) can return entire original path, but head(length) cannot?
test/state/mpt.cpp
Outdated
| else | ||
| n = std::move(children[0]); | ||
|
|
||
| MPTNode* branch = nullptr; |
There was a problem hiding this comment.
Add some comment(s) what's happening in this part - converting to a branch etc...
test/state/mpt.cpp
Outdated
| bytes m_value; | ||
| std::unique_ptr<MPTNode> children[num_children]; | ||
|
|
||
| MPTNode(Kind kind, const Path& path, bytes&& value = {}) noexcept |
There was a problem hiding this comment.
I think it's better to have bytes value parameter (pass by value). As I remember, && is a universal reference, supposed to handle both r-value references and values, useful mostly in templates. Here you intend to always move into constructor.
Also in leaf() and insert().
There was a problem hiding this comment.
This is universal reference only when combined with a template. Here this is rvalue reference. User is forced to move the bytes. This is enough for our use cases and eliminates unnecessary copies.
Passing by value is indeed more generic and provides better API.
|
Inspired by the review, I changed the "insert" implementation in a way the nodes are built bottom-up. Therefore, they can be built completely in a functional way. |
test/state/mpt.cpp
Outdated
| new_idx, | ||
| std::make_unique<MPTNode>(leaf(path.tail(mismatch_pos + 1), std::move(value)))); | ||
|
|
||
| *this = (mismatch_pos == 0) ? |
There was a problem hiding this comment.
What if mismatch_pos == 1, it will create new extension node with 1-nibble path, is this correct?
There was a problem hiding this comment.
Yes. Sometimes we need to extract common path prefix to "ext" node. If the paths differ at position 0 then there is not common prefix. I will document all these.
test/state/mpt.cpp
Outdated
|
|
||
| [[nodiscard]] Path head(size_t size) const noexcept | ||
| { | ||
| assert(size < length); |
There was a problem hiding this comment.
still not sure why this is not inclusive
| assert(size < length); | |
| assert(size <= length); |
There was a problem hiding this comment.
The assert comes from how this is used in MPT: using head() or tail() it may produce empty path but never produces a full copy of a path. I added separate asserts for this scope narrowing.
Add insert-only Merkle Patricia Trie implementation for computing MPT root hashes of (key, value) maps.
Add insert-only Merkle Patricia Trie implementation for computing MTP root hashes of (key, value) maps.