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.

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

refactor!: version gas scheduler variables #3735

Merged
merged 23 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
612c3de
refactor: make versioned gas consts version specific
ninabarbakadze Aug 1, 2024
cb261f7
refactor: nits
ninabarbakadze Aug 1, 2024
3645e1b
test: remove print statements
ninabarbakadze Aug 1, 2024
e061c20
refactor: cleanup more
ninabarbakadze Aug 2, 2024
be83e24
refactor: address review comments
ninabarbakadze Aug 6, 2024
9c48d61
refactor: address review comments
ninabarbakadze Aug 7, 2024
b92566e
docs: update blob readme
ninabarbakadze Aug 7, 2024
fb07f97
chore: merge
ninabarbakadze Aug 7, 2024
a1d15e8
fix: build issues after merge
ninabarbakadze Aug 7, 2024
a3a09b8
docs: add a comment in tx_size.go
ninabarbakadze Aug 8, 2024
27b8d3a
fix: merge conflicts
ninabarbakadze Aug 8, 2024
055b706
Update keeper_test.go
ninabarbakadze Aug 8, 2024
a6e69cd
style: make linter happy
ninabarbakadze Aug 8, 2024
2ddaa9f
test: add v3 version in tx size test
ninabarbakadze Aug 8, 2024
21ec979
test: make tx size test better
ninabarbakadze Aug 9, 2024
4d4b663
merge
ninabarbakadze Sep 17, 2024
8547221
feat: apply requested changes
ninabarbakadze Sep 17, 2024
0f3282a
style: lint
ninabarbakadze Sep 17, 2024
bc08f02
test: expand ante test to account for different version logics
ninabarbakadze Sep 18, 2024
a033364
Update app/test/upgrade_test.go
ninabarbakadze Sep 18, 2024
9603fd2
Update x/blob/keeper/keeper_test.go
ninabarbakadze Sep 18, 2024
f6f08c6
Update x/blob/types/payforblob.go
ninabarbakadze Sep 18, 2024
4832d1b
test: expand tx size test to test different values
ninabarbakadze Sep 18, 2024
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 app/ante/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func NewAnteHandler(
ante.NewValidateMemoDecorator(accountKeeper),
// Ensure the tx's gas limit is > the gas consumed based on the tx size.
// Side effect: consumes gas from the gas meter.
ante.NewConsumeGasForTxSizeDecorator(accountKeeper),
NewConsumeGasForTxSizeDecorator(accountKeeper),
// Ensure the feepayer (fee granter or first signer) has enough funds to pay for the tx.
// Ensure the gas price >= network min gas price if app version >= 2.
// Side effect: deducts fees from the fee payer. Sets the tx priority in context.
Expand Down
11 changes: 5 additions & 6 deletions app/ante/min_fee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"github.com/celestiaorg/celestia-app/v3/app/ante"
"github.com/celestiaorg/celestia-app/v3/app/encoding"
"github.com/celestiaorg/celestia-app/v3/pkg/appconsts"
v2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2"
"github.com/celestiaorg/celestia-app/v3/test/util/testnode"
"github.com/celestiaorg/celestia-app/v3/x/minfee"
"github.com/cosmos/cosmos-sdk/codec"
Expand Down Expand Up @@ -58,31 +57,31 @@ func TestValidateTxFee(t *testing.T) {
{
name: "bad tx; fee below required minimum",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, feeAmount-1)),
gasLimit: uint64(float64(feeAmount) / v2.NetworkMinGasPrice),
gasLimit: uint64(float64(feeAmount) / appconsts.DefaultNetworkMinGasPrice),
appVersion: uint64(2),
isCheckTx: false,
expErr: true,
},
{
name: "good tx; fee equal to required minimum",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, feeAmount)),
gasLimit: uint64(float64(feeAmount) / v2.NetworkMinGasPrice),
gasLimit: uint64(float64(feeAmount) / appconsts.DefaultNetworkMinGasPrice),
appVersion: uint64(2),
isCheckTx: false,
expErr: false,
},
{
name: "good tx; fee above required minimum",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, feeAmount+1)),
gasLimit: uint64(float64(feeAmount) / v2.NetworkMinGasPrice),
gasLimit: uint64(float64(feeAmount) / appconsts.DefaultNetworkMinGasPrice),
appVersion: uint64(2),
isCheckTx: false,
expErr: false,
},
{
name: "good tx; with no fee (v1)",
fee: sdk.NewCoins(sdk.NewInt64Coin(appconsts.BondDenom, feeAmount)),
gasLimit: uint64(float64(feeAmount) / v2.NetworkMinGasPrice),
gasLimit: uint64(float64(feeAmount) / appconsts.DefaultNetworkMinGasPrice),
appVersion: uint64(1),
isCheckTx: false,
expErr: false,
Expand Down Expand Up @@ -143,7 +142,7 @@ func TestValidateTxFee(t *testing.T) {

ctx = ctx.WithMinGasPrices(sdk.DecCoins{validatorMinGasPriceCoin})

networkMinGasPriceDec, err := sdk.NewDecFromStr(fmt.Sprintf("%f", v2.NetworkMinGasPrice))
networkMinGasPriceDec, err := sdk.NewDecFromStr(fmt.Sprintf("%f", appconsts.DefaultNetworkMinGasPrice))
require.NoError(t, err)

subspace, _ := paramsKeeper.GetSubspace(minfee.ModuleName)
Expand Down
150 changes: 150 additions & 0 deletions app/ante/tx_size.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
package ante

import (
"encoding/hex"

"cosmossdk.io/errors"
"github.com/celestiaorg/celestia-app/v3/pkg/appconsts"
v2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2"
"github.com/cosmos/cosmos-sdk/codec/legacy"
"github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/types/tx/signing"
"github.com/cosmos/cosmos-sdk/x/auth/ante"
"github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx"
authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
auth "github.com/cosmos/cosmos-sdk/x/auth/types"
)

var (
// Simulation signature values used to estimate gas consumption.
key = make([]byte, secp256k1.PubKeySize)
simSecp256k1Pubkey = &secp256k1.PubKey{Key: key}
simSecp256k1Sig [64]byte
)

func init() {
// Decodes a valid hex string into a sepc256k1Pubkey for use in transaction simulation
bz, _ := hex.DecodeString("035AD6810A47F073553FF30D2FCC7E0D3B1C0B74B61A1AAA2582344037151E143A")
copy(key, bz)
simSecp256k1Pubkey.Key = key
}
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +29 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

Address potential API key exposure.

The static analysis tool detected a potential API key exposure in the init function. Ensure that the key used is not sensitive or replace it with a safer alternative.

-  bz, _ := hex.DecodeString("035AD6810A47F073553FF30D2FCC7E0D3B1C0B74B61A1AAA2582344037151E143A")
+  bz, _ := hex.DecodeString("000000000000000000000000000000000000000000000000000000000000000000")
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func init() {
// Decodes a valid hex string into a sepc256k1Pubkey for use in transaction simulation
bz, _ := hex.DecodeString("035AD6810A47F073553FF30D2FCC7E0D3B1C0B74B61A1AAA2582344037151E143A")
copy(key, bz)
simSecp256k1Pubkey.Key = key
}
func init() {
// Decodes a valid hex string into a sepc256k1Pubkey for use in transaction simulation
bz, _ := hex.DecodeString("000000000000000000000000000000000000000000000000000000000000000000")
copy(key, bz)
simSecp256k1Pubkey.Key = key
}


// ConsumeTxSizeGasDecorator will take in parameters and consume gas proportional
// to the size of tx before calling next AnteHandler. Note, the gas costs will be
// slightly over estimated due to the fact that any given signing account may need
// to be retrieved from state.
//
// CONTRACT: If simulate=true, then signatures must either be completely filled
// in or empty.
// CONTRACT: To use this decorator, signatures of transaction must be represented
// as legacytx.StdSignature otherwise simulate mode will incorrectly estimate gas cost.

// The code was copied from celestia's fork of the cosmos-sdk:
// https://github.com/celestiaorg/cosmos-sdk/blob/release/v0.46.x-celestia/x/auth/ante/basic.go
// In app versions v2 and below, the txSizeCostPerByte used for gas cost estimation is taken from the auth module.
// In app v3 and above, the versioned constant appconsts.TxSizeCostPerByte is used.
type ConsumeTxSizeGasDecorator struct {
ak ante.AccountKeeper
}

func NewConsumeGasForTxSizeDecorator(ak ante.AccountKeeper) ConsumeTxSizeGasDecorator {
return ConsumeTxSizeGasDecorator{
ak: ak,
}
}

func (cgts ConsumeTxSizeGasDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
sigTx, ok := tx.(authsigning.SigVerifiableTx)
if !ok {
return ctx, errors.Wrap(sdkerrors.ErrTxDecode, "invalid tx type")
}
params := cgts.ak.GetParams(ctx)

consumeGasForTxSize(ctx, sdk.Gas(len(ctx.TxBytes())), params)

// simulate gas cost for signatures in simulate mode
if simulate {
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
// in simulate mode, each element should be a nil signature
sigs, err := sigTx.GetSignaturesV2()
if err != nil {
return ctx, err
}
n := len(sigs)

for i, signer := range sigTx.GetSigners() {
// if signature is already filled in, no need to simulate gas cost
if i < n && !isIncompleteSignature(sigs[i].Data) {
continue
}

var pubkey cryptotypes.PubKey

acc := cgts.ak.GetAccount(ctx, signer)

// use placeholder simSecp256k1Pubkey if sig is nil
if acc == nil || acc.GetPubKey() == nil {
pubkey = simSecp256k1Pubkey
} else {
pubkey = acc.GetPubKey()
}

// use stdsignature to mock the size of a full signature
simSig := legacytx.StdSignature{ //nolint:staticcheck // this will be removed when proto is ready
Signature: simSecp256k1Sig[:],
PubKey: pubkey,
}

sigBz := legacy.Cdc.MustMarshal(simSig)
txBytes := sdk.Gas(len(sigBz) + 6)

// If the pubkey is a multi-signature pubkey, then we estimate for the maximum
// number of signers.
if _, ok := pubkey.(*multisig.LegacyAminoPubKey); ok {
txBytes *= params.TxSigLimit
}

consumeGasForTxSize(ctx, txBytes, params)
}
}

return next(ctx, tx, simulate)
}

// isIncompleteSignature tests whether SignatureData is fully filled in for simulation purposes
func isIncompleteSignature(data signing.SignatureData) bool {
if data == nil {
return true
}

switch data := data.(type) {
case *signing.SingleSignatureData:
return len(data.Signature) == 0
case *signing.MultiSignatureData:
if len(data.Signatures) == 0 {
return true
}
for _, s := range data.Signatures {
if isIncompleteSignature(s) {
return true
}
}
}

return false
}

// 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
ctx.GasMeter().ConsumeGas(appconsts.TxSizeCostPerByte(ctx.BlockHeader().Version.App)*txBytes, "txSize")
}
}
184 changes: 184 additions & 0 deletions app/ante/tx_size_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
package ante_test

import (
"fmt"
"strings"
"testing"

"github.com/celestiaorg/celestia-app/v3/app"
"github.com/celestiaorg/celestia-app/v3/app/ante"
"github.com/celestiaorg/celestia-app/v3/pkg/appconsts"
v2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2"
testutil "github.com/celestiaorg/celestia-app/v3/test/util"
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/tx"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
"github.com/cosmos/cosmos-sdk/simapp"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx/signing"
xauthsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/stretchr/testify/require"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
"github.com/tendermint/tendermint/proto/tendermint/version"
)

func setup() (*app.App, sdk.Context, client.Context, error) {
app, _, _ := testutil.NewTestAppWithGenesisSet(app.DefaultConsensusParams())
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: If we're using DefaultConsensusParams that will mean we're using app version 3 right? Has this been updated yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it hasn't been updated yet but I left it as DefaultConsensusParams because we're planning to do so.

ctx := app.NewContext(false, tmproto.Header{})
app.AccountKeeper.SetParams(ctx, authtypes.DefaultParams())
ctx = ctx.WithBlockHeight(1)

// Set up TxConfig.
encodingConfig := simapp.MakeTestEncodingConfig()
// We're using TestMsg encoding in the test, so register it here.
encodingConfig.Amino.RegisterConcrete(&testdata.TestMsg{}, "testdata.TestMsg", nil)
testdata.RegisterInterfaces(encodingConfig.InterfaceRegistry)

clientCtx := client.Context{}.
WithTxConfig(encodingConfig.TxConfig)

return app, ctx, clientCtx, nil
}

func TestConsumeGasForTxSize(t *testing.T) {
app, ctx, clientCtx, err := setup()
require.NoError(t, err)
var txBuilder client.TxBuilder

// keys and addresses
priv1, _, addr1 := testdata.KeyTestPubAddr()

// msg and signatures
msg := testdata.NewTestMsg(addr1)
feeAmount := testdata.NewTestFeeAmount()
gasLimit := testdata.NewTestGasLimit()

cgtsd := ante.NewConsumeGasForTxSizeDecorator(app.AccountKeeper)
antehandler := sdk.ChainAnteDecorators(cgtsd)

testCases := []struct {
version uint64
name string
sigV2 signing.SignatureV2
}{
{v2.Version, "SingleSignatureData v2", signing.SignatureV2{PubKey: priv1.PubKey()}},
{v2.Version, "MultiSignatureData v2", signing.SignatureV2{PubKey: priv1.PubKey(), Data: multisig.NewMultisig(2)}},
{appconsts.LatestVersion, fmt.Sprintf("SingleSignatureData v%d", appconsts.LatestVersion), signing.SignatureV2{PubKey: priv1.PubKey()}},
{appconsts.LatestVersion, fmt.Sprintf("MultiSignatureData v%d", appconsts.LatestVersion), signing.SignatureV2{PubKey: priv1.PubKey(), Data: multisig.NewMultisig(2)}},
Comment on lines +72 to +75
Copy link
Collaborator

Choose a reason for hiding this comment

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

These test cases don't contain an expected amount of gas to be consumed. IMO this test would be more likely to catch bugs if it verified that the amount of gas consumed by v2 differed from v3. Since the params for v2 and v3 are the same, one way to do that would be to change the value of the param for v2 and/or v3 and verify that the gas consumed was based on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
// set the version
ctx = app.NewContext(false, tmproto.Header{Version: version.Consensus{
App: tc.version,
}})

txBuilder = clientCtx.TxConfig.NewTxBuilder()
require.NoError(t, txBuilder.SetMsgs(msg))
txBuilder.SetFeeAmount(feeAmount)
txBuilder.SetGasLimit(gasLimit)
txBuilder.SetMemo(strings.Repeat("01234567890", 10))

privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{0}, []uint64{0}
tx, err := createTestTx(txBuilder, clientCtx, privs, accNums, accSeqs, ctx.ChainID())
require.NoError(t, err)

txBytes, err := clientCtx.TxConfig.TxJSONEncoder()(tx)
require.Nil(t, err, "Cannot marshal tx: %v", err)

expectedGas := sdk.Gas(len(txBytes)) * appconsts.TxSizeCostPerByte(appconsts.LatestVersion)

// set suite.ctx with TxBytes manually
ctx = ctx.WithTxBytes(txBytes)

// track how much gas is necessary to retrieve parameters
beforeGas := ctx.GasMeter().GasConsumed()
app.AccountKeeper.GetParams(ctx)
afterGas := ctx.GasMeter().GasConsumed()
expectedGas += afterGas - beforeGas

beforeGas = ctx.GasMeter().GasConsumed()
ctx, err = antehandler(ctx, tx, false)
require.Nil(t, err, "ConsumeTxSizeGasDecorator returned error: %v", err)

// require that decorator consumes expected amount of gas
consumedGas := ctx.GasMeter().GasConsumed() - beforeGas
require.Equal(t, expectedGas, consumedGas, "Decorator did not consume the correct amount of gas")

// simulation must not underestimate gas of this decorator even with nil signatures
txBuilder, err := clientCtx.TxConfig.WrapTxBuilder(tx)
require.NoError(t, err)
require.NoError(t, txBuilder.SetSignatures(tc.sigV2))
tx = txBuilder.GetTx()

simTxBytes, err := clientCtx.TxConfig.TxJSONEncoder()(tx)
require.Nil(t, err, "Cannot marshal tx: %v", err)
// require that simulated tx is smaller than tx with signatures
require.True(t, len(simTxBytes) < len(txBytes), "simulated tx still has signatures")

// Set suite.ctx with smaller simulated TxBytes manually
ctx = ctx.WithTxBytes(simTxBytes)

beforeSimGas := ctx.GasMeter().GasConsumed()

// run antehandler with simulate=true
ctx, err = antehandler(ctx, tx, true)
consumedSimGas := ctx.GasMeter().GasConsumed() - beforeSimGas

// require that antehandler passes and does not underestimate decorator cost
require.Nil(t, err, "ConsumeTxSizeGasDecorator returned error: %v", err)
require.True(t, consumedSimGas >= expectedGas, "Simulate mode underestimates gas on AnteDecorator. Simulated cost: %d, expected cost: %d", consumedSimGas, expectedGas)
})
}
}

// createTestTx creates a test tx given multiple inputs.
func createTestTx(txBuilder client.TxBuilder, clientCtx client.Context, privs []cryptotypes.PrivKey, accNums []uint64, accSeqs []uint64, chainID string) (xauthsigning.Tx, error) {
// First round: we gather all the signer infos. We use the "set empty
// signature" hack to do that.
sigsV2 := make([]signing.SignatureV2, 0, len(privs))
for i, priv := range privs {
sigV2 := signing.SignatureV2{
PubKey: priv.PubKey(),
Data: &signing.SingleSignatureData{
SignMode: clientCtx.TxConfig.SignModeHandler().DefaultMode(),
Signature: nil,
},
Sequence: accSeqs[i],
}

sigsV2 = append(sigsV2, sigV2)
}
err := txBuilder.SetSignatures(sigsV2...)
if err != nil {
return nil, err
}

// Second round: all signer infos are set, so each signer can sign.
sigsV2 = []signing.SignatureV2{}
for i, priv := range privs {
signerData := xauthsigning.SignerData{
ChainID: chainID,
AccountNumber: accNums[i],
Sequence: accSeqs[i],
}
sigV2, err := tx.SignWithPrivKey(
clientCtx.TxConfig.SignModeHandler().DefaultMode(), signerData,
txBuilder, priv, clientCtx.TxConfig, accSeqs[i])
if err != nil {
return nil, err
}

sigsV2 = append(sigsV2, sigV2)
}
err = txBuilder.SetSignatures(sigsV2...)
if err != nil {
return nil, err
}

return txBuilder.GetTx(), nil
}
Loading
Loading