-
Notifications
You must be signed in to change notification settings - Fork 285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MTP: Extension node is a lie! #703
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #703 +/- ##
=======================================
Coverage 94.29% 94.29%
=======================================
Files 143 143
Lines 16145 16110 -35
=======================================
- Hits 15224 15191 -33
+ Misses 921 919 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
fcb7222
to
75ec69a
Compare
65c369a
to
c4e9edf
Compare
c4e9edf
to
6004d73
Compare
42f8279
to
ce0bca3
Compare
b4727c9
to
f6bd4c1
Compare
11af3f5
to
60c37ca
Compare
After extracting refactoring to #751, this is finally ready for review. |
test/state/mpt.cpp
Outdated
const Path extended_path{m_path.begin(), this_idx_it}; | ||
const auto this_idx = *this_idx_it; | ||
m_path = Path{this_idx_it + 1, m_path.end()}; // shorten this path, invalidates this_idx_it | ||
*this = branch(extended_path, this_idx, std::make_unique<MPTNode>(std::move(*this)), | ||
*insert_idx_it, leaf(insert_tail, std::move(value))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we have a method to copy node with children, and try to simplify like:
const Path extended_path{m_path.begin(), this_idx_it}; | |
const auto this_idx = *this_idx_it; | |
m_path = Path{this_idx_it + 1, m_path.end()}; // shorten this path, invalidates this_idx_it | |
*this = branch(extended_path, this_idx, std::make_unique<MPTNode>(std::move(*this)), | |
*insert_idx_it, leaf(insert_tail, std::move(value))); | |
// The original node must be pushed down. | |
const Path common_path{m_path.begin(), this_idx_it}; | |
const Path this_node_tail{this_idx_it + 1, m_path.end()}; | |
auto this_node = copy_node(m_kind, this_node_tail, std::move(m_children), std::move(m_value)); // this_idx_it is not invalidated | |
auto new_leaf = leaf(insert_tail, std::move(value)); | |
*this = branch(common_path, *this_idx_it, std::move(this_node), *insert_idx_it, std::move(new_leaf)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need special method. The move constructor can do this. I believe I tried this but the effect was more code similarly confusing. I'm still thinking about some refactoring here (e.g. removing named constructors leaf()
and branch()
) but wanted to minimize the diff this time.
Notice that the formally specified extended node in the Merkle Patricia Trie always leads to a branch node. Therefore, we can treat branch nodes as having potentially non-empty "extended path" and remove the explicit extended node kind. This significantly simplifies the implementation.
MPT: Remove explicit extended node kind
Notice that the formally specified extended node in
the Merkle Patricia Trie always leads to a branch node.
Therefore, we can treat branch nodes as having potentially non-empty
"extended path" and remove the explicit extended node kind.
This significantly simplifies the implementation.