Added initial changes to support multiple Merkle trees in the storage. by HristoStaykov · Pull Request #2953 · vmware/concord-bft

HristoStaykov · 2023-03-10T18:50:22Z

Added assertion to check for collisions + fix.
Added the address field to the InternalNodeKey's serialization.

Problem Overview
Initial work to support multiple Merkle trees in storage. This will be needed when we build a MPT per account and a global MPT where the keys will be the account addresses and the values will be the account storage Merkle roots.
Testing Done
All unit tests pass + added a non empty address in unit test
serialization_unit_test/stale_db_key_internal for better
code coverage.
Ran locally 200 times the property based test sparse_merkle_storage_db_adapter_property_test

Added assertion to check for collisions + fix. Added the address field to the InternalNodeKey's serialization.

Fix to take in consideration the address field of the InternalNodeKey.

* Fix for correct reading of the size of nibbles. * Changes to the way we deserialize nibbles - we no longer rely on them being the last component in the buffer. * Fix for deserialization of the address field. * Added a non empty address in unit test serialization_unit_test/stale_db_key_internal for better code coverage.

vrancurel · 2023-03-16T21:51:10Z

    # The version of this key is the *tree* version, not block version
    uint64 version
    NibblePath path
+    string address


OK so we serialize all Merkle tree in the same collection, and we distinguish the merkle tree node keys by the account address ?

Yes, we can have all MPT-s (the Global state, which will have an empty address field and each account's tree) in the same storage space (CF).

teoparvanov

Great job, thanks @HristoStaykov !
(please include the additional property-based testing you did over the weekend in the "Testing done" section)

vrancurel · 2023-03-20T16:20:15Z

 }

 inline void serializeImp(const sparse_merkle::InternalNodeKey &key, std::string &out) {
+  serializeImp(key.address(), out);


Is it possible to have a prefix to distinguish for other types of nodes ? (we would have all the Merkle keys in the same CF).

Yes, we will have to prefix the InternalNodeKey as well, this is a work in progress.

Created types (Address and PaddedAddress) to hold addresses in LeafKey and InternalNodeKey.

We need to take in consideration the newly added PaddedAddress field.

teoparvanov

LGTM, thanks @HristoStaykov !

cloudnoize · 2023-04-04T09:50:32Z

+  size_t size() const { return prefix_.size(); }
+
+ protected:
+  Type prefix_;


why not to use std::string directly?

cloudnoize · 2023-04-04T09:53:31Z

+class PaddedCustomPrefix : public CustomPrefix {
+ public:
+  template <class T>
+  PaddedCustomPrefix(T&& arg) : CustomPrefix(std::forward<T>(arg)) {}


is this template can accept other types than std::string?

cloudnoize · 2023-04-04T09:59:14Z


  // Return the root of a sparse merkle tree at a given version.
-  static InternalNodeKey root(Version version) { return InternalNodeKey(version, NibblePath()); }
+  template <class T>


same as above, do we need to template it? As far as I only std::string satisfies it

cloudnoize · 2023-04-04T10:35:51Z

  // path, since there are no collisions requring going deeper into the tree.
  auto& root = batch.internal_nodes.at(0);
-  ASSERT_EQ(InternalNodeKey(1, NibblePath()), root.first);
+  ASSERT_EQ(InternalNodeKey("", 1, NibblePath()), root.first);


do you have tests that create multiple trees?

yes, a test covering multiple MPT-s is implemented here PR-3007

vmware#2953) * Added initial changes to support multiple Merkle trees in the storage. Added assertion to check for collisions + fix. Added the address field to the InternalNodeKey's serialization. * Fix for sparse_merkle_storage_serialization_unit_test. Fix to take in consideration the address field of the InternalNodeKey. * Serialization fixes. * Fix for correct reading of the size of nibbles. * Changes to the way we deserialize nibbles - we no longer rely on them being the last component in the buffer. * Fix for deserialization of the address field. * Added a non empty address in unit test serialization_unit_test/stale_db_key_internal for better code coverage. * Addressed clang-tidy warnings. * Moved address to be the first field in InternalNodeKey. * Added Address field to LeafKey. Created types (Address and PaddedAddress) to hold addresses in LeafKey and InternalNodeKey. * Fix for kvbc/test/sparse_merkle_storage/serialization_unit_test. We need to take in consideration the newly added PaddedAddress field. * Clang tidy report fixes. * Renamed address to custom_prefix.

#2953) * Added initial changes to support multiple Merkle trees in the storage. Added assertion to check for collisions + fix. Added the address field to the InternalNodeKey's serialization. * Fix for sparse_merkle_storage_serialization_unit_test. Fix to take in consideration the address field of the InternalNodeKey. * Serialization fixes. * Fix for correct reading of the size of nibbles. * Changes to the way we deserialize nibbles - we no longer rely on them being the last component in the buffer. * Fix for deserialization of the address field. * Added a non empty address in unit test serialization_unit_test/stale_db_key_internal for better code coverage. * Addressed clang-tidy warnings. * Moved address to be the first field in InternalNodeKey. * Added Address field to LeafKey. Created types (Address and PaddedAddress) to hold addresses in LeafKey and InternalNodeKey. * Fix for kvbc/test/sparse_merkle_storage/serialization_unit_test. We need to take in consideration the newly added PaddedAddress field. * Clang tidy report fixes. * Renamed address to custom_prefix.

vmware#2953) * Added initial changes to support multiple Merkle trees in the storage. Added assertion to check for collisions + fix. Added the address field to the InternalNodeKey's serialization. * Fix for sparse_merkle_storage_serialization_unit_test. Fix to take in consideration the address field of the InternalNodeKey. * Serialization fixes. * Fix for correct reading of the size of nibbles. * Changes to the way we deserialize nibbles - we no longer rely on them being the last component in the buffer. * Fix for deserialization of the address field. * Added a non empty address in unit test serialization_unit_test/stale_db_key_internal for better code coverage. * Addressed clang-tidy warnings. * Moved address to be the first field in InternalNodeKey. * Added Address field to LeafKey. Created types (Address and PaddedAddress) to hold addresses in LeafKey and InternalNodeKey. * Fix for kvbc/test/sparse_merkle_storage/serialization_unit_test. We need to take in consideration the newly added PaddedAddress field. * Clang tidy report fixes. * Renamed address to custom_prefix.

#2953) * Added initial changes to support multiple Merkle trees in the storage. Added assertion to check for collisions + fix. Added the address field to the InternalNodeKey's serialization. * Fix for sparse_merkle_storage_serialization_unit_test. Fix to take in consideration the address field of the InternalNodeKey. * Serialization fixes. * Fix for correct reading of the size of nibbles. * Changes to the way we deserialize nibbles - we no longer rely on them being the last component in the buffer. * Fix for deserialization of the address field. * Added a non empty address in unit test serialization_unit_test/stale_db_key_internal for better code coverage. * Addressed clang-tidy warnings. * Moved address to be the first field in InternalNodeKey. * Added Address field to LeafKey. Created types (Address and PaddedAddress) to hold addresses in LeafKey and InternalNodeKey. * Fix for kvbc/test/sparse_merkle_storage/serialization_unit_test. We need to take in consideration the newly added PaddedAddress field. * Clang tidy report fixes. * Renamed address to custom_prefix.

vmware#2953) * Added initial changes to support multiple Merkle trees in the storage. Added assertion to check for collisions + fix. Added the address field to the InternalNodeKey's serialization. * Fix for sparse_merkle_storage_serialization_unit_test. Fix to take in consideration the address field of the InternalNodeKey. * Serialization fixes. * Fix for correct reading of the size of nibbles. * Changes to the way we deserialize nibbles - we no longer rely on them being the last component in the buffer. * Fix for deserialization of the address field. * Added a non empty address in unit test serialization_unit_test/stale_db_key_internal for better code coverage. * Addressed clang-tidy warnings. * Moved address to be the first field in InternalNodeKey. * Added Address field to LeafKey. Created types (Address and PaddedAddress) to hold addresses in LeafKey and InternalNodeKey. * Fix for kvbc/test/sparse_merkle_storage/serialization_unit_test. We need to take in consideration the newly added PaddedAddress field. * Clang tidy report fixes. * Renamed address to custom_prefix.

Added initial changes to support multiple Merkle trees in the storage.

0e49cbf

Added assertion to check for collisions + fix. Added the address field to the InternalNodeKey's serialization.

HristoStaykov requested review from cloudnoize, eyalrund, nkgeorgiev and teoparvanov March 10, 2023 18:50

HristoStaykov requested a review from a team as a code owner March 10, 2023 18:50

vmwclabot added the cla-not-required label Mar 10, 2023

Fix for sparse_merkle_storage_serialization_unit_test.

7341f5a

Fix to take in consideration the address field of the InternalNodeKey.

HristoStaykov requested a review from toly-kournik March 14, 2023 13:21

HristoStaykov added 2 commits March 16, 2023 11:18

Addressed clang-tidy warnings.

519dd97

HristoStaykov requested a review from vrancurel March 16, 2023 13:32

vrancurel reviewed Mar 16, 2023

View reviewed changes

Comment thread kvbc/include/merkle_tree_serialization.h Outdated

Moved address to be the first field in InternalNodeKey.

85e64f8

teoparvanov approved these changes Mar 20, 2023

View reviewed changes

vrancurel reviewed Mar 20, 2023

View reviewed changes

cloudnoize force-pushed the storage/changes_for_eth_support branch from 37f2266 to 2a769b8 Compare March 27, 2023 08:36

vrancurel suggested changes Mar 27, 2023

View reviewed changes

Comment thread kvbc/cmf/categorized_kvbc_msgs.cmf Outdated

vrancurel suggested changes Mar 27, 2023

View reviewed changes

Comment thread kvbc/include/categorization/block_merkle_category.h Outdated

HristoStaykov added 4 commits March 27, 2023 19:18

Added Address field to LeafKey.

d3fd201

Created types (Address and PaddedAddress) to hold addresses in LeafKey and InternalNodeKey.

Fix for kvbc/test/sparse_merkle_storage/serialization_unit_test.

5667051

We need to take in consideration the newly added PaddedAddress field.

Clang tidy report fixes.

ec0309d

Renamed address to custom_prefix.

1f605ed

vrancurel approved these changes Mar 30, 2023

View reviewed changes

teoparvanov approved these changes Mar 31, 2023

View reviewed changes

HristoStaykov merged commit fe408d5 into vmware:storage/changes_for_eth_support Mar 31, 2023

cloudnoize reviewed Apr 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added initial changes to support multiple Merkle trees in the storage.#2953

Added initial changes to support multiple Merkle trees in the storage.#2953
HristoStaykov merged 9 commits intovmware:storage/changes_for_eth_supportfrom
HristoStaykov:WIP_support_mutiple_mpts

HristoStaykov commented Mar 10, 2023 •

edited

Loading

Uh oh!

vrancurel Mar 16, 2023

Uh oh!

HristoStaykov Mar 17, 2023

Uh oh!

Uh oh!

teoparvanov left a comment

Uh oh!

vrancurel Mar 20, 2023

Uh oh!

HristoStaykov Mar 22, 2023

Uh oh!

Uh oh!

Uh oh!

teoparvanov left a comment

Uh oh!

cloudnoize Apr 4, 2023

Uh oh!

cloudnoize Apr 4, 2023

Uh oh!

cloudnoize Apr 4, 2023

Uh oh!

cloudnoize Apr 4, 2023

Uh oh!

HristoStaykov May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

HristoStaykov commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

teoparvanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

teoparvanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HristoStaykov commented Mar 10, 2023 •

edited

Loading