-
Notifications
You must be signed in to change notification settings - Fork 631
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: state-compatibility documentation #2514
Conversation
Todo: mention historical queries that can be pruned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome addition, well overdue. Thanks for adding this! Left a couple comments but overall I think the main points are covered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
CONTRIBUTING.md
Outdated
Version A | ||
```go | ||
func (sk Keeper) validateAmount(ctx context.Context, amount sdk.Int) error { | ||
if amount.IsNegative() { | ||
return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "amount must be positive or zero") | ||
} | ||
return nil, errors.New("error") | ||
} | ||
``` | ||
|
||
Version B | ||
```go | ||
func (sk Keeper) validateAmount(ctx context.Context, amount sdk.Int) error { | ||
if amount.IsNegative() || amount.IsZero() { | ||
return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "amount must be positive") | ||
} | ||
return nil, errors.New("error") | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this also be state incompatible for a second reason? Not only is 0 now returning a different result, but now a negative number also returns a different error response right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good observation.
It seems to me that the error message does not affect things unless we store it in state for some reason which really should never happen.
However, if we were to change error code, that would cause an issue. As per Dev's comment:
#2514 (comment)
there is a transaction response code that gets included in the LastResultsHash
. This is what the error code here seems to represent. So we should be really careful about error codes. I will add a section about that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! This is one of the documentations that we have needed!
Would it be possible to also add docs about changes of Queries and Msgs?
CONTRIBUTING.md
Outdated
Therefore, we introduced a state-incompatibility by merklezing diverging gas | ||
usage. | ||
|
||
##### Network Requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we specify what we mean by network requests here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This specific part refers to querying external services, for example, some API for getting exchange rates. This is not allowed since such API might be unavailable in some regions, rate limit or just be down.
I haven't seen anyone doing that but I think it might make sense to mention for beginners since querying an API is a common activity in other systems
|
||
#### Sources of State-incompatibility | ||
|
||
##### Creating Additional State |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also handle Changing Existing State(Ex changing of existing proto field)
CONTRIBUTING.md
Outdated
The state compatibility is ensured by taking the app hash of the state and comparing it | ||
to the app hash with the rest of the network. Essentially, an app hash is a hash of | ||
hashes of every store's Merkle root. | ||
|
||
For example, at height n, we compute: | ||
`app hash = hash(hash(root of x/epochs), hash(root of x/gamm)...)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also probably note the LastResultsHash
for tx results matters as well: https://github.com/tendermint/tendermint/blob/main/proto/tendermint/types/types.proto#L70-L77
(The other hashes are unlikely to be an issue / you don't really have to separately think about)
The LastResultsHash today contains:
https://github.com/tendermint/tendermint/blob/main/types/results.go#L47-L54
- Tx GasWanted
- Tx GasUsed
- Tx response "Data"
- Tx response "Code"
The data field includes the proto message response (therefore we cannot change these in patch releases). Tbh idk what the Code field does, as far as I can tell its not even used: https://github.com/cosmos/cosmos-sdk/blob/main/baseapp/abci.go#L298-L304
The gas used field being merkelized means that:
- right now we cannot change the order of operations within a tx execution (different gas used in edge cases of out of gas'ing)
- Therefore we must be worried about refactoring errors earlier in the tx stack
- We cannot change gas usage amounts (e.g. removing extraneous store reads)
Gas used being merkelized is slated for removal in a subsequent Tendermint release, at which point we will have more flexibility in reordering operations / erroring.
Note that these all stem from DeliverTx execution path, which handles:
- AnteHandler's marked as deliver tx
- msg.ValidateBasic
- execution of a message from the message server
AnteHandlers with CheckTx fields can always change at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Great points
re: code
Apparently, it seems to be used when an error is returned. I discussed this briefly here:
#2514 (comment)
Planning to include a few sentences on it.
CONTRIBUTING.md
Outdated
It is critical to avoid performing network requests since it is common | ||
for services to be unavailable or rate-limit. As a result, nodes | ||
may get diverging responses, leading to state breakage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrmm not sure I follow. Maybe we can clarify by stating what is the state machine scope, which is:
- All messages supported by the chain
- Tx gas usage
- Queries in {whitelist file}
- All BeginBlock/Endblock logic
Importantly the following are not state machine versioned:
- Events
- Queries not in the whitelist file
- CLI interfaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add the suggestions.
This specific part refers to querying external services, for example, some API for getting exchange rates. This is not allowed since such API might be unavailable in some regions, rate limit or just be down.
Pretty sure I covered all suggestions in the updates. Please let me know what you all think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Closes: #XXX
What is the purpose of the change
Add documentation for state-compatibility safety, patch and minor releases.
Brief Changelog
This change is a trivial rework / code cleanup without any test coverage.
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? no