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: agoric-sdk state-sync support #297

Merged
merged 4 commits into from
Mar 23, 2023
Merged

feat: agoric-sdk state-sync support #297

merged 4 commits into from
Mar 23, 2023

Conversation

mhofman
Copy link

@mhofman mhofman commented Feb 24, 2023

Refs: Agoric/agoric-sdk#3769

Description

This PR backports a couple cosmos PRs related to state-sync into the Agoric 0.45 based fork and exposes the Snapshot method of BaseApp so that agoric-sdk can hook into the snapshot process without suffering from the consequence of commit races.

The backports include a change in the ExtensionSnapshotter interface that simplifies the handling of snapshot payload, as well as a double close fix. The latter had significant merge conflicts to resolve since the original is based on a fairly large refactor (cosmos#11496)

For the change to BaseApp, SwingStore does not support opening a transaction at any random height, which means we need to open a read transaction before the next commit. To that end, we make the agoric-sdk cosmos app responsible to trigger the BaseApp snapshot with the correct synchronization, by exposing the Snapshot operation and introducing a new CommitWithoutSnapshot that does not automatically trigger the snapshot as part of Commit.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@michaelfig
Copy link
Collaborator

Your split-Commit commit looks fine. How do I see what modifications you needed to cherry-pick the other commits?

@mhofman
Copy link
Author

mhofman commented Feb 25, 2023

How do I see what modifications you needed to cherry-pick the other commits?

Besides side by side comparing the diffs, I don't know.

The 11825 was pretty much fully automated conflict resolution, which make for a straightforward difference between ca318be and cosmos@e397434

`diff -U 10 <(git show ca318be) <(git show e397434) > 11825-resolutions.diff`:
--- /dev/fd/63	2023-02-25 15:20:48.809214994 +0000
+++ /dev/fd/62	2023-02-25 15:20:48.809214994 +0000
@@ -1,11 +1,11 @@
-commit ca318be1bb1e89fa459d3b56b7a633d785bcb0f1
+commit e397434d9ec69c3605dc47e8fb4995b98c1b3df2
 Author: yihuang <huang@crypto.com>
 Date:   Thu Aug 18 15:33:55 2022 +0800
 
     feat: Make extension snapshotter interface safer to use (#11825)
     
     * Make extension snapshotter interface safer to use
     
     Closes: #11824
     Solution:
     - Use new methods `SnapshotExtension`/`RestoreExtension` to handle payload stream specifically.
@@ -15,34 +15,32 @@
     
     * Update snapshots/types/util.go
     
     * changelog
     
     * go linter
     
     * Update CHANGELOG.md
     
     Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
-    
-    (cherry picked from commit e397434d9ec69c3605dc47e8fb4995b98c1b3df2)
 
 diff --git a/CHANGELOG.md b/CHANGELOG.md
-index 627045e2f..2fe5b9ea1 100644
+index 43d16411c..88b9821b3 100644
 --- a/CHANGELOG.md
 +++ b/CHANGELOG.md
-@@ -55,6 +55,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
- ### API Breaking Changes
- 
- * [#13673](https://github.com/cosmos/cosmos-sdk/pull/13673) The `GetFromFields` function now takes `Context` as an argument and removes `genOnly`.
+@@ -105,6 +105,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
+   A SendEnabled query has been added to both GRPC and CLI.
+ * (appModule) Remove `Route`, `QuerierRoute` and `LegacyQuerierHandler` from AppModule Interface. 
+ * (x/modules) Remove all LegacyQueries and related code from modules
 +* (store) [#11825](https://github.com/cosmos/cosmos-sdk/pull/11825) Make extension snapshotter interface safer to use, renamed the util function `WriteExtensionItem` to `WriteExtensionPayload`.
  
- ## v0.45.10 - 2022-10-24
+ ### CLI Breaking Changes
  
 diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
 index e1616c226..5cc2b684c 100644
 --- a/docs/architecture/adr-049-state-sync-hooks.md
 +++ b/docs/architecture/adr-049-state-sync-hooks.md
 @@ -3,10 +3,11 @@
  ## Changelog
  
  - Jan 19, 2022: Initial Draft
 +- Apr 29, 2022: Safer extension snapshotter interface
@@ -82,98 +80,98 @@
 +	SnapshotExtension(height uint64, payloadWriter ExtensionPayloadWriter) error
 +
 +	// RestoreExtension restores an extension state snapshot,
 +	// the payload reader returns `io.EOF` when reached the extension boundaries.
 +	RestoreExtension(height uint64, format uint32, payloadReader ExtensionPayloadReader) error
 +
  }
  ```
  
 diff --git a/snapshots/helpers_test.go b/snapshots/helpers_test.go
-index 219dfd93e..7eb2aeae9 100644
+index 24051a17a..2dc18102c 100644
 --- a/snapshots/helpers_test.go
 +++ b/snapshots/helpers_test.go
 @@ -18,6 +18,7 @@ import (
  	"github.com/cosmos/cosmos-sdk/snapshots"
- 	"github.com/cosmos/cosmos-sdk/snapshots/types"
  	snapshottypes "github.com/cosmos/cosmos-sdk/snapshots/types"
+ 	"github.com/cosmos/cosmos-sdk/testutil"
 +	sdk "github.com/cosmos/cosmos-sdk/types"
  	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
  )
  
 @@ -62,7 +63,7 @@ func readChunks(chunks <-chan io.ReadCloser) [][]byte {
  }
  
  // snapshotItems serialize a array of bytes as SnapshotItem_ExtensionPayload, and return the chunks.
 -func snapshotItems(items [][]byte) [][]byte {
 +func snapshotItems(items [][]byte, ext snapshottypes.ExtensionSnapshotter) [][]byte {
  	// copy the same parameters from the code
  	snapshotChunkSize := uint64(10e6)
  	snapshotBufferSize := int(snapshotChunkSize)
 @@ -74,8 +75,20 @@ func snapshotItems(items [][]byte) [][]byte {
  		zWriter, _ := zlib.NewWriterLevel(bufWriter, 7)
  		protoWriter := protoio.NewDelimitedWriter(zWriter)
  		for _, item := range items {
--			types.WriteExtensionItem(protoWriter, item)
-+			types.WriteExtensionPayload(protoWriter, item)
+-			_ = snapshottypes.WriteExtensionItem(protoWriter, item)
++			_ = snapshottypes.WriteExtensionPayload(protoWriter, item)
  		}
 +		// write extension metadata
 +		_ = protoWriter.WriteMsg(&snapshottypes.SnapshotItem{
 +			Item: &snapshottypes.SnapshotItem_Extension{
 +				Extension: &snapshottypes.SnapshotExtensionMeta{
 +					Name:   ext.SnapshotName(),
 +					Format: ext.SnapshotFormat(),
 +				},
 +			},
 +		})
 +		_ = ext.SnapshotExtension(0, func(payload []byte) error {
 +			return snapshottypes.WriteExtensionPayload(protoWriter, payload)
 +		})
- 		protoWriter.Close()
- 		zWriter.Close()
- 		bufWriter.Flush()
-@@ -107,10 +120,11 @@ func (m *mockSnapshotter) Restore(
+ 		_ = protoWriter.Close()
+ 		_ = zWriter.Close()
+ 		_ = bufWriter.Flush()
+@@ -110,10 +123,11 @@ func (m *mockSnapshotter) Restore(
  		return snapshottypes.SnapshotItem{}, errors.New("already has contents")
  	}
  
 +	var item snapshottypes.SnapshotItem
  	m.items = [][]byte{}
  	for {
 -		item := &snapshottypes.SnapshotItem{}
 -		err := protoReader.ReadMsg(item)
 +		item.Reset()
 +		err := protoReader.ReadMsg(&item)
  		if err == io.EOF {
  			break
  		} else if err != nil {
-@@ -118,17 +132,17 @@ func (m *mockSnapshotter) Restore(
+@@ -121,17 +135,17 @@ func (m *mockSnapshotter) Restore(
  		}
  		payload := item.GetExtensionPayload()
  		if payload == nil {
 -			return snapshottypes.SnapshotItem{}, sdkerrors.Wrap(err, "invalid protobuf message")
 +			break
  		}
  		m.items = append(m.items, payload.Payload)
  	}
  
 -	return snapshottypes.SnapshotItem{}, nil
 +	return item, nil
  }
  
  func (m *mockSnapshotter) Snapshot(height uint64, protoWriter protoio.Writer) error {
  	for _, item := range m.items {
--		if err := types.WriteExtensionItem(protoWriter, item); err != nil {
-+		if err := types.WriteExtensionPayload(protoWriter, item); err != nil {
+-		if err := snapshottypes.WriteExtensionItem(protoWriter, item); err != nil {
++		if err := snapshottypes.WriteExtensionPayload(protoWriter, item); err != nil {
  			return err
  		}
  	}
-@@ -193,3 +207,52 @@ func (m *hungSnapshotter) Restore(
+@@ -216,3 +230,52 @@ func (m *hungSnapshotter) Restore(
  ) (snapshottypes.SnapshotItem, error) {
  	panic("not implemented")
  }
 +
 +type extSnapshotter struct {
 +	state []uint64
 +}
 +
 +func newExtSnapshotter(count int) *extSnapshotter {
 +	state := make([]uint64, 0, count)
@@ -213,46 +211,46 @@
 +			break
 +		} else if err != nil {
 +			return err
 +		}
 +		s.state = append(s.state, sdk.BigEndianToUint64(payload))
 +	}
 +	// finalize restoration
 +	return nil
 +}
 diff --git a/snapshots/manager.go b/snapshots/manager.go
-index 74ea4f4d8..1ec632718 100644
+index 05cbad3f5..efc123e9e 100644
 --- a/snapshots/manager.go
 +++ b/snapshots/manager.go
-@@ -79,6 +79,9 @@ func NewManagerWithExtensions(store *Store, multistore types.Snapshotter, extens
+@@ -84,6 +84,9 @@ func NewManager(store *Store, opts types.SnapshotOptions, multistore types.Snaps
  
  // RegisterExtensions register extension snapshotters to manager
  func (m *Manager) RegisterExtensions(extensions ...types.ExtensionSnapshotter) error {
 +	if m.extensions == nil {
 +		m.extensions = make(map[string]types.ExtensionSnapshotter, len(extensions))
 +	}
  	for _, extension := range extensions {
  		name := extension.SnapshotName()
  		if _, ok := m.extensions[name]; ok {
-@@ -195,7 +198,10 @@ func (m *Manager) createSnapshot(height uint64, ch chan<- io.ReadCloser) {
+@@ -215,7 +218,10 @@ func (m *Manager) createSnapshot(height uint64, ch chan<- io.ReadCloser) {
  			streamWriter.CloseWithError(err)
  			return
  		}
 -		if err := extension.Snapshot(height, streamWriter); err != nil {
 +		payloadWriter := func(payload []byte) error {
 +			return types.WriteExtensionPayload(streamWriter, payload)
 +		}
 +		if err := extension.SnapshotExtension(height, payloadWriter); err != nil {
  			streamWriter.CloseWithError(err)
  			return
  		}
-@@ -285,24 +291,40 @@ func (m *Manager) Restore(snapshot types.Snapshot) error {
+@@ -305,24 +311,40 @@ func (m *Manager) Restore(snapshot types.Snapshot) error {
  
  // restoreSnapshot do the heavy work of snapshot restoration after preliminary checks on request have passed.
  func (m *Manager) restoreSnapshot(snapshot types.Snapshot, chChunks <-chan io.ReadCloser) error {
 +	var nextItem types.SnapshotItem
 +
  	streamReader, err := NewStreamReader(chChunks)
  	if err != nil {
  		return err
  	}
  	defer streamReader.Close()
@@ -283,109 +281,109 @@
  			break
  		}
 -		metadata := next.GetExtension()
 +		metadata := nextItem.GetExtension()
  		if metadata == nil {
 -			return sdkerrors.Wrapf(sdkerrors.ErrLogic, "unknown snapshot item %T", next.Item)
 +			return sdkerrors.Wrapf(sdkerrors.ErrLogic, "unknown snapshot item %T", nextItem.Item)
  		}
  		extension, ok := m.extensions[metadata.Name]
  		if !ok {
-@@ -311,10 +333,14 @@ func (m *Manager) restoreSnapshot(snapshot types.Snapshot, chChunks <-chan io.Re
+@@ -331,10 +353,14 @@ func (m *Manager) restoreSnapshot(snapshot types.Snapshot, chChunks <-chan io.Re
  		if !IsFormatSupported(extension, metadata.Format) {
  			return sdkerrors.Wrapf(types.ErrUnknownFormat, "format %v for extension %s", metadata.Format, metadata.Name)
  		}
 -		next, err = extension.Restore(snapshot.Height, metadata.Format, streamReader)
 -		if err != nil {
 +
 +		if err := extension.RestoreExtension(snapshot.Height, metadata.Format, payloadReader); err != nil {
  			return sdkerrors.Wrapf(err, "extension %s restore", metadata.Name)
  		}
 +
 +		if nextItem.GetExtensionPayload() != nil {
 +			return sdkerrors.Wrapf(err, "extension %s don't exhausted payload stream", metadata.Name)
 +		}
  	}
  	return nil
  }
 diff --git a/snapshots/manager_test.go b/snapshots/manager_test.go
-index 256b5b2cc..e8d02ca28 100644
+index 7fbddd6c7..58a302e87 100644
 --- a/snapshots/manager_test.go
 +++ b/snapshots/manager_test.go
-@@ -61,11 +61,15 @@ func TestManager_Take(t *testing.T) {
- 	snapshotter := &mockSnapshotter{
- 		items: items,
+@@ -68,11 +68,15 @@ func TestManager_Take(t *testing.T) {
+ 		items:         items,
+ 		prunedHeights: make(map[int64]struct{}),
  	}
 -	expectChunks := snapshotItems(items)
 +	extSnapshotter := newExtSnapshotter(10)
 +
 +	expectChunks := snapshotItems(items, extSnapshotter)
- 	manager := snapshots.NewManager(store, snapshotter)
+ 	manager := snapshots.NewManager(store, opts, snapshotter, nil, log.NewNopLogger())
 +	err := manager.RegisterExtensions(extSnapshotter)
 +	require.NoError(t, err)
  
  	// nil manager should return error
 -	_, err := (*snapshots.Manager)(nil).Create(1)
 +	_, err = (*snapshots.Manager)(nil).Create(1)
  	require.Error(t, err)
  
  	// creating a snapshot at a lower height than the latest should error
-@@ -79,7 +83,7 @@ func TestManager_Take(t *testing.T) {
+@@ -91,7 +95,7 @@ func TestManager_Take(t *testing.T) {
  		Height: 5,
  		Format: snapshotter.SnapshotFormat(),
  		Chunks: 1,
 -		Hash:   []uint8{0xcd, 0x17, 0x9e, 0x7f, 0x28, 0xb6, 0x82, 0x90, 0xc7, 0x25, 0xf3, 0x42, 0xac, 0x65, 0x73, 0x50, 0xaa, 0xa0, 0x10, 0x5c, 0x40, 0x8c, 0xd5, 0x1, 0xed, 0x82, 0xb5, 0xca, 0x8b, 0xe0, 0x83, 0xa2},
 +		Hash:   []uint8{0x89, 0xfa, 0x18, 0xbc, 0x5a, 0xe3, 0xdc, 0x36, 0xa6, 0x95, 0x5, 0x17, 0xf9, 0x2, 0x1a, 0x55, 0x36, 0x16, 0x5d, 0x4b, 0x8b, 0x2b, 0x3d, 0xfd, 0xe, 0x2f, 0xb6, 0x40, 0x6b, 0xc3, 0xbc, 0x23},
  		Metadata: types.Metadata{
  			ChunkHashes: checksums(expectChunks),
  		},
-@@ -117,7 +121,10 @@ func TestManager_Prune(t *testing.T) {
- func TestManager_Restore(t *testing.T) {
- 	store := setupStore(t)
- 	target := &mockSnapshotter{}
+@@ -133,7 +137,10 @@ func TestManager_Restore(t *testing.T) {
+ 	target := &mockSnapshotter{
+ 		prunedHeights: make(map[int64]struct{}),
+ 	}
 +	extSnapshotter := newExtSnapshotter(0)
- 	manager := snapshots.NewManager(store, target)
+ 	manager := snapshots.NewManager(store, opts, target, nil, log.NewNopLogger())
 +	err := manager.RegisterExtensions(extSnapshotter)
 +	require.NoError(t, err)
  
  	expectItems := [][]byte{
  		{1, 2, 3},
-@@ -125,10 +132,10 @@ func TestManager_Restore(t *testing.T) {
+@@ -141,10 +148,10 @@ func TestManager_Restore(t *testing.T) {
  		{7, 8, 9},
  	}
  
 -	chunks := snapshotItems(expectItems)
 +	chunks := snapshotItems(expectItems, newExtSnapshotter(10))
  
  	// Restore errors on invalid format
 -	err := manager.Restore(types.Snapshot{
 +	err = manager.Restore(types.Snapshot{
  		Height:   3,
  		Format:   0,
  		Hash:     []byte{1, 2, 3},
-@@ -186,6 +193,7 @@ func TestManager_Restore(t *testing.T) {
+@@ -204,6 +211,7 @@ func TestManager_Restore(t *testing.T) {
  	}
  
  	assert.Equal(t, expectItems, target.items)
 +	assert.Equal(t, 10, len(extSnapshotter.state))
  
  	// Starting a new restore should fail now, because the target already has contents.
  	err = manager.Restore(types.Snapshot{
 diff --git a/snapshots/types/snapshotter.go b/snapshots/types/snapshotter.go
-index f747920d1..1044d57b6 100644
+index 76f800484..1641042a6 100644
 --- a/snapshots/types/snapshotter.go
 +++ b/snapshots/types/snapshotter.go
-@@ -11,17 +11,20 @@ type Snapshotter interface {
- 	// Snapshot writes snapshot items into the protobuf writer.
- 	Snapshot(height uint64, protoWriter protoio.Writer) error
+@@ -22,17 +22,20 @@ type Snapshotter interface {
+ 	// to determine which heights to retain until after the snapshot is complete.
+ 	SetSnapshotInterval(snapshotInterval uint64)
  
--	// Restore restores a state snapshot from the protobuf items read from the reader.
+-	// Restore restores a state snapshot, taking snapshot chunk readers as input.
 -	// If the ready channel is non-nil, it returns a ready signal (by being closed) once the
 -	// restorer is ready to accept chunks.
 +	// Restore restores a state snapshot, taking the reader of protobuf message stream as input.
  	Restore(height uint64, format uint32, protoReader protoio.Reader) (SnapshotItem, error)
  }
  
 +// ExtensionPayloadReader read extension payloads,
 +// it returns io.EOF when reached either end of stream or the extension boundaries.
 +type ExtensionPayloadReader = func() ([]byte, error)
 +
@@ -393,21 +391,21 @@
 +type ExtensionPayloadWriter = func([]byte) error
 +
  // ExtensionSnapshotter is an extension Snapshotter that is appended to the snapshot stream.
  // ExtensionSnapshotter has an unique name and manages it's own internal formats.
  type ExtensionSnapshotter interface {
 -	Snapshotter
 -
  	// SnapshotName returns the name of snapshotter, it should be unique in the manager.
  	SnapshotName() string
  
-@@ -32,4 +35,11 @@ type ExtensionSnapshotter interface {
+@@ -43,4 +46,11 @@ type ExtensionSnapshotter interface {
  
  	// SupportedFormats returns a list of formats it can restore from.
  	SupportedFormats() []uint32
 +
 +	// SnapshotExtension writes extension payloads into the underlying protobuf stream.
 +	SnapshotExtension(height uint64, payloadWriter ExtensionPayloadWriter) error
 +
 +	// RestoreExtension restores an extension state snapshot,
 +	// the payload reader returns `io.EOF` when reached the extension boundaries.
 +	RestoreExtension(height uint64, format uint32, payloadReader ExtensionPayloadReader) error

The more problematic one was 13400 because of tests having been heavily refactored, so I mostly applied what passed automated conflict resolution, dropped changes that didn't apply, and manually fixed the tests that were still broken because they hadn't been patched. I also bumped the format from 1 -> 2 instead of 2 -> 3 in the original PR, figuring we would bump from 2 -> 3 later when we integrate whatever made the 1 -> 2 change needed upstream.

`diff -U 10 <(git show 7a23db1) <(git show dcb0c9c) > 13400-resolutions.diff`:
--- /dev/fd/63	2023-02-25 15:24:03.149425981 +0000
+++ /dev/fd/62	2023-02-25 15:24:03.149425981 +0000
@@ -1,151 +1,153 @@
-commit 7a23db1d5ef4c3586e4d1a335f372b6ad5ec80bf
+commit dcb0c9c04c5d8406bede521cde94335015185b2b
 Author: mmsqe <mavis@crypto.com>
 Date:   Wed Sep 28 02:16:13 2022 +0800
 
     fix: double close (#13400)
-    
-    (cherry picked from commit dcb0c9c04c5d8406bede521cde94335015185b2b)
 
 diff --git a/CHANGELOG.md b/CHANGELOG.md
-index 2fe5b9ea1..ae68bfc16 100644
+index d08dcb773..f8d2180a3 100644
 --- a/CHANGELOG.md
 +++ b/CHANGELOG.md
-@@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
- 
- * (deps) Bump Tendermint version to [v0.34.23](https://github.com/tendermint/tendermint/releases/tag/v0.34.23).
- * (deps) Bump IAVL version to [v0.19.4](https://github.com/cosmos/iavl/releases/tag/v0.19.4).
+@@ -176,6 +176,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
+ * (x/gov) [#13051](https://github.com/cosmos/cosmos-sdk/pull/13051) In SubmitPropsal, when a legacy msg fails it's handler call, wrap the error as ErrInvalidProposalContent (instead of ErrNoProposalHandlerExists).
+ * (x/gov) [#13045](https://github.com/cosmos/cosmos-sdk/pull/13045) Fix gov migrations for v3(0.46).
+ * (Store) [#13334](https://github.com/cosmos/cosmos-sdk/pull/13334) Call streaming listeners for deliver tx event, it was removed accidentally.
 +* (snapshot) [#13400](https://github.com/cosmos/cosmos-sdk/pull/13400) Fix snapshot checksum issue in golang 1.19.
  
- ### Bug Fixes
- 
-diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go
-index f9cdfcb71..bf962439c 100644
---- a/baseapp/baseapp_test.go
-+++ b/baseapp/baseapp_test.go
-@@ -167,7 +167,7 @@ func TestListSnapshots(t *testing.T) {
- 	app, _ := setupBaseAppWithSnapshots(t, 2, 5)
+ ### Deprecated
  
- 	expected := abci.ResponseListSnapshots{Snapshots: []*abci.Snapshot{
--		{Height: 2, Format: 1, Chunks: 2},
-+		{Height: 2, Format: 2, Chunks: 2},
- 	}}
- 
- 	resp := app.ListSnapshots(abci.RequestListSnapshots{})
 diff --git a/baseapp/deliver_tx_test.go b/baseapp/deliver_tx_test.go
-index c0126f996..df2dea33a 100644
+index 7eacbee32..453ddab35 100644
 --- a/baseapp/deliver_tx_test.go
 +++ b/baseapp/deliver_tx_test.go
-@@ -39,13 +39,13 @@ func TestLoadSnapshotChunk(t *testing.T) {
- 		chunk       uint32
- 		expectEmpty bool
+@@ -666,7 +666,7 @@ func TestSnapshotWithPruning(t *testing.T) {
+ 				pruningOpts:        pruningtypes.NewPruningOptions(pruningtypes.PruningNothing),
+ 			},
+ 			expectedSnapshots: []*abci.Snapshot{
+-				{Height: 20, Format: 2, Chunks: 5},
++				{Height: 20, Format: snapshottypes.CurrentFormat, Chunks: 5},
+ 			},
+ 		},
+ 		"prune everything with snapshot": {
+@@ -678,7 +678,7 @@ func TestSnapshotWithPruning(t *testing.T) {
+ 				pruningOpts:        pruningtypes.NewPruningOptions(pruningtypes.PruningEverything),
+ 			},
+ 			expectedSnapshots: []*abci.Snapshot{
+-				{Height: 20, Format: 2, Chunks: 5},
++				{Height: 20, Format: snapshottypes.CurrentFormat, Chunks: 5},
+ 			},
+ 		},
+ 		"default pruning with snapshot": {
+@@ -690,7 +690,7 @@ func TestSnapshotWithPruning(t *testing.T) {
+ 				pruningOpts:        pruningtypes.NewPruningOptions(pruningtypes.PruningDefault),
+ 			},
+ 			expectedSnapshots: []*abci.Snapshot{
+-				{Height: 20, Format: 2, Chunks: 5},
++				{Height: 20, Format: snapshottypes.CurrentFormat, Chunks: 5},
+ 			},
+ 		},
+ 		"custom": {
+@@ -702,8 +702,8 @@ func TestSnapshotWithPruning(t *testing.T) {
+ 				pruningOpts:        pruningtypes.NewCustomPruningOptions(12, 12),
+ 			},
+ 			expectedSnapshots: []*abci.Snapshot{
+-				{Height: 25, Format: 2, Chunks: 6},
+-				{Height: 20, Format: 2, Chunks: 5},
++				{Height: 25, Format: snapshottypes.CurrentFormat, Chunks: 6},
++				{Height: 20, Format: snapshottypes.CurrentFormat, Chunks: 5},
+ 			},
+ 		},
+ 		"no snapshots": {
+@@ -724,9 +724,9 @@ func TestSnapshotWithPruning(t *testing.T) {
+ 				pruningOpts:        pruningtypes.NewPruningOptions(pruningtypes.PruningNothing),
+ 			},
+ 			expectedSnapshots: []*abci.Snapshot{
+-				{Height: 9, Format: 2, Chunks: 2},
+-				{Height: 6, Format: 2, Chunks: 2},
+-				{Height: 3, Format: 2, Chunks: 1},
++				{Height: 9, Format: snapshottypes.CurrentFormat, Chunks: 2},
++				{Height: 6, Format: snapshottypes.CurrentFormat, Chunks: 2},
++				{Height: 3, Format: snapshottypes.CurrentFormat, Chunks: 1},
+ 			},
+ 		},
+ 	}
+@@ -788,7 +788,7 @@ func TestLoadSnapshotChunk(t *testing.T) {
+ 		blocks:             2,
+ 		blockTxs:           5,
+ 		snapshotInterval:   2,
+-		snapshotKeepRecent: 2,
++		snapshotKeepRecent: snapshottypes.CurrentFormat,
+ 		pruningOpts:        pruningtypes.NewPruningOptions(pruningtypes.PruningNothing),
+ 	}
+ 	app, err := setupBaseAppWithSnapshots(t, setupConfig)
+@@ -802,7 +802,7 @@ func TestLoadSnapshotChunk(t *testing.T) {
  	}{
--		"Existing snapshot": {2, 1, 1, false},
--		"Missing height":    {100, 1, 1, true},
--		"Missing format":    {2, 2, 1, true},
--		"Missing chunk":     {2, 1, 9, true},
--		"Zero height":       {0, 1, 1, true},
-+		"Existing snapshot": {2, 2, 1, false},
-+		"Missing height":    {100, 2, 1, true},
-+		"Missing format":    {2, 3, 1, true},
-+		"Missing chunk":     {2, 2, 9, true},
-+		"Zero height":       {0, 2, 1, true},
+ 		"Existing snapshot": {2, snapshottypes.CurrentFormat, 1, false},
+ 		"Missing height":    {100, snapshottypes.CurrentFormat, 1, true},
+-		"Missing format":    {2, 3, 1, true},
++		"Missing format":    {2, snapshottypes.CurrentFormat + 1, 1, true},
+ 		"Missing chunk":     {2, snapshottypes.CurrentFormat, 9, true},
+ 		"Zero height":       {0, snapshottypes.CurrentFormat, 1, true},
  		"Zero format":       {2, 0, 1, true},
--		"Zero chunk":        {2, 1, 0, false},
-+		"Zero chunk":        {2, 2, 0, false},
- 	}
- 	for name, tc := range testcases {
- 		tc := tc
 diff --git a/snapshots/helpers_test.go b/snapshots/helpers_test.go
-index 7eb2aeae9..d84ae45c7 100644
+index 5a9e44daf..449b95739 100644
 --- a/snapshots/helpers_test.go
 +++ b/snapshots/helpers_test.go
 @@ -90,7 +90,6 @@ func snapshotItems(items [][]byte, ext snapshottypes.ExtensionSnapshotter) [][]b
  			return snapshottypes.WriteExtensionPayload(protoWriter, payload)
  		})
- 		protoWriter.Close()
--		zWriter.Close()
- 		bufWriter.Flush()
- 		chunkWriter.Close()
+ 		_ = protoWriter.Close()
+-		_ = zWriter.Close()
+ 		_ = bufWriter.Flush()
+ 		_ = chunkWriter.Close()
  	}()
-@@ -150,11 +149,11 @@ func (m *mockSnapshotter) Snapshot(height uint64, protoWriter protoio.Writer) er
- }
- 
- func (m *mockSnapshotter) SnapshotFormat() uint32 {
--	return 1
-+	return 2
- }
- 
- func (m *mockSnapshotter) SupportedFormats() []uint32 {
--	return []uint32{1}
-+	return []uint32{2}
- }
- 
- // setupBusyManager creates a manager with an empty store that is busy creating a snapshot at height 1.
 diff --git a/snapshots/manager_test.go b/snapshots/manager_test.go
-index e8d02ca28..efd3a7f78 100644
+index 58a302e87..ee4c3b471 100644
 --- a/snapshots/manager_test.go
 +++ b/snapshots/manager_test.go
-@@ -83,7 +83,7 @@ func TestManager_Take(t *testing.T) {
+@@ -95,7 +95,7 @@ func TestManager_Take(t *testing.T) {
  		Height: 5,
  		Format: snapshotter.SnapshotFormat(),
  		Chunks: 1,
 -		Hash:   []uint8{0x89, 0xfa, 0x18, 0xbc, 0x5a, 0xe3, 0xdc, 0x36, 0xa6, 0x95, 0x5, 0x17, 0xf9, 0x2, 0x1a, 0x55, 0x36, 0x16, 0x5d, 0x4b, 0x8b, 0x2b, 0x3d, 0xfd, 0xe, 0x2f, 0xb6, 0x40, 0x6b, 0xc3, 0xbc, 0x23},
 +		Hash:   []uint8{0xc5, 0xf7, 0xfe, 0xea, 0xd3, 0x4d, 0x3e, 0x87, 0xff, 0x41, 0xa2, 0x27, 0xfa, 0xcb, 0x38, 0x17, 0xa, 0x5, 0xeb, 0x27, 0x4e, 0x16, 0x5e, 0xf3, 0xb2, 0x8b, 0x47, 0xd1, 0xe6, 0x94, 0x7e, 0x8b},
  		Metadata: types.Metadata{
  			ChunkHashes: checksums(expectChunks),
  		},
-@@ -162,7 +162,7 @@ func TestManager_Restore(t *testing.T) {
- 	// Starting a restore works
- 	err = manager.Restore(types.Snapshot{
- 		Height:   3,
--		Format:   1,
-+		Format:   2,
- 		Hash:     []byte{1, 2, 3},
- 		Chunks:   1,
- 		Metadata: types.Metadata{ChunkHashes: checksums(chunks)},
-@@ -211,7 +211,7 @@ func TestManager_Restore(t *testing.T) {
- 	target.items = nil
- 	err = manager.Restore(types.Snapshot{
- 		Height:   3,
--		Format:   1,
-+		Format:   2,
- 		Hash:     []byte{1, 2, 3},
- 		Chunks:   1,
- 		Metadata: types.Metadata{ChunkHashes: checksums(chunks)},
 diff --git a/snapshots/stream.go b/snapshots/stream.go
-index 80cd5c3df..ba622ebfb 100644
+index 88bef0768..fa61e1457 100644
 --- a/snapshots/stream.go
 +++ b/snapshots/stream.go
 @@ -57,10 +57,6 @@ func (sw *StreamWriter) Close() error {
  		sw.chunkWriter.CloseWithError(err)
  		return err
  	}
 -	if err := sw.zWriter.Close(); err != nil {
 -		sw.chunkWriter.CloseWithError(err)
 -		return err
 -	}
  	if err := sw.bufWriter.Flush(); err != nil {
  		sw.chunkWriter.CloseWithError(err)
  		return err
 diff --git a/snapshots/types/format.go b/snapshots/types/format.go
-index edfdb36d7..d5e960660 100644
+index d5e960660..317b6a6e3 100644
 --- a/snapshots/types/format.go
 +++ b/snapshots/types/format.go
 @@ -3,4 +3,4 @@ package types
  // CurrentFormat is the currently used format for snapshots. Snapshots using the same format
  // must be identical across all nodes for a given height, so this must be bumped when the binary
  // snapshot output changes.
--const CurrentFormat uint32 = 1
-+const CurrentFormat uint32 = 2
+-const CurrentFormat uint32 = 2
++const CurrentFormat uint32 = 3
 diff --git a/store/rootmulti/snapshot_test.go b/store/rootmulti/snapshot_test.go
-index 7a3071940..48cd4ff8f 100644
+index bad1603da..de7880788 100644
 --- a/store/rootmulti/snapshot_test.go
 +++ b/store/rootmulti/snapshot_test.go
 @@ -128,7 +128,7 @@ func TestMultistoreSnapshot_Checksum(t *testing.T) {
  			"aa048b4ee0f484965d7b3b06822cf0772cdcaad02f3b1b9055e69f2cb365ef3c",
  			"7921eaa3ed4921341e504d9308a9877986a879fe216a099c86e8db66fcba4c63",
  			"a4a864e6c02c9fca5837ec80dc84f650b25276ed7e4820cf7516ced9f9901b86",
 -			"ca2879ac6e7205d257440131ba7e72bef784cd61642e32b847729e543c1928b9",
 +			"980925390cc50f14998ecb1e87de719ca9dd7e72f5fefbe445397bf670f36c31",
  		}},
  	}

@mhofman mhofman marked this pull request as ready for review March 12, 2023 14:48
Copy link
Collaborator

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

As discussed earlier, LGTM! I hope CI can be green again before landing, but I'm alright with landing this first if @JimLarson concurs.

adu-web3 and others added 4 commits March 23, 2023 03:50
Closes: cosmos#7340

This is the initial draft of ADR-049 state sync hooks based on the discussion of all parties and the proposal from @yihuang  in cosmos#7340 and the implementation in cosmos#10961.

(cherry picked from commit d10034f)
* Make extension snapshotter interface safer to use

Closes: cosmos#11824
Solution:
- Use new methods `SnapshotExtension`/`RestoreExtension` to handle payload stream specifically.
- Improve unit tests.

* update changelog

* Update snapshots/types/util.go

* changelog

* go linter

* Update CHANGELOG.md

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

(cherry picked from commit e397434)
(cherry picked from commit dcb0c9c)
Allow manual snapshot making
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants