Don't panic during any store operations.#5294
Conversation
DimitrisJim
force-pushed
the
jim/5282-remove-panics-in-wrapped-store
branch
from
53fdf45 to
e1e6e67
Compare
DimitrisJim
requested review from
AdityaSripal,
charleenfei,
chatton,
colin-axner,
damiannolan and
srdtrk
as code owners
|
|
||
| // 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 { |
There was a problem hiding this comment.
could also return invalid one directly (start == end) if people think that would be better?
There was a problem hiding this comment.
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()`
There was a problem hiding this comment.
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 🤷
| 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 |
There was a problem hiding this comment.
tho I must say one thing I don't like about this approach is that now errors are just as silent as can be
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
crodriguezvega
left a comment
crodriguezvega
left a comment
There was a problem hiding this comment.
Thanks, @DimitrisJim. I think this all looks good. Left just a couple of comments.
| 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 |
There was a problem hiding this comment.
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.
DimitrisJim
force-pushed
the
jim/5282-remove-panics-in-wrapped-store
branch
from
e1e6e67 to
783e4e2
Compare
DimitrisJim
force-pushed
the
jim/5282-remove-panics-in-wrapped-store
branch
from
d99f4a0 to
c981859
Compare
|
|
||
| // 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 { |
There was a problem hiding this comment.
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()`
damiannolan
left a comment
damiannolan
left a comment
There was a problem hiding this comment.
ACK, thanks @DimitrisJim
* Don't panic during any store operations. * Panic when creating wrapped store if substores are nil. * Address Damian's feedback. (cherry picked from commit b7d6a30) # Conflicts: # modules/light-clients/08-wasm/types/export_test.go # modules/light-clients/08-wasm/types/store_test.go
* Don't panic during any store operations. * Panic when creating wrapped store if substores are nil. * Address Damian's feedback. (cherry picked from commit b7d6a30)
* Don't panic during any store operations. (#5294) * Don't panic during any store operations. * Panic when creating wrapped store if substores are nil. * Address Damian's feedback. (cherry picked from commit b7d6a30) # Conflicts: # modules/light-clients/08-wasm/types/export_test.go # modules/light-clients/08-wasm/types/store_test.go * fix conflicts. --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
* Don't panic during any store operations. * Panic when creating wrapped store if substores are nil. * Address Damian's feedback.
3 participants
Description
should be mostly ready, wanna go through it again before opening
closes: #5282
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.