Skip to content

Commit

Permalink
chore: move cip-24 to review (#208)
Browse files Browse the repository at this point in the history
* chore: move cip-24 to review

* docs: add first few bits of reference implementation

* docs: add reference implementation section

* docs: change it to diffs
  • Loading branch information
ninabarbakadze authored Sep 24, 2024
1 parent b7ca0c9 commit d51851a
Showing 1 changed file with 43 additions and 2 deletions.
45 changes: 43 additions & 2 deletions cips/cip-24.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
| description | Transition to hard fork-only updates for gas scheduler variables |
| author | Nina Barbakadze ([@ninabarbakadze](https://github.com/ninabarbakadze)) |
| discussions-to | <https://forum.celestia.org/t/cip-versioned-gas-scheduler-variables/1785> |
| status | Draft |
| status | Review |
| type | Standards Track |
| category | Core |
| created | 2024-07-24 |
Expand Down Expand Up @@ -45,7 +45,48 @@ Test cases should verify that gas scheduler variables are exclusively updated vi

## Reference Implementation

TBC
Starting from v3, we updated the `PayForBlobs()` function in `x/blob/keeper.go` to use versioned `GasPerBlobByte` parameter when calculating gas based on the size of the blobs while maintaining compatibility with previous versions.

```diff
- gasToConsume := types.GasToConsume(msg.BlobSizes, k.GasPerBlobByte(ctx))
+ // GasPerBlobByte is a versioned param from version 3 onwards.
+ var gasToConsume uint64
+ if ctx.BlockHeader().Version.App <= v2.Version {
+ gasToConsume = types.GasToConsume(msg.BlobSizes, k.GasPerBlobByte(ctx))
+ } else {
+ gasToConsume = types.GasToConsume(msg.BlobSizes, appconsts.GasPerBlobByte(ctx.BlockHeader().Version.App))
+ }
+
```

Additionally, we modified the PFB gas estimation logic to use `appconsts.DefaultTxSizeCostPerByte`.

```diff
-// DefaultEstimateGas runs EstimateGas with the system defaults. The network may change these values
-// through governance, thus this function should predominantly be used in testing.
+// DefaultEstimateGas runs EstimateGas with the system defaults.
func DefaultEstimateGas(blobSizes []uint32) uint64 {
- return EstimateGas(blobSizes, appconsts.DefaultGasPerBlobByte, auth.DefaultTxSizeCostPerByte)
+ return EstimateGas(blobSizes, appconsts.DefaultGasPerBlobByte, appconsts.DefaultTxSizeCostPerByte)
}
```

We also needed to update the gas consumption logic related to transaction size in the ante handler. The `AnteHandle` function within `NewConsumeGasForTxSizeDecorator` has been modified to retrieve the `TxSizeCostPerByte` value from app constants for versions v3 and later. The logic for earlier versions remains unchanged.

```diff
+// consumeGasForTxSize consumes gas based on the size of the transaction.
+// It uses different parameters depending on the app version.
+func consumeGasForTxSize(ctx sdk.Context, txBytes uint64, params auth.Params) {
+ // For app v2 and below we should get txSizeCostPerByte from auth module
+ if ctx.BlockHeader().Version.App <= v2.Version {
+ ctx.GasMeter().ConsumeGas(params.TxSizeCostPerByte*txBytes, "txSize")
+ } else {
+ // From v3 onwards, we should get txSizeCostPerByte from appconsts
+ txSizeCostPerByte := appconsts.TxSizeCostPerByte(ctx.BlockHeader().Version.App)
+ ctx.GasMeter().ConsumeGas(txSizeCostPerByte*txBytes, "txSize")
+ }
+}
```

## Security Considerations

Expand Down

0 comments on commit d51851a

Please sign in to comment.