Fix Begin/EndBlock stuck at infinite loop in contracts#457
Conversation
| defer metrics.MeasureSudoExecutionDuration(time.Now(), msgType) | ||
| // set up a tmp context to prevent race condition in reading gas consumed | ||
| tmpCtx := sdkCtx.WithGasMeter(sdk.NewInfiniteGasMeter()) | ||
| tmpCtx := sdkCtx.WithGasMeter(sdk.NewGasMeter(sdkCtx.GasMeter().Limit())) |
There was a problem hiding this comment.
Where is this gas limit determined for begin / end block?
There was a problem hiding this comment.
it's set at the beginning of Begin/EndBlock (and it merely infers the value here): https://github.com/sei-protocol/sei-chain/blob/master/x/dex/module.go#L234 and https://github.com/sei-protocol/sei-chain/blob/master/x/dex/contract/abci.go#L45
There was a problem hiding this comment.
This doesn't account for the already spent gas though, right? because lets say gas limit is 1000, but the beginblock already spent 250, and the wasm contract needs 800. Then with this current PR, it would allow a total consumption of 1050 gas, right? or is that fine since we're only looking to mitigate the infinite gas issue?
There was a problem hiding this comment.
that's intentional. We don't want to take into account already spent gas because otherwise it may cause non-determinism - contract A may be processed and consume most gas in val 1 and contract B would fail because it only has little gas remaining whereas it could be opposite in val 2.
There was a problem hiding this comment.
So IUCC in some cases (example Uday listed above) may perform more computation than the original gas limit but it will still fail later when we check the gas consumed?
gasConsumed := tmpCtx.GasMeter().GasConsumed()
if gasConsumed > 0 {
sdkCtx.GasMeter().ConsumeGas(gasConsumed, "sudo")
}
If so, it might be useful to add a comment here for future reference.
There was a problem hiding this comment.
I don't think it would fail later because of greater than expected gas consumption, because the gas being consumed by contracts is on a new meter, so doesn't actually affect the original gas meter. I was just wondering whether that was ok considering that gas is generally tied to compute cost but this seems fine.
There was a problem hiding this comment.
yeah the gas limit is more like "no individual call can exceed it", but a soft limit overall. I'll add a comment for this
|
## Describe your changes and provide context
When we create tokens using token factory, denoms are created in the
`factory/{owner_walltet_id}/{denom}` format
E.g. `factory/sei1gxskuzvhr4s8sdm2rpruaf7yx2dnmjn0zfdu9q/NEWCOIN`
When querying metadata via
`/cosmos/bank/v1beta1/denoms_metadata/{denom}` endpoint for such a denom
we are getting error like
```
{
"code": 12,
"message": "Not Implemented",
"details": []
}
```
This happens, because routing logic splits the denom uri param into 3
components rather then one and hence tries to router request to a
non-existing handler. Url encoding does not help either as it still
results in same issue. We would like to add additional endpoint to
handle token factory created metadata retrieval, and move denom for the
endpoint from uri param to query param.
so then for token factory created tokens the query would be
`/cosmos/bank/v1beta1/token_factory/denoms_metadata?denom=factory/{owner_walltet_id}/{denom}`
## Testing performed to validate your change
Note: some demom tests were disables, and I have enabled them.
- Functional testing on local node with updated source
- unit/integration tests
## Describe your changes and provide context
When we create tokens using token factory, denoms are created in the
`factory/{owner_walltet_id}/{denom}` format
E.g. `factory/sei1gxskuzvhr4s8sdm2rpruaf7yx2dnmjn0zfdu9q/NEWCOIN`
When querying metadata via
`/cosmos/bank/v1beta1/denoms_metadata/{denom}` endpoint for such a denom
we are getting error like
```
{
"code": 12,
"message": "Not Implemented",
"details": []
}
```
This happens, because routing logic splits the denom uri param into 3
components rather then one and hence tries to router request to a
non-existing handler. Url encoding does not help either as it still
results in same issue. We would like to add additional endpoint to
handle token factory created metadata retrieval, and move denom for the
endpoint from uri param to query param.
so then for token factory created tokens the query would be
`/cosmos/bank/v1beta1/token_factory/denoms_metadata?denom=factory/{owner_walltet_id}/{denom}`
## Testing performed to validate your change
Note: some demom tests were disables, and I have enabled them.
- Functional testing on local node with updated source
- unit/integration tests
3 participants
Describe your changes and provide context
The
wasmmodule would allow contracts to run forever ifctx.GasMeter()doesn't have a limit. So instead of using an infinite meter as the temporary meter (which was originally added to prevent race conditions between concurrent calls), we will use one that inherits the limit depending on whether it's called within BeginBlock or EndBlock (inferred fromctx.GasMeter().Limit().Testing performed to validate your change
Tested on local chain with contract that has infinite loop in hooks