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

feat: ADR for api cleanup and async commit #790

Merged
merged 8 commits into from
Aug 16, 2023
Merged
Changes from 2 commits
Commits
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
98 changes: 98 additions & 0 deletions docs/architecture/adr-002-api-cleanup.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
# ADR ADR-002: API Cleanup to Support Store V2

## Changelog

* 2023-06-06: First draft

## Status

DRAFT

## Abstract

This ADR proposes a cleanup of the API to support [Store V2](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-065-store-v2.md).

There are some proposals for the speedup of the `Commit` by the async writes. See the [Discussion](https://github.com/cosmos/cosmos-sdk/issues/16173) for more details.

There is a lot of legacy code in the SDK that is not used anymore and can be removed. See the [Discussion](https://github.com/cosmos/iavl/issues/737) for more details.

## Context

The introduction of `Store V2` separates SS (State Storage) and SC (State Commitment), where `iavl` is used for SC and `versionDB` is already implemented for SS. The index of the `fast node` is not needed in `iavl` since `versionDB` allows key-value queries for a specific version.
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
Additionally, `Store V2` introduces a new approach to `SaveVersion` that accepts a batch object for the atomicity of the commit and removes the usage of `dbm.Batch` in `nodeDB`.

The current implementation of `iavl` suffers from performance issues due to synchronous writes during the `Commit` process. To address this, the proposed changes aim to finalize the current version in memory, parallelize hash calculations in `WorkingHash`, and introduce asynchronous writes.

Moreover, the existing architecture of `iavl` lacks modularity and code organization:

- The boundary between `ImmutableTree` and `MutableTree` is not clear.
- There are many public methods in `nodeDB`, making it less structured.
- `nodeDB` serves as both storage and cache simultaneously.

## Decision
Copy link
Member

Choose a reason for hiding this comment

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

what is the external api to users? the below seems to focus on internal changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you think iavl needs the external API interface to clarify? I plan to keep the majority external API


To support `Store V2` and improve `Commit` performance, we propose the following changes:

### Support Store V2

- Refactor the `SaveVersion` function to accept a batch object.
- Remove the index of the `fast node` from `iavl`.
- Eliminate the usage of `dbm.Batch` in `nodeDB`.

### Async Commit

- Parallelize hash calculations in `WorkingHash`.
- Finalize the current version in memory during `Commit`.
- Perform async writes in the background.

### API Cleanup

- Merge `ImmutableTree` and `MutableTree` into a single `Tree` structure.
- Make `nodeDB` methods private.
- Separate the cache from `nodeDB` and introduce a new `nodeCache` component.

The modified API will have the following structure:

```go
Tree interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

imo having read-only views as an interface makes sense

Copy link
Collaborator Author

@cool-develope cool-develope Jun 13, 2023

Choose a reason for hiding this comment

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

I am just thinking iavl will be used as a SC layer and it may mislead if have a read-only interface.
Also, we have only one interface for iavl tree in store module.

Copy link
Member

Choose a reason for hiding this comment

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

read only would be good as we still need the iavl tree for commitments, store v2 adr talks close to nothing about this. I wouldn't use the current adr as final its very very high level with a lack of experimentation and research

Has(key []byte) (bool, error)
Get(key []byte) ([]byte, error)
SaveVersion(batch struct{
keys [][]btye
values [][]byte
}) ([]byte, int64, error)
cool-develope marked this conversation as resolved.
Show resolved Hide resolved
Version() int64
Hash() []byte
WorkingHash() []byte
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
VersionExists(version int64) bool
DeleteVersionsTo(version int64) error
GetVersioned(key []byte, version int64) ([]byte, *cmtprotocrypto.ProofOps, error)
GetTree(version int64) (*iavl.Tree, error)
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 could be unnecessary if we move all query functionalities to SS...

SetInitialVersion(version uint64)
Iterator(start, end []byte, ascending bool) (types.Iterator, error)
LoadVersionForOverwriting(targetVersion int64) error
cool-develope marked this conversation as resolved.
Show resolved Hide resolved
}
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
```
## Consequences

We expect the proposed changes to improve the performance of Commit through async writes and parallelized hash calculations. Additionally, the codebase will become more organized and maintainable.

### Backwards Compatibility

While this ADR will break the external API of `iavl`, it will not affect the internal state of nodeDB. Compatibility measures and migration steps will be necessary during the `Store V2` migration to handle the breaking changes.

### Positive

- Atomicity of the `Commit`.
- Improved Commit performance through async writes and parallelized hash calculations.
- Increased flexibility and ease of modification and refactoring for iavl.

### Negative

- Async Commit may result in increased memory usage and increased code complexity.

## References

- [Store V2](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-065-store-v2.md)
- https://github.com/cosmos/cosmos-sdk/issues/16173
- https://github.com/cosmos/iavl/issues/737