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

ADR-040: Storage and SMT State Commitments #8430

Merged
merged 24 commits into from
May 11, 2021
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
11728cf
ADR-040: Storage and SMT State Commitments
robert-zaremba Jan 25, 2021
662ec91
Update docs/architecture/adr-040-storage-and-smt-state-commitments.md
robert-zaremba Jan 26, 2021
5fdbe5d
Update docs/architecture/adr-040-storage-and-smt-state-commitments.md
robert-zaremba Jan 26, 2021
fa8e9e3
Added more details for snapshotting and pruning.
robert-zaremba Jan 26, 2021
864927e
updated links and references
robert-zaremba Jan 26, 2021
78215b2
add blockchains which already use SMT
robert-zaremba Jan 26, 2021
6dd0323
reorganize versioning and pruning
robert-zaremba Jan 27, 2021
250b5ff
Update docs/architecture/adr-040-storage-and-smt-state-commitments.md
robert-zaremba Jan 29, 2021
374916f
Update docs/architecture/adr-040-storage-and-smt-state-commitments.md
robert-zaremba Jan 29, 2021
e90bf8a
adding a paragraph about state management
robert-zaremba Jan 29, 2021
8602b3e
adr-40: update 'accessing old state' section
robert-zaremba Feb 25, 2021
ca39df5
Merge branch 'master' into robert/adr-040
robert-zaremba Apr 23, 2021
aedce21
update based on all recent discussions and validations
robert-zaremba Apr 23, 2021
f704279
adding more explanation about KV interface
robert-zaremba Apr 27, 2021
06d1952
Merge branch 'master' into robert/adr-040
robert-zaremba Apr 27, 2021
7537c84
Apply suggestions from code review
robert-zaremba Apr 28, 2021
1cc123e
Apply suggestions from code review
robert-zaremba Apr 28, 2021
d321dac
review comments
robert-zaremba Apr 28, 2021
80d0122
adding paragraph about commiting to an object without storying it
robert-zaremba Apr 28, 2021
962a28b
review updates
robert-zaremba Apr 30, 2021
bb89798
Apply suggestions from code review
robert-zaremba May 5, 2021
19d2126
review udpates
robert-zaremba May 5, 2021
356f987
adding clarification
robert-zaremba May 7, 2021
42e7f08
Merge branch 'master' into robert/adr-040
robert-zaremba May 11, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/architecture/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,4 @@ Read about the [PROCESS](./PROCESS.md).
- [ADR 037: Governance Split Votes](./adr-037-gov-split-vote.md)
- [ADR 038: State Listening](./adr-038-state-listening.md)
- [ADR 039: Epoched Staking](./adr-039-epoched-staking.md)
- [ADR 040: Storage and SMT State Commitments](./adr-040-storage-and-smt-state-commitments.md)
171 changes: 171 additions & 0 deletions docs/architecture/adr-040-storage-and-smt-state-commitments.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
# ADR 040: Storage and SMT State Commitments

## Changelog

- 2020-01-15: Draft

## Status

DRAFT Not Implemented


## Abstract

Sparse Merke Tree (SMT) is a version of a Merkle Tree with various storage and performance optimizations. This ADR defines a separation of state commitments from data storage and the SDK transition from IAVL to SMT.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this shouldn't in fact be two ADRs instead? One for separating storage and commitments and one about the SMT.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was also thinking about it. But they are highly related - one cannot be done without other. Hence, I'm proposing here a general design and leave a space for future ADR for RDMS which will introduce SDK breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

Well we could separate the two with IAVL right? We don't need SMT for that AFAIK...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@aaronc, we could describe here only SMT, but it will only a half backed idea without a working solution:

  • keeping IAVL (in it's current implementation) with anything else doesn't make sense because we double the data.
  • the main value proposition here is to not store objects in SMT (we store only hashes).

Do you have something else in mind?

robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved


## Context

Currently, Cosmos SDK uses IAVL for both state commitments and data storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would define what state commitments are and how it differs from data storage. It can be concise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't it self explaining? State commitment is a commitment to a state. I can add a link to explain more general commitment schemes.


IAVL has effectively become an orphaned project within the Cosmos ecosystem and it's proven to be an inefficient state commitment.
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
In the current design, IAVL is used for both data storage and as a Merkle Tree for state commitments. IAVL is meant to be a standalone Merkelized key/value database, however it's using a KV DB engine to store all tree nodes. So, each node is stored in a separate record in the KV DB. This causes many inefficiencies and problems:

+ Each object select requires a tree traversal from the root
+ Each edge traversal requires a DB query (nodes are not stored in a memory)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when traversing, we a tree we are always doing a DB query. However subsequent queries are cached on SDK level, not the IAVL level. I can add that calcification.

+ Creating snapshots is [expensive](https://github.com/cosmos/cosmos-sdk/issues/7215#issuecomment-684804950). It takes about 30 seconds to export less than 100 MB of state (as of March 2020).
+ Updates in IAVL may trigger tree reorganization and possible O(log(n)) hashes re-computation, which can become a CPU bottleneck.
+ The leaf structure is pretty expensive: it contains the `(key, value)` pair, additional metadata such as height, version. The entire node is hashed, and that hash is used as the key in the underlying database, [ref](https://github.com/cosmos/iavl/blob/master/docs/node/node.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please elaborate on why it's "expensive".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It contains lot of data, which is not needed in the new structure. We don't really need the metadata in the new structure.

).

Moreover, the IAVL project lacks support and a maintainer and we already see better and well-established alternatives. Instead of optimizing the IAVL, we are looking into other solutions for both storage and state commitments.


## Decision

We propose to separate the concerns of state commitment (**SC**), needed for consensus, and state storage (**SS**), needed for state machine. Finally we replace IAVL with [LazyLedgers' SMT](https://github.com/lazyledger/smt). LazyLedger SMT is based on Diem (called jellyfish) design [*] - it uses a compute-optimised SMT by replacing subtrees with only default values with a single node (same approach is used by Ethereum2 as well).
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what are the core differences between LL's SMT, Diem's and Eth2's?

Copy link
Contributor

@adlerjohn adlerjohn Apr 29, 2021

Choose a reason for hiding this comment

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

Unlike Diem's SMT, LL's:

  • implements compact proofs
  • Uses more standardized placeholders and domain separators (e.g. Diem uses this hash for the SMT default, LL uses sha256(0b))
  • is otherwise logically the same


The storage model presented here doesn't deal with data structure nor serialization. It's a Key-Value database, where both key and value are binaries. The storage user is responsible for data serialization.

### Decouple state commitment from storage


Separation of storage and commitment (by the SMT) will allow the optimization of different components according to their usage and access patterns.

SMT will use it's own storage (could use the same database underneath) from the state machine store. For every `(key, value)` pair, the SMT will store `hash(key)` in a path (needed to evenly distribute keys in the tree) and `hash(key, value)` in a leaf (to bind the (key, value) pair stored in the `SS`). Since we don't know a structure of a value (in particular if it contains the key) we hash both the key and the value in the `SC` leaf.
Copy link
Member

Choose a reason for hiding this comment

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

nit: I don't follow this first sentence. Is it using its own storage or the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the previous paragraphs I'm writing that we will separate state storage from state commitment. So the State commitment will have it's own storage (won't share the same namespace as the state storage). I will try to reword the paragraph to make it more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't follow this. It's hard to understand and there's a few punctuation errors. I would suggest re-wording this to make the points more clear and easy to follow.


For data access we propose 2 additional KV buckets (namespaces for the key-value pairs, sometimes called [column family](https://github.com/facebook/rocksdb/wiki/Terminology)):
1. B1: `key → value`: the principal object storage, used by a state machine, behind the SDK `KVStore` interface: provides direct access by key and allows prefix iteration (KV DB backend must support it).
2. B2: `hash(key, value) → key`: an index needed to extract a value (through: SMT → B2 → B1) having only a Merkle Path. Recall that SMT will store `hash(key, value)` in it's leafs.
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 follow this.

Copy link
Collaborator Author

@robert-zaremba robert-zaremba Apr 30, 2021

Choose a reason for hiding this comment

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

You can't get a data using SMT data. SMT only stores hashes.
So, if you read a value from SMT, and you want to get a data out, you need to recover the key.

Copy link
Contributor

Choose a reason for hiding this comment

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

So sort of like an inverted index then. Can you rewrite this sentence like you just explained to make it clearer please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

3. we could use more buckets to optimize the app usage if needed.

Above, we propose to use a KV DB. However, for the state machine, we could use an RDBMS, which we discuss below.


### Requirements

State Storage requirements:
+ range queries
+ quick (key, value) access
+ creating a snapshot
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
+ prunning (garbage collection)
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved

State Commitment requirements:
+ fast updates
+ tree path should be short
+ creating a snapshot
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
+ pruning (garbage collection)


### LazyLedger SMT for State Commitment

A Sparse Merkle tree is based on the idea of a complete Merkle tree of an intractable size. The assumption here is that as the size of the tree is intractable, there would only be a few leaf nodes with valid data blocks relative to the tree size, rendering the tree as sparse.


### Snapshots for storage sync and versioning

One of the Stargate core features are snapshots and state sync delivered in the `/snapshot` package. This feature is implemented in SDK and requires storage support. Currently IAVL is the only supported backend.

Database snapshot is a view of DB state at a certain time or transaction. It's not a full copy of a database (it would be too big), usually a snapshot mechanism is based on a _copy on write_ and it allows to efficiently deliver DB state at a certain stage.
Some DB engines support snapshotting. Hence, we propose to reuse that functionality for the state sync and versioning (described below). It will the supported DB engines to ones which efficiently implement snapshots. In a final section we will discuss evaluated DBs.
Comment on lines +81 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Database snapshot is a view of DB state at a certain time or transaction. It's not a full copy of a database (it would be too big), usually a snapshot mechanism is based on a _copy on write_ and it allows to efficiently deliver DB state at a certain stage.
Some DB engines support snapshotting. Hence, we propose to reuse that functionality for the state sync and versioning (described below). It will the supported DB engines to ones which efficiently implement snapshots. In a final section we will discuss evaluated DBs.
Database versioning provides a view of DB state at a certain time or transaction. It's not a full copy of a database (it would be too big), usually a versioning mechanism is based on a _copy on write_ and it allows to efficiently deliver DB state at a certain stage.
Some DB engines support viewing past versions. Hence, we propose to reuse that functionality for the state sync snapshots and versioning (described below). It will limit the supported DB engines to ones which efficiently implement versioning. In a final section we will discuss DBs evaluated for this feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm defining database snapshot here, so I prefer to use snapshot mechanism here, so I prefer to keep the original language.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay I may have misunderstood. This is what some DBs call snapshots, and distinct from state sync snapshots as used in the ABCI, right? (although it can be used to implement ABCI snapshots)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, here we are talking about a database engine mechanism.


New snapshot will be created in every `EndBlocker`. The `rootmulti.Store` keeps track of the version number and implements the `MultiStore` interface. `MultiStore` encapsulates a `Commiter` interface, which has the `Commit`, `SetPruning`, `GetPruning` functions which will be used for creating and removing snapshots. The `Store.Commit` function increments the version on each call, and checks if it needs to remove old versions. We will need to update the SMT interface to implement the `Commiter` interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is snapshot creation part of the state-machine process? Also, if you just take a direct DB snapshot, how do you perform verification?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the App has a knowledge when to create a snapshot. Storage doesn't have that knowledge. We could assume that it can create a snapshot on each commit, but it will make the design more constrained, and the library less robust.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about verification and the time it takes to create a snapshot?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's very efficient - the DB is using copy-on-write.

Copy link
Contributor

Choose a reason for hiding this comment

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

For clarification, copy-on-write is used to maintain historical versions, but the state sync snapshot still involves copying the entire state store at the time of creation (at least, that is how it's currently implemented).

robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
NOTE: `Commit` must be called exactly once per block. Otherwise we risk going out of sync for the version number and block height.
NOTE: For the SDK storage, we may consider splitting that interface into `Committer` and `PrunningCommiter` - only the multiroot should implement `PrunningCommiter` (cache and prefix store don't need pruning).
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved

Number of historical versions (snapshots) for `abci.Query` and fast sync is part of a node configuration, not a chain configuration (configuration implied by the blockchain consensus). A configuration should allow to specify number of past blocks and number of past blocks modulo some number (eg: 100 past blocks and one snapshot every 100 blocks for past 2000 blocks). Archival nodes can keep all snapshots.
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved

Pruning old snapshots is effectively done by a database. Whenever we update a record in `SC`, SMT will create a new one without removing the old one. Since we are snapshoting each block, we update the mechanism and immediately remove an orphaned from the storage. This is a safe operation - snapshots will keep track of the records which should be available for past versions.
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved

To manage the active snapshots we will either us a DB _max number of snapshots_ option (if available), or will remove snapshots in the `EndBlocker`. The latter option can be done efficiently by identifying snapshots with block height.
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit confusing to me. Pruning of Snapshots and pruning of application states, currently, are two separate configurable parameters. Are we merging these two? If so can it worded this way.

What is the impact to disk size with this design?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do you define application state pruning? For me, it is removing not needed records by a module (eg removing zero balances).
In this document I don't refer to "application state pruning". We don't do app state pruning here.

Copy link
Member

Choose a reason for hiding this comment

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

I am talking about how we currently prune application states or versions. You are talking about pruning versions or snapshots which are used for versions. This is application state pruning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I understand your concern. ADR-40 is not about pruning application state. Old SS (state storage) versions (a version of the whole state) are covered by snapshots. If we want to remove an old version we remove a snapshot.


#### Accessing old state versions

One of the functional requirements is to access old state. This is done through `abci.Query` structure. The version is specified by a block height (so we query for an object by a key `K` at block height `H`). The number of old versions supported for `abci.Query` is configurable. Accessing an old state is done by using available snapshots.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't specific to abci.Query. Might make more sense to reword in the sense of querying.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why it's not specific for abci.Query? I don't see any other use-case than using the ABCI to query old state.

Copy link
Member

Choose a reason for hiding this comment

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

Users want to query old state as well.. Many dont want to go through abci.Query.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How they do it now? Are you talking about a new feature?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
One of the functional requirements is to access old state. This is done through `abci.Query` structure. The version is specified by a block height (so we query for an object by a key `K` at block height `H`). The number of old versions supported for `abci.Query` is configurable. Accessing an old state is done by using available snapshots.
One of the functional requirements is to access old state. This is done through `abci.Query` structure. The version is specified by a block height (so we query for an object by a key `K` at block height `H`). The number of old versions supported for `abci.Query` is configurable. Accessing an old state is done by using available historical versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same here - I prefer to be consistent and use snapshot.

`abci.Query` doesn't need old state of `SC`. So, for efficiency, we should keep `SC` and `SS` in different databases (however using the same DB engine).

Moreover, SDK could provide a way to directly access the state. However, a state machines shouldn't do that - since the number of snapshots is configurable, it would lead to nondeterministic execution.

We positively [validated](https://github.com/cosmos/cosmos-sdk/discussions/8297) a snapshot mechanism for querying old state with regards to the database we evaluated.
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved

### State Proofs

For any object stored in State Store (SS), we have corresponding object in `SC`. A proof for object `V` identified by a key `K` is a branch of `SC`, where the path corresponds to the key `hash(K)`, and the leaf is `hash(K, V)`.

### Rollbacks

We need to be able to process transactions and roll-back state updates if a transaction fails. This can be done in the following way: during transaction processing, we keep all state change requests (writes) in a `CacheWrapper` abstraction (as it's done today). Once we finish the block processing, in the `Endblocker`, we commit a root store - at that time, all changes are written to the SMT and to the `SS` and a snapshot is created.


### Committing to an object without saving it

We identified use-cases, where modules will need to save an object commitment without storing an object itself. Sometimes clients are receiving complex objects, and they have no way to prove a correctness of that object without knowing the storage layout. For those use cases it would be easier to commit to the object without storing it directly.



## Consequences


### Backwards Compatibility

This ADR doesn't introduce any SDK level API changes.

We change the storage layout of the state machine, a storage migration and network upgrade is required to incorporate these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

a storage migration and network upgrade is required to incorporate these changes.

Is this changing key paths as well?

Can you discuss backwards compatibility with respect to ics23? It sounds like SMT will be responsible maintaining compatibility? This is a little beyond my area of expertise but I guess my question is:

  • will SMT support ICS 23 (or does it already)?
  • will SMT require a different proof spec from the existing one?

I ask these questions, because changing how proofs are verified or the proof paths can possibly break IBC resulting in the best case an asynchronous network upgrade and in the worst case a synchronous network upgrade (all IBC chains need to synchronously upgrade to continue communication)

cc @AdityaSripal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this changing key paths as well?

Yes, it's essentially a hard fork.

will SMT support ICS 23?

It's not done - it's in the scope of work we defined few weeks ago. I will clarify it here.

will SMT require a different proof spec from the existing one?

No, it's a merkle tree after all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

proof paths can possibly break IBC

This sounds important. The proofs will change (because it will require a hard fork), however the verification will be the same. Shall we discuss the network upgrade mechanism in the ADR, or it's a separate discussion?
I think it should be following:

  • disable IBC
  • export chain
  • update binary and import genesis
  • enable IBC

Copy link
Contributor

@colin-axner colin-axner Apr 30, 2021

Choose a reason for hiding this comment

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

Thanks for the response.

I'll defer to @AdityaSripal and @cwgoes to correct me, but I believe we need to consider the following cases:

IBC increases the complexity of upgrades because the counterparty needs to be informed of any changes in how the communication is being conducted.

Lets say we send a packet and construct a packet commitment under the path K. Normally the counterparty light client can verify a proof of the packet commitment, P, at some block height (ie VerifyProof(K, P)). In the code, our packet commitment proof path looks like this, a store prefix is also added before performing verification of this path. Until given further notice (unsupported atm), the counterparty will continue to assume the packet commit will only ever exist under key path K.

If the key paths are changing, then lets say K' represents the key path after the hard fork and P' represents the proof after the hard fork. My understanding is that K != K' but that P may equal P' (if the same value was to be proved at the same height, but just with different proof paths). K' would always fail on counterparties which are incapable of knowing the key path should be K'

Lets say we do the hard fork at height X, ie height X we are on the current code, height X + 1 we use SMT. The problem is all our counterparties need to be aware that at height X + 1 all key paths should use the new format. Not only do they need to be aware of that, but they need to be capable of using the new key paths for any proof heights >= X + 1. If the upgrade can be somewhat asynchronous, then chains need to be capable of supporting the old and new proof paths.

Re: disable/enable IBC

Unfortunately, there is no disabling/enabling IBC. We can disable/enable IBC transfers, but IBC itself is constantly moving forward. Clients may become expired if they are not successfully updated within a finite amount of time. This would result in funds essentially being "stuck". We do have recovery mechanisms, but ideally we coordinate a solution to avoid such a situation.

Currently, changing key paths/migrating IBC store is an unsupported upgrade, meaning existing IBC channels would be incapable of being used in the upgraded version. I believe it is entirely possible to add support for this, but we will need some time to specify the changes and implement/test them. In addition, all existing channels which cannot be lost between upgrades require all the counterparties to upgrade to the new IBC code which supports this upgrade route. Only then can the upgrade be performed without losing access to the channels.

Re: proof spec changes

I believe ics23 is a generic merkle proofs which uses proof specs for specific merkle tree representation. Changing the proof specs is an easier upgrade mechanism (still requires some coordination) than the above (it is supported), but this is would only be needed if it changed things like the hash function

This is a lot of unpack in a github comment and I think it will require more coordination/clarification - maybe best done in a live call. The primary concern is if a proof key path used in the new code cannot be verified using the old code (and vice versa). This requires the most difficult IBC upgrade path, but there may be clever tricks we can do to alleviate the difficulty over a period of time, if we are aware this is something that should be addressed in the near future

With regards to the ADR, if any the areas above will be effected, then it should be mentioned that these upgrade paths will need to be pursued and ibc-go may need to add more functionality, but the specific upgrade path itself is out of scope

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for comment @colin-axner . I think we should move this to the ADR-40 discussion page, so let me answer it there: #8297 (comment)

TL;DR: we need to design a mechanism for IBC which will work support hard forks anyway. They are inevitable.

In my opinion, this should not block ADR-40, for me this is related, but more general problem: how to handle hard forks. That being said - it will block a release - we need to have a general solution before doing any hard fork.

### Positive

+ Decoupling state from state commitment introduce better engineering opportunities for further optimizations and better storage patterns.
+ Performance improvements.
+ Joining SMT based camp which has wider and proven adoption than IAVL. Example projects which decided on SMT: Ethereum2, Diem (Libra), Trillan, Tezos, LazyLedger.

### Negative

+ Storage migration
+ LL SMT doesn't support pruning - we will need to add and test that functionality.

### Neutral

+ Deprecating IAVL, which is one of the core proposals of Cosmos Whitepaper.
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved


## Alternative designs.

Most of the alternative designs were evaluated in [state commitments and storage report](https://paper.dropbox.com/published/State-commitments-and-storage-review--BDvA1MLwRtOx55KRihJ5xxLbBw-KeEB7eOd11pNrZvVtqUgL3h).

Ethereum research published [Verkle Tire](https://notes.ethereum.org/_N1mutVERDKtqGIEYc-Flw#fnref1) - an idea of combining polynomial commitments with merkle tree in order to reduce the tree height. This concept has a very good potential, but we think it's too early to implement it. The current, SMT based design could be easily updated to the Verkle Tire once other research implement all necessary libraries. The main advantage of the design described in this ADR is the separation of state commitments from the data storage and designing a more powerful interface.


## Further Discussions

### Evaluated KV Databases

We verified existing databases KV databases for evaluating snapshot support. The following databases provide efficient snapshot mechanism: Badger, RocksDB, [Pebble](https://github.com/cockroachdb/pebble). Databases which don't provide such support or are not production ready: boltdb, leveldb, goleveldb, membdb, lmdb.
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved

### RDBMS

Use of RDBMS instead of simple KV store for state. Use of RDBMS will require an SDK API breaking change (`KVStore` interface), will allow better data extraction and indexing solutions. Instead of saving an object as a single blob of bytes, we could save it as record in a table in the state storage layer, and as a `hash(key, protobuf(object))` in the SMT as outlined above. To verify that an object registered in RDBMS is same as the one committed to SMT, one will need to load it from RDBMS, marshal using protobuf, hash and do SMT search.

### Off Chain Store

We were discussing use case where modules can use a support database, which is not automatically committed. Module will responsible for having a sound storage model and can optionally use the feature discussed in __Committing to an object without saving it_ section.


## References
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved

+ [IAVL What's Next?](https://github.com/cosmos/cosmos-sdk/issues/7100)
+ [IAVL overview](https://docs.google.com/document/d/16Z_hW2rSAmoyMENO-RlAhQjAG3mSNKsQueMnKpmcBv0/edit#heading=h.yd2th7x3o1iv) of it's state v0.15
+ [State commitments and storage report](https://paper.dropbox.com/published/State-commitments-and-storage-review--BDvA1MLwRtOx55KRihJ5xxLbBw-KeEB7eOd11pNrZvVtqUgL3h)
+ [LazyLedger SMT](https://github.com/lazyledger/smt)
+ Facebook Diem (Libra) SMT [design](https://developers.diem.com/papers/jellyfish-merkle-tree/2021-01-14.pdf)
+ [Trillian Revocation Transparency](https://github.com/google/trillian/blob/master/docs/papers/RevocationTransparency.pdf), [Trillian Verifiable Data Structures](https://github.com/google/trillian/blob/master/docs/papers/VerifiableDataStructures.pdf).
+ Design and implementation [discussion](https://github.com/cosmos/cosmos-sdk/discussions/8297).