[STALE - DO NOT REVIEW][Persistence] State Commitment - State Hash

[Olshansk]

Description

This is currently a WIP PR to compute the state hash.

Fixes #147.

Type of change

Please mark the options that are relevant.

• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation
• Other

How Has This Been Tested?

• TODO: Add more tests
• make test_all
• LocalNet

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have tested my changes using the available tooling
• If applicable, I have made corresponding changes to related or global README
• If applicable, I have added added new diagrams using mermaid.js
• If applicable, I have added tests that prove my fix is effective or that my feature works

[Olshansk]

tl;dr @andrewnguyen22 Could you take a look at this for preliminary feedback

---

This is far from complete (no tests, incomplete implementation, little integration, etc...) but the direction and core of how we'll compute the state hash for the MVP is there. It's only done for App at the moment, but the logic for others will be almost identical.

The short of it is:

1. Get each actor (flag, param, etc...) updated at a certain height (the context's height)
2. Compute the protobuf (the deterministic schema we use as source of truth)
3. Serialize the data struct
4. Update the corresponding merkle tree
5. Compute a state hash from the aggregated roots of all trees as per https://github.com/pokt-network/pocket-network-protocol/tree/main/persistence#562-state-transition-sequence-diagram

This is definitely only going to be something we submit AFTER your major persistence refactor, but I was hoping you could review and see if it looks like the right path given all the context you have (v0, refactoring persistence, etc...).

Personally, I think it makes sense but actually turns out to be much simpler than I thought which makes me feel like I'm missing something.

[andrewnguyen22]

This isn't very helpful

[Olshansk]

Agreed. I should have made the comment: "reminder in this commit: do integration with consensus module."

I was hoping for more preliminary feedback on the business logic in updateStateHash.

Going to leave a comment in the main PR with more pointers.

[andrewnguyen22]

@Olshansk

Preliminary feedback:

While, I am accustomed to each write operation actually updating the app-hash by updating the tree as a block is processed, I love the simplicity.

Basically, you've adopted a 'update the actor set each block' and that will affect the app-hash. I was originally skeptical of the design, but I'm beginning to appreciate it.

My current reservations:

• a) You are having to do a large read operation per 'tree' in order to compute the diff in the state hash. We have these objects during processing. Perhaps there's a clever caching design or something we may take advantage of.
• b) I believe that the leafs of the tree should actually be SHA3(appBytes) not appBytes
• c) In my experience, we will need to update more than just the actors so we might have quite a few state trees. Let's be explicit about the intention of the design to not just limit to actors (in documentation).
• d) Sorting the roots lexicographically and not by 'name' means that the order of the roots actually depends on the roots values. This seems like a strange design to me. Rather, would recommend a deterministic ordering based on the schema_name
• e) actors, err := p.GetActorsUpdated(schema.ApplicationActor, height) I don't think this is a deterministic ordering. We need to not only strictly enforce the ordering, we need to explicitly document it as such. This part of the code is one of the most important in terms of clarity and thinking through each decision as it will have to be replicated exactly agnostic of language in future clients.

EDIT: Starting to wonder if the merkle tree itself will enforce the ordering for (e) - unsure at this point

[Olshansk]

tl;dr Thanks for the review. I've gained confidence that we're heading in the right (simple AND efficient) solution after tending to these questions

a - Answered and think we're okay but will add TODO for cache optimizations.
b - Will double check but think it's the case as is.
c - Already done :)
d - Either works and I have a preference but not a strong one.
e - Order is not needed since we are using SMTs

While, I am accustomed to each write operation actually updating the app-hash by updating the tree as a block is processed, I love the simplicity.

Basically, you've adopted a 'update the actor set each block' and that will affect the app-hash. I was originally skeptical of the design, but I'm beginning to appreciate it.

Thank you. Let's keep thinking and discussing it. I have a good feeling about it (i.e. no allergies at the moment) but am also trying to figure out if there's a flaw due to its simplicity.



a) You are having to do a large read operation per 'tree' in order to compute the diff in the state hash. We have these objects during processing. Perhaps there's a clever caching design or something we may take advantage of.

Given that we have a constant number of trees (i.e. not a dynamic variable but can change), we are actually doing:

• O(1) read operations from the postgres DB (i.e. # of trees in total)
• We are reading O(N) # of rows where N is the numbers of elements modified at that height
• We are doing O(N) writes across all of the trees

See the screenshot below. Given that relational database are usually implemented using B-tree (good at read amplification) and key-value stores are usually implemented using LSM-trees (i.e. good at write amplification), I think this actually gives us a good balance of the two.

There is 100% still room for caching and optimization, so I will a TODO or github issue to investigate it. CC @okdas for a telemetry request :)

b) I believe that the leafs of the tree should actually be SHA3(appBytes) not appBytes

I'm still in the process of familiarizing myself with all of the details of https://github.com/celestiaorg/smt, but the interface is implemented like this:

nodeStore := smt.NewSimpleMap()
valueStore := smt.NewSimpleMap()

trees[treeType] = smt.NewSparseMerkleTree(nodeStore, valueStore, sha256.New())

I will confirm (while working on this commit) by reading the source code, but my understanding is that there is a behind the scenes optimization of using the provider hasher for the store used to compute the root, while the other one computes the values.

c) In my experience, we will need to update more than just the actors so we might have quite a few state trees. Let's be explicit about the intention of the design to not just limit to actors (in documentation).

Acknoeledged and agreed. Here is the current list of trees in the code. Note that I have added a couple for flags and params, so there is no limitation on just actors.

After we agree on a "high level design" with the app, I'll probably need copy-pasta the code to GetAllRowsUpdated for each of the other trees.

const (
    // Actors
    AppMerkleTree MerkleTree = iota
    ValMerkleTree
    FishMerkleTree
    ServiceNodeMerkleTree
    AccountMerkleTree
    PoolMerkleTree
    // Data / state
    BlocksMerkleTree
    ParamsMerkleTree
    FlagsMerkleTree
    lastMerkleTree // Used for iteration purposes only - see https://stackoverflow.com/a/64178235/768439
)

d) Sorting the roots lexicographically and not by 'name' means that the order of the roots actually depends on the roots values. This seems like a strange design to me. Rather, would recommend a deterministic ordering based on the schema_name

I was originally referencing the original persistence (see image) spec ad nauseum until you brought up.

Thinking about it thought, each tree root is deterministic based on the schema, so the lexographical order of each root should also be deterministic.

I don't have a strong preference here and can change it to include the schema name as the ordering key, but have a personal preference toward basic lexographic order simply because it's less moving pieces and provides the same guarantee.

e) actors, err := p.GetActorsUpdated(schema.ApplicationActor, height) I don't think this is a deterministic ordering. We need to not only strictly enforce the ordering, we need to explicitly document it as such. This part of the code is one of the most important in terms of clarity and thinking through each decision as it will have to be replicated exactly agnostic of language in future clients.

One of the nice things about using Sparse Merkle Tree instead of AVL trees is that the order doesn't matter in terms of tree construction. The tree is not balanced (but optimized in other ways that I can explain if needed), but since the tree path/branch is based solely on the key, we can do it in any order :)

[andrewnguyen22]

@Olshansk great response :)

Thinking about it thought, each tree root is deterministic based on the schema, so the lexographical order of each root should also be deterministic.

I don't follow this logic, because no-matter the schema, the values are arbitrary bytes - meaning it's very likely and possible the ordering of the trees is going to switch if you base the order around lexicographical sorting of the values. Instead, you should base the order around the lexicographical sorting of the schema_names.

A mock example:

Strict Ordering: sha3(app_tree_root + fish_tree_root + service_node_tree_root + validator_tree_root)

As opposed to

Value Ordering sha3(app_tree_root <= + fish_tree_root <= + service_node_tree_root <= + validator_tree_root)

[Olshansk]

Thanks for the additional explanation re ordering. SGTM! 💯

[Olshansk]

The learnings/discussions from this PR will be implemented in #284.