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

Don't panic during any store operations. #5294

Merged
merged 4 commits into from
Dec 4, 2023
Merged

Don't panic during any store operations. #5294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
783e4e2
Don't panic during any store operations.
DimitrisJim Dec 1, 2023
c981859
Panic when creating wrapped store if substores are nil.
DimitrisJim Dec 4, 2023
38764f7
Address Damian's feedback.
DimitrisJim Dec 4, 2023
dac06c5
Merge branch 'main' into jim/5282-remove-panics-in-wrapped-store
DimitrisJim Dec 4, 2023
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
2 changes: 1 addition & 1 deletion modules/light-clients/08-wasm/types/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func NewMigrateProposalWrappedStore(subjectStore, substituteStore storetypes.KVS
}

// GetStore is a wrapper around getStore to allow the function to be directly called in tests.
func (ws migrateClientWrappedStore) GetStore(key []byte) storetypes.KVStore {
func (ws migrateClientWrappedStore) GetStore(key []byte) (storetypes.KVStore, bool) {
return ws.getStore(key)
}

Expand Down
83 changes: 61 additions & 22 deletions modules/light-clients/08-wasm/types/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package types
import (
"bytes"
"errors"
"fmt"
"io"
"reflect"
"strings"
Expand All @@ -29,13 +28,20 @@ var (
//
// Both stores are used for reads, but only the subjectStore is used for writes. For all operations, the key
// is checked to determine which store to use and must be prefixed with either "subject/" or "substitute/" accordingly.
// If the key is not prefixed with either "subject/" or "substitute/", a panic is thrown.
// If the key is not prefixed with either "subject/" or "substitute/", a default action is taken (e.g. no-op for Set/Delete).
type migrateClientWrappedStore struct {
subjectStore storetypes.KVStore
substituteStore storetypes.KVStore
}

func newMigrateClientWrappedStore(subjectStore, substituteStore storetypes.KVStore) migrateClientWrappedStore {
if subjectStore == nil {
panic(errors.New("subjectStore must not be nil"))
}
if substituteStore == nil {
panic(errors.New("substituteStore must not be nil"))
}

return migrateClientWrappedStore{
subjectStore: subjectStore,
substituteStore: substituteStore,
Expand All @@ -44,11 +50,17 @@ func newMigrateClientWrappedStore(subjectStore, substituteStore storetypes.KVSto

// Get implements the storetypes.KVStore interface. It allows reads from both the subjectStore and substituteStore.
//
// Get will panic if the key is not prefixed with either "subject/" or "substitute/".
// Get will return an empty byte slice if the key is not prefixed with either "subject/" or "substitute/".
func (ws migrateClientWrappedStore) Get(key []byte) []byte {
prefix, key := splitPrefix(key)

return ws.getStore(prefix).Get(key)
store, found := ws.getStore(prefix)
if !found {
// return a nil byte slice as KVStore.Get() does by default
return []byte(nil)
}

return store.Get(key)
}

// Has implements the storetypes.KVStore interface. It allows reads from both the subjectStore and substituteStore.
Expand All @@ -57,59 +69,75 @@ func (ws migrateClientWrappedStore) Get(key []byte) []byte {
func (ws migrateClientWrappedStore) Has(key []byte) bool {
prefix, key := splitPrefix(key)

return ws.getStore(prefix).Has(key)
store, found := ws.getStore(prefix)
if !found {
// return false as value when store is not found
return false
}

return store.Has(key)
}

// Set implements the storetypes.KVStore interface. It allows writes solely to the subjectStore.
//
// Set will panic if the key is not prefixed with "subject/".
// Set will no-op if the key is not prefixed with "subject/".
func (ws migrateClientWrappedStore) Set(key, value []byte) {
prefix, key := splitPrefix(key)
if !bytes.Equal(prefix, subjectPrefix) {
panic(fmt.Errorf("writes only allowed on subject store; key must be prefixed with \"%s\"", subjectPrefix))
return // no-op
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tho I must say one thing I don't like about this approach is that now errors are just as silent as can be

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes... Would there be some check we could do in 08-wasm's CheckSubstituteAndUpdateState after calling the contract to validate that the subject client state has been updated with the substitute's? Maybe checking the status of the subject client? If the call to the contract succeeded, then we would expect the subject client to be active again, so if the call to the contract succeeds, but the subject client is not active, then we could maybe maybe hint that the problem might be here?

Similarly, maybe there's also a check on the consensus state that we could do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think the check for the status could be done in 02-client layer? For 07-tendermint this is done implicitly considering we check the subject status before calling CheckSubstituteAndUpdateState.

As for consensus state checks, is this an assumption we shouldn't be making? I.e solomachine precedent.

}

ws.subjectStore.Set(key, value)
}

// Delete implements the storetypes.KVStore interface. It allows deletions solely to the subjectStore.
//
// Delete will panic if the key is not prefixed with "subject/".
// Delete will no-op if the key is not prefixed with "subject/".
func (ws migrateClientWrappedStore) Delete(key []byte) {
prefix, key := splitPrefix(key)
if !bytes.Equal(prefix, subjectPrefix) {
panic(fmt.Errorf("writes only allowed on subject store; key must be prefixed with \"%s\"", subjectPrefix))
return // no-op
}

ws.subjectStore.Delete(key)
}

// Iterator implements the storetypes.KVStore interface. It allows iteration over both the subjectStore and substituteStore.
//
// Iterator will panic if the start or end keys are not prefixed with either "subject/" or "substitute/".
// Iterator will return a closed iterator if the start or end keys are not prefixed with either "subject/" or "substitute/".
func (ws migrateClientWrappedStore) Iterator(start, end []byte) storetypes.Iterator {
prefixStart, start := splitPrefix(start)
prefixEnd, end := splitPrefix(end)

if !bytes.Equal(prefixStart, prefixEnd) {
panic(errors.New("start and end keys must be prefixed with the same prefix"))
return ws.closedIterator()
}

return ws.getStore(prefixStart).Iterator(start, end)
store, found := ws.getStore(prefixStart)
if !found {
return ws.closedIterator()
}

return store.Iterator(start, end)
}

// ReverseIterator implements the storetypes.KVStore interface. It allows iteration over both the subjectStore and substituteStore.
//
// ReverseIterator will panic if the start or end keys are not prefixed with either "subject/" or "substitute/".
// ReverseIterator will return a closed iterator if the start or end keys are not prefixed with either "subject/" or "substitute/".
func (ws migrateClientWrappedStore) ReverseIterator(start, end []byte) storetypes.Iterator {
prefixStart, start := splitPrefix(start)
prefixEnd, end := splitPrefix(end)

if !bytes.Equal(prefixStart, prefixEnd) {
panic(errors.New("start and end keys must be prefixed with the same prefix"))
return ws.closedIterator()
}

store, found := ws.getStore(prefixStart)
if !found {
return ws.closedIterator()
}

return ws.getStore(prefixStart).ReverseIterator(start, end)
return store.ReverseIterator(start, end)
}

// GetStoreType implements the storetypes.KVStore interface, it is implemented solely to satisfy the interface.
Expand All @@ -127,18 +155,29 @@ func (ws migrateClientWrappedStore) CacheWrapWithTrace(w io.Writer, tc storetype
return cachekv.NewStore(tracekv.NewStore(ws, w, tc))
}

// getStore returns the store to be used for the given key. If the key is prefixed with "subject/", the subjectStore
// is returned. If the key is prefixed with "substitute/", the substituteStore is returned.
// getStore returns the store to be used for the given key and a boolean flag indicating if that store was found.
// If the key is prefixed with "subject/", the subjectStore is returned. If the key is prefixed with "substitute/",
// the substituteStore is returned.
//
// If the key is not prefixed with either "subject/" or "substitute/", a panic is thrown.
func (ws migrateClientWrappedStore) getStore(prefix []byte) storetypes.KVStore {
// If the key is not prefixed with either "subject/" or "substitute/", a nil store is returned and the boolean flag is false.
func (ws migrateClientWrappedStore) getStore(prefix []byte) (storetypes.KVStore, bool) {
if bytes.Equal(prefix, subjectPrefix) {
return ws.subjectStore
return ws.subjectStore, true
} else if bytes.Equal(prefix, substitutePrefix) {
return ws.substituteStore
return ws.substituteStore, true
}

panic(fmt.Errorf("key must be prefixed with either \"%s\" or \"%s\"", subjectPrefix, substitutePrefix))
return nil, false
}

// closedIterator returns an iterator that is always closed, used when Iterator() or ReverseIterator() is called
// with an invalid prefix or start/end key.
func (ws migrateClientWrappedStore) closedIterator() storetypes.Iterator {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could also return invalid one directly (start == end) if people think that would be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

there's no danger in returning a closed iterator, right? it will just be false on iter.Valid() and iter.Next(), and return nilon calls toKey()andValue()`

Copy link
Contributor Author

@DimitrisJim DimitrisJim Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

key/value/next should panic as calls, according to Iterator interface. Concretely, if they don't call Valid before trying to iterate, we can't help em is my view 🤷

// Create a dummy iterator that is always closed right away.
it := ws.subjectStore.Iterator([]byte{0}, []byte{1})
it.Close()

return it
}

// splitPrefix splits the key into the prefix and the key itself, if the key is prefixed with either "subject/" or "substitute/".
Expand Down
Loading