Skip to content
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

Do we need to specify merkle value for closestDescendantMerkleValue? #88

Closed
bkchr opened this issue Sep 6, 2023 · 5 comments · Fixed by #89
Closed

Do we need to specify merkle value for closestDescendantMerkleValue? #88

bkchr opened this issue Sep 6, 2023 · 5 comments · Fixed by #89

Comments

@bkchr
Copy link
Member

bkchr commented Sep 6, 2023

Reading the spec it isn't clear to me if merkle value returned by closestDescendantMerkleValue is an opaque object or we need to specify it? I mean it probably makes 100% sense to keep it opaque, but then we should mention this and also mention that this can not be compared across different implementations (not sure someone would ever do this).

@tomaka
Copy link
Contributor

tomaka commented Sep 6, 2023

The "Merkle value" is what is defined in the spec: https://spec.polkadot.network/chap-state#sect-merkl-proof

It's either the node value if it's < 32 bytes or the hash of the node value if it's >= 32 bytes.
(and the node value is concat(header, partial key, children bitmap, storage value, children))

I don't really see the problem with making it well defined. It's not like we're going to change this aspect of the trie any time soon.
And I don't think "the substrate-trie-db code is a bit confusing" is a good argument to make it opaque.

Saying that the value is opaque instead of well-defined means that the value might change after chainHead_follow generates a stop event, which actually complicates the JSON-RPC-client-side code (maybe quite a lot) as it must make sure to clear all its caches.

@bkchr
Copy link
Member Author

bkchr commented Sep 6, 2023

I'm also in for making it non opaque. That was the main reason I created this issue, to get consensus on this.

@bkchr
Copy link
Member Author

bkchr commented Sep 6, 2023

But then we should still specify it. I would assume return an encoded version of a SCALE encoded enum like this:

enum MerkleValue {
     Node(Vec<u8>),
     Hash(H),
}

Does that makes sense to you?

@tomaka
Copy link
Contributor

tomaka commented Sep 6, 2023

The Merkle value is:

  • Equal to the node value if the node value is < 32 bytes and the node is not the root of the trie
  • Equal to the hash of the node value otherwise

https://spec.polkadot.network/chap-state#defn-merkle-value

I don't see what more to specify

@bkchr
Copy link
Member Author

bkchr commented Sep 6, 2023

  • Equal to the node value if the node value is < 32 bytes and the node is not the root of the trie

The correct way is if the node value is <= 32 bytes. This means there is no way to distinguish if this is a node value or a hash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants