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.

Sign up for GitHub

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

fix(store/v2): don't delete future version when calling LoadVersion #22681

Merged
merged 7 commits into from
Nov 30, 2024
Merged

fix(store/v2): don't delete future version when calling LoadVersion #22681

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c379dcf
fix(store/v2): don't delete future version when calling LoadVersion
julienrbrt Nov 28, 2024
6a086bd
Merge branch 'main' into julien/scfix
julienrbrt Nov 29, 2024
824ab15
add api
julienrbrt Nov 29, 2024
7c0c71d
fix
julienrbrt Nov 30, 2024
1e2f13f
updates
julienrbrt Nov 30, 2024
91128cd
updates
julienrbrt Nov 30, 2024
deeb32d
fix
julienrbrt Nov 30, 2024
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
7 changes: 7 additions & 0 deletions store/v2/commitment/iavl/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ func (t *IavlTree) WorkingHash() []byte {

// LoadVersion loads the state at the given version.
func (t *IavlTree) LoadVersion(version uint64) error {
_, err := t.tree.LoadVersion(int64(version))
return err
}

// LoadVersionForOverwriting loads the state at the given version.
// Any versions greater than targetVersion will be deleted.
func (t *IavlTree) LoadVersionForOverwriting(version uint64) error {
return t.tree.LoadVersionForOverwriting(int64(version))
}

Expand Down
4 changes: 4 additions & 0 deletions store/v2/commitment/mem/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ func (t *Tree) LoadVersion(version uint64) error {
return nil
}

func (t *Tree) LoadVersionForOverwriting(version uint64) error {
return nil
}

func (t *Tree) Commit() ([]byte, uint64, error) {
return nil, 0, nil
}
Expand Down
25 changes: 20 additions & 5 deletions store/v2/commitment/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,16 @@
for storeKey := range c.multiTrees {
storeKeys = append(storeKeys, storeKey)
}
return c.loadVersion(targetVersion, storeKeys)
return c.loadVersion(targetVersion, storeKeys, false)
}

func (c *CommitStore) LoadVersionForOverwriting(targetVersion uint64) error {
storeKeys := make([]string, 0, len(c.multiTrees))
for storeKey := range c.multiTrees {
storeKeys = append(storeKeys, storeKey)
}
Comment on lines +95 to +97

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism

return c.loadVersion(targetVersion, storeKeys, true)
}

// LoadVersionAndUpgrade implements store.UpgradeableStore.
Expand Down Expand Up @@ -133,10 +142,10 @@
return err
}

return c.loadVersion(targetVersion, newStoreKeys)
return c.loadVersion(targetVersion, newStoreKeys, true)
}

func (c *CommitStore) loadVersion(targetVersion uint64, storeKeys []string) error {
func (c *CommitStore) loadVersion(targetVersion uint64, storeKeys []string, overrideAfter bool) error {
// Rollback the metadata to the target version.
latestVersion, err := c.GetLatestVersion()
if err != nil {
Expand All @@ -154,8 +163,14 @@
}

for _, storeKey := range storeKeys {
if err := c.multiTrees[storeKey].LoadVersion(targetVersion); err != nil {
return err
if overrideAfter {
if err := c.multiTrees[storeKey].LoadVersionForOverwriting(targetVersion); err != nil {
return err
}
} else {
if err := c.multiTrees[storeKey].LoadVersion(targetVersion); err != nil {
return err
}
}
}

Expand Down
1 change: 1 addition & 0 deletions store/v2/commitment/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type Tree interface {
Version() uint64

LoadVersion(version uint64) error
LoadVersionForOverwriting(version uint64) error
Commit() ([]byte, uint64, error)
SetInitialVersion(version uint64) error
GetProof(version uint64, key []byte) (*ics23.CommitmentProof, error)
Expand Down
4 changes: 4 additions & 0 deletions store/v2/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ type Committer interface {
// LoadVersion loads the tree at the given version.
LoadVersion(targetVersion uint64) error

// LoadVersionForOverwriting loads the tree at the given version.
// Any versions greater than targetVersion will be deleted.
LoadVersionForOverwriting(targetVersion uint64) error

// Commit commits the working tree to the database.
Commit(version uint64) (*proof.CommitInfo, error)

Expand Down
14 changes: 14 additions & 0 deletions store/v2/mock/db_mock.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 21 additions & 6 deletions store/v2/root/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@
return err
}

return s.loadVersion(lv, nil)
return s.loadVersion(lv, nil, false)
}

func (s *Store) LoadVersion(version uint64) error {
Expand All @@ -259,7 +259,16 @@
defer s.telemetry.MeasureSince(now, "root_store", "load_version")
}

return s.loadVersion(version, nil)
return s.loadVersion(version, nil, false)
}

func (s *Store) LoadVersionForOverwriting(version uint64) error {
if s.telemetry != nil {
now := time.Now()

Check warning

Code scanning / CodeQL

Calling the system time Warning

Calling the system time may be a possible source of non-determinism
defer s.telemetry.MeasureSince(now, "root_store", "load_version_for_overwriting")
}

return s.loadVersion(version, nil, true)
}

// LoadVersionAndUpgrade implements the UpgradeableStore interface.
Expand All @@ -278,7 +287,7 @@
return errors.New("cannot upgrade while migrating")
}

if err := s.loadVersion(version, upgrades); err != nil {
if err := s.loadVersion(version, upgrades, true); err != nil {
return err
}

Expand All @@ -294,12 +303,18 @@
return nil
}

func (s *Store) loadVersion(v uint64, upgrades *corestore.StoreUpgrades) error {
func (s *Store) loadVersion(v uint64, upgrades *corestore.StoreUpgrades, overrideAfter bool) error {
s.logger.Debug("loading version", "version", v)

if upgrades == nil {
if err := s.stateCommitment.LoadVersion(v); err != nil {
return fmt.Errorf("failed to load SC version %d: %w", v, err)
if !overrideAfter {
if err := s.stateCommitment.LoadVersion(v); err != nil {
return fmt.Errorf("failed to load SC version %d: %w", v, err)
}
} else {
if err := s.stateCommitment.LoadVersionForOverwriting(v); err != nil {
return fmt.Errorf("failed to load SC version %d: %w", v, err)
}
Comment on lines +306 to +317
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid using boolean parameters to control function behavior

The loadVersion function now includes a boolean parameter overrideAfter that alters its behavior. According to the Uber Go Style Guide, it's recommended to avoid boolean parameters that change the function's behavior because they can make the code less readable and harder to maintain.

Suggestion:

Consider refactoring the function to eliminate the boolean parameter. One approach is to split loadVersion into two separate functions with clear and descriptive names:

  1. loadVersion – retains the original behavior without overwriting future versions.
  2. loadVersionForOverwriting – handles loading versions with overwriting.

Refactored code:

func (s *Store) loadVersion(v uint64, upgrades *corestore.StoreUpgrades) error {
    // Existing behavior without overwriting
    // ...
}

func (s *Store) loadVersionForOverwriting(v uint64, upgrades *corestore.StoreUpgrades) error {
    // Behavior that allows overwriting future versions
    // ...
}

This separation enhances clarity by making the function's purpose explicit and aligns with best practices.

}
} else {
// if upgrades are provided, we need to load the version and apply the upgrades
Expand Down
68 changes: 68 additions & 0 deletions store/v2/root/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,74 @@ func (s *RootStoreTestSuite) TestLoadVersion() {
s.Require().NoError(err)
s.Require().Equal([]byte("val003"), val)

// attempt to write and commit a few changesets
for v := 4; v <= 5; v++ {
val := fmt.Sprintf("overwritten_val%03d", v) // overwritten_val004, overwritten_val005

cs := corestore.NewChangeset(uint64(v))
cs.Add(testStoreKeyBytes, []byte("key"), []byte(val), false)

_, err := s.rootStore.Commit(cs)
s.Require().ErrorContains(err, "was already saved")
}

// ensure the latest version is correct
latest, err = s.rootStore.GetLatestVersion()
s.Require().NoError(err)
s.Require().Equal(uint64(3), latest) // should have stayed at 3 after failed commits

// query state and ensure values returned are based on the loaded version
_, ro, err = s.rootStore.StateLatest()
s.Require().NoError(err)

reader, err = ro.GetReader(testStoreKeyBytes)
s.Require().NoError(err)
val, err = reader.Get([]byte("key"))
s.Require().NoError(err)
s.Require().Equal([]byte("val003"), val)
}

func (s *RootStoreTestSuite) TestLoadVersionForOverwriting() {
// write and commit a few changesets
for v := uint64(1); v <= 5; v++ {
val := fmt.Sprintf("val%03d", v) // val001, val002, ..., val005

cs := corestore.NewChangeset(v)
cs.Add(testStoreKeyBytes, []byte("key"), []byte(val), false)

commitHash, err := s.rootStore.Commit(cs)
s.Require().NoError(err)
s.Require().NotNil(commitHash)
}

// ensure the latest version is correct
latest, err := s.rootStore.GetLatestVersion()
s.Require().NoError(err)
s.Require().Equal(uint64(5), latest)

// attempt to load a non-existent version
err = s.rootStore.LoadVersionForOverwriting(6)
s.Require().Error(err)

// attempt to load a previously committed version
err = s.rootStore.LoadVersionForOverwriting(3)
s.Require().NoError(err)

// ensure the latest version is correct
latest, err = s.rootStore.GetLatestVersion()
s.Require().NoError(err)
s.Require().Equal(uint64(3), latest)

// query state and ensure values returned are based on the loaded version
_, ro, err := s.rootStore.StateLatest()
s.Require().NoError(err)

reader, err := ro.GetReader(testStoreKeyBytes)
s.Require().NoError(err)
val, err := reader.Get([]byte("key"))
s.Require().NoError(err)
s.Require().Equal([]byte("val003"), val)

// attempt to write and commit a few changesets
for v := 4; v <= 5; v++ {
val := fmt.Sprintf("overwritten_val%03d", v) // overwritten_val004, overwritten_val005
Expand Down
4 changes: 4 additions & 0 deletions store/v2/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ type RootStore interface {
// LoadVersion loads the RootStore to the given version.
LoadVersion(version uint64) error

// LoadVersionForOverwriting loads the state at the given version.
// Any versions greater than targetVersion will be deleted.
LoadVersionForOverwriting(version uint64) error

// LoadLatestVersion behaves identically to LoadVersion except it loads the
// latest version implicitly.
LoadLatestVersion() error
Expand Down
Loading