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

Specify merkle value #89

Merged
merged 4 commits into from
Sep 7, 2023
Merged

Specify merkle value #89

merged 4 commits into from
Sep 7, 2023

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Sep 6, 2023

Closes: #88

src/api.md Outdated
Comment on lines 11 to 17
- ["Merkle value"](https://spec.polkadot.network/chap-state#defn-merkle-value) designates a SCALE encoded value of the following type:
```
enum MerkleValue {
Node(Vec<u8>),
Hash(Hash)
}
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the same thing as what we said in #88 and it's not the same thing as what the spec say.

If you SCALE-encode a MerkleValue::Node for example, the output is starts with a 0 to indicate the enum variant, then the compact length of the node value, then finally the node value itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for explaining me how the type would be SCALE encoded.

The point being that if we want to make this non opaque, it means that we would need to have a fixed format to decode this. As I said in the issue, the value is either a 32 bytes long or the hash is 32 bytes long. You can not distinguish them. You need to have some form of encoding to distinguish them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I said in #88 was wrong indeed, but there's no need for the JSON-RPC client to know whether this is a node value or a hash. From the point of view of the JSON-RPC client it's opaque.
The only thing that matters is that all JSON-RPC servers generate the same value in a deterministic way, and for that there's no need for that enum.

Copy link
Contributor

@tomaka tomaka Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A light client implements the "closest descendant merkle value" by looking at the trie node of the closest ancestor and looking at the Merkle value of the corresponding children.

Consequently it also has no way to distinguish between a node value and its hash, unless it does a second networking round trip to also download the child. A light client simply sends back the opaque value found in the Merkle proof directly to the JSON-RPC client.

In other words, if you add an enum here, you force light clients to make an extra round trip, and it kind of kills the point of adding this "closest descendant merkle value" in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you will need polkadot-fellows/RFCs#9 to implement this in smoldot?

Copy link
Contributor

@tomaka tomaka Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it already works right now.
If the JSON-RPC client asks for the "closest descendant merkle value" of foo, I ask for a proof of parent(foo).
Ah yeah that's the intent, and actually I need that RFC in order to be able to ask for parent(foo). The code in smoldot that decodes proofs is completely ready though.

src/api.md Outdated
@@ -8,3 +8,4 @@ Any missing parameter, or parameter with an invalid format, should result in a J

- "hexadecimal-encoded" designates a binary value encoded as hexadecimal. The value must either be empty, or start with `"0x"` and contain an even number of characters.
- "SCALE-encoded" designates a value encoded using [the SCALE codec](https://docs.substrate.io/v3/advanced/scale-codec/).
- "Merkle value" as decribed in the [Polkadot spec](https://spec.polkadot.network/chap-state#defn-merkle-value).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's really the right place, as the Merkle value is used by a single function in the entire API. I'd just put that sentence in chainHead_unstable_follow.md.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also mention it in chainHead_unstable_storage. IMO would be nice to just use the "global" glossary.

@bkchr
Copy link
Member Author

bkchr commented Sep 7, 2023

@tomaka are you happy with the current state?

josepot
josepot previously approved these changes Sep 7, 2023
src/api.md Outdated Show resolved Hide resolved
Co-authored-by: Pierre Krieger <[email protected]>
@josepot josepot self-requested a review September 7, 2023 13:03
@josepot josepot merged commit aae7190 into main Sep 7, 2023
@josepot josepot deleted the bkchr-specify-merkle-value branch September 7, 2023 13:03
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 this pull request may close these issues.

Do we need to specify merkle value for closestDescendantMerkleValue?
3 participants