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

docs: ADR-049 state sync hooks #10976

Merged
merged 15 commits into from
Feb 15, 2022
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
170 changes: 170 additions & 0 deletions docs/architecture/adr-049-state-sync-hooks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
# ADR 049: State Sync Hooks

## Changelog

- Jan 19, 2022: Initial Draft

## Status

Draft, Under Implementation

## Abstract

This ADR provides hooks for app modules to publish snapshot of additional state(outside of IAVL tree) for state-sync.
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

## Context

New clients use state-sync to download snapshots of module state from peer nodes. Currently, the snapshot consists of a
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
stream of `SnapshotStoreItem` and `SnapshotIAVLItem`, which means for app modules that maintain their states outside of
the IAVL tree, they can not add their states to the snapshot stream for state-sync.
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

## Decision

A simple proposal based on our existing implementation is that, we can add two new message types: `SnapshotExtensionMeta`
and `SnapshotExtensionPayload`, and they are appended to the existing multi-store stream with `SnapshotExtensionMeta`
acting as a delimiter between extensions.

```proto
// SnapshotItem is an item contained in a rootmulti.Store snapshot.
message SnapshotItem {
// item is the specific type of snapshot item.
oneof item {
SnapshotStoreItem store = 1;
SnapshotIAVLItem iavl = 2 [(gogoproto.customname) = "IAVL"];
SnapshotExtensionMeta extension = 3;
SnapshotExtensionPayload extension_payload = 4;
}
}

// SnapshotStoreItem contains metadata about a snapshotted store.
message SnapshotStoreItem {
string name = 1;
}

// SnapshotIAVLItem is an exported IAVL node.
message SnapshotIAVLItem {
bytes key = 1;
bytes value = 2;
int64 version = 3;
int32 height = 4;
}
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

// SnapshotExtensionMeta contains metadata about an external snapshotter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here there would be one SnapshotExtensionMeta for x/wasm module, maybe a second one for Ethermint module, etc?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, one for each extension snapshotter. You can have multiple snapshotters.

Copy link
Contributor Author

@adu-web3 adu-web3 Jan 21, 2022

Choose a reason for hiding this comment

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

// One module may need multiple snapshotters, so each module may have multiple SnapshotExtensionMeta.

add a comment and hope that can help clarify the question here

message SnapshotExtensionMeta {
string name = 1;
uint32 format = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

format is globally unique?
like x/wasm needs one, ethermint another, agoric a 3rd?
Or is this used within one namespace?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It’s used within the snapshotter/namespace.

Copy link
Member

Choose a reason for hiding this comment

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

They don't need to globally unique. They need to be unique in the state machine. So if you are 3 different modules that use different types of state sync packets you need formats 1, 2, 3. I don't see why they should be global

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, they are not global, they can be 1 at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zmanian and @yihuang you are saying different things.

Zaki: They need to be unique in the state machine

This was more or less what I meant by globally unique (on the chain). That you cannot have 2 modules on the same chain both using format 1.

Yi Huang: yes, they are not global, they can be 1 at the same time.

This is what I understood the answer to be, that (name, format) must be unique within this state machine. (And if we keep name globally unique, then we only have to consider format in the context of our own module).

Copy link
Collaborator

@yihuang yihuang Jan 24, 2022

Choose a reason for hiding this comment

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

To clarify there are two formats at play in the snapshot:

  • one is the snapshot format currently used, it decides both the msg serialization/chunking and multi-store snapshot formats.
  • then it's the extension snapshotter formats, they are only used by each extension snapshotter to decide the format of payloads included in SnapshotExtensionPayload message. So they are not related to one another.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is what I understood the answer to be, that (name, format) must be unique within this state machine. (And if we keep name globally unique, then we only have to consider format in the context of our own module).

technically speaking we can have duplicated names in the stream, it just means the snapshotter's Restore methods will be called multiple times. But I don't think that makes much sense, maybe we should even validate to prevent that situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is somewhat confusing. I recommend updating the field comment to be very explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Up. It would be good to clarify if name is unique, or (name, format) is. IMO first one is simpler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Up. It would be good to clarify if name is unique, or (name, format) is. IMO first one is simpler.

name is unique in current implementation.

}

// SnapshotExtensionPayload contains payloads of an external snapshotter.
message SnapshotExtensionPayload {
bytes payload = 1;
}
```

The snapshot stream would look like this:
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

```go
// multi-store snapshot
{SnapshotStoreItem | SnapshotIAVLItem, ...}
// extension1 snapshot
SnapshotExtensionMeta
{SnapshotExtensionPayload, ...}
// extension2 snapshot
SnapshotExtensionMeta
{SnapshotExtensionPayload, ...}
Comment on lines +75 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the ordering of these extensions in the stream if we register multiple in RegisterExtensions? Alphabetical by name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's alphabetical by name in current implementation.

Copy link
Contributor

@amaury1093 amaury1093 Feb 14, 2022

Choose a reason for hiding this comment

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

Sounds good, would like to see some wording for this. i suppose name should be unique then (ref: #10976 (comment))?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, would like to see some wording for this. i suppose name should be unique then (ref: #10976 (comment))?

yes

```

Add `extensions` field to snapshot `Manager` for extension snapshotters. Multistore snapshotter is a special one and it doesn't need a name because it is always placed at the beginning of the binary stream.

```go
type Manager struct {
store *Store
multistore types.Snapshotter
extensions map[string]types.NamedSnapshotter
mtx sync.Mutex
operation operation
chRestore chan<- io.ReadCloser
chRestoreDone <-chan restoreDone
restoreChunkHashes [][]byte
restoreChunkIndex uint32
}
```

For extension snapshotters that have implemented the `NamedSnapshotter` interface, their names should be registered to the snapshotter `Manager` by calling
`RegisterExtensions` when setting up the application.

```go
// RegisterExtensions register extension snapshotters to manager
func (m *Manager) RegisterExtensions(extensions ...types.NamedSnapshotter) error
```

As with `Snapshotter` interface for `multistore` snapshotter, two more function signatures: `SnapshotFormat()` and `SupportedFormats()` are added.
<ul>
<li> removal of format parameter: `Snapshotter` chooses a format autonomously, not pass in from the caller. </li>
<li> `Snapshotter` announces the format it uses to snapshot with method `SnapshotFormat()`. </li>
<ul>

```go
type Snapshotter interface {
// Snapshot writes snapshot items into the protobuf writer.
Snapshot(height uint64, protoWriter protoio.WriteCloser) error

// Restore restores a state snapshot from the protobuf items read from the reader.
// If the ready channel is non-nil, it returns a ready signal (by being closed) once the
// restorer is ready to accept chunks.
Restore(height uint64, format uint32, protoReader protoio.ReadCloser) (SnapshotItem, error)

// SnapshotFormat returns the default format used to take a snapshot.
SnapshotFormat() uint32

// SupportedFormats returns a list of formats it can restore from.
SupportedFormats() []uint32
}
```

Add `NamedSnapshotter` interface for extension snapshotters.

```go
type NamedSnapshotter interface {
Snapshotter

// SnapshotterName returns the name of snapshotter, it should be unique in the manager.
SnapshotterName() string
}
```

## Consequences

As a result of this implementation, we are able to create snapshots of binary chunk stream for the state that we maintain outside of the IAVL Tree, CosmWasm blobs for example. And new clients are able to fetch sanpshots of state for all modules that have implemented the corresponding interface from peer nodes.


### Backwards Compatibility

This ADR introduces new proto message types, add field for extension snapshotters to snapshot `Manager`, add new function signatures `SnapshotFormat()`, `SupportedFormats()` to `Snapshotter` interface and add new `NamedSnapshotter` interface, so this is not backwards compatible.

### Positive

State maintained outside of IAVL tree like CosmWasm blobs can create snapshots by implementing extension snapshotters, and being fetched by new clients via state-sync.
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

### Negative

// Todo
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved

### Neutral

All modules that maintain state outside of IAVL tree need to implement `NamedSnapshotter` and the snapshot manager need to call `RegisterExtensions` when setting up the application.

## Further Discussions

While an ADR is in the DRAFT or PROPOSED stage, this section should contain a summary of issues to be solved in future iterations (usually referencing comments from a pull-request discussion).
Later, this section can optionally list ideas or improvements the author or reviewers found during the analysis of this ADR.

## Test Cases [optional]

Test cases for an implementation are mandatory for ADRs that are affecting consensus changes. Other ADRs can choose to include links to test cases if applicable.

## References

- https://github.com/cosmos/cosmos-sdk/pull/10961
- https://github.com/cosmos/cosmos-sdk/issues/7340
- https://hackmd.io/gJoyev6DSmqqkO667WQlGw