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 4 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
146 changes: 146 additions & 0 deletions app/ante/tx_size.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
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"
v3 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v3"
"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.
type ConsumeTxSizeGasDecorator struct {
ak ante.AccountKeeper
}
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved

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)
cost := 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 {
cost *= params.TxSigLimit
}

consumeGasForTxSize(ctx, cost, 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, cost uint64, params auth.Params) {
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
// For app v2 and below we should get txSizeCostPerByte from auth module
if ctx.BlockHeader().Version.App <= v2.Version {
ctx.GasMeter().ConsumeGas(params.TxSizeCostPerByte*cost, "txSize")
} else {
// From v3 onwards, we should get txSizeCostPerByte from appconsts
ctx.GasMeter().ConsumeGas(appconsts.TxSizeCostPerByte(v3.Version)*cost, "txSize")
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
}
}
189 changes: 189 additions & 0 deletions app/ante/tx_size_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
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"
v3 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v3"
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"
authante "github.com/cosmos/cosmos-sdk/x/auth/ante"
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"
)

func setup() (*app.App, sdk.Context, client.Context, sdk.AnteHandler, error) {
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
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)

anteHandler, err := authante.NewAnteHandler(
authante.HandlerOptions{
AccountKeeper: app.AccountKeeper,
BankKeeper: app.BankKeeper,
FeegrantKeeper: app.FeeGrantKeeper,
SignModeHandler: encodingConfig.TxConfig.SignModeHandler(),
SigGasConsumer: authante.DefaultSigVerificationGasConsumer,
},
)
if err != nil {
return nil, sdk.Context{}, client.Context{}, nil, fmt.Errorf("error creating AnteHandler: %v", err)
}

return app, ctx, clientCtx, anteHandler, 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 {
name string
sigV2 signing.SignatureV2
}{
{"SingleSignatureData", signing.SignatureV2{PubKey: priv1.PubKey()}},
{"MultiSignatureData", signing.SignatureV2{PubKey: priv1.PubKey(), Data: multisig.NewMultisig(2)}},
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
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(v3.Version)

// 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) {
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
// 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
}
4 changes: 0 additions & 4 deletions pkg/appconsts/initial_consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@ const (
// maximum number of bytes allowed in a valid block.
DefaultMaxBytes = DefaultGovMaxSquareSize * DefaultGovMaxSquareSize * ContinuationSparseShareContentSize

// DefaultGasPerBlobByte is the default gas cost deducted per byte of blob
// included in a PayForBlobs txn
DefaultGasPerBlobByte = 8

Comment on lines -20 to -23
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this is breaking so we should prob add ! to the PR title.

[no change needed] If we want to prep for the v3 release, we could also write a changelog entry on how consumers of this package need to modify their code if they used DefaultGasPerBlobByte. We could write these entires in a new markdown file in a new directory like changelog/v3.md

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/celestiaorg/celestia-app/blob/main/docs/release-notes/pending-release.md now exists so we should add a line that states this constant was removed and what library consumers should use instead.

// DefaultMinGasPrice is the default min gas price that gets set in the app.toml file.
// The min gas price acts as a filter. Transactions below that limit will not pass
// a nodes `CheckTx` and thus not be proposed by that node.
Expand Down
7 changes: 7 additions & 0 deletions pkg/appconsts/v3/app_consts.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package v3

const (
Version uint64 = 3
TxSizeCostPerByte uint64 = 10
GasPerBlobByte uint32 = 8
)
ninabarbakadze marked this conversation as resolved.
Show resolved Hide resolved
11 changes: 11 additions & 0 deletions pkg/appconsts/versioned_consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package appconsts
import (
v1 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v1"
v2 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v2"
v3 "github.com/celestiaorg/celestia-app/v3/pkg/appconsts/v3"
)

const (
Expand All @@ -26,7 +27,17 @@ func SquareSizeUpperBound(_ uint64) int {
return v1.SquareSizeUpperBound
}

func TxSizeCostPerByte(_ uint64) uint64 {
return v3.TxSizeCostPerByte
}

func GasPerBlobByte(_ uint64) uint32 {
return v3.GasPerBlobByte
}

var (
DefaultSubtreeRootThreshold = SubtreeRootThreshold(LatestVersion)
DefaultSquareSizeUpperBound = SquareSizeUpperBound(LatestVersion)
DefaultTxSizeCostPerByte = TxSizeCostPerByte(LatestVersion)
DefaultGasPerBlobByte = GasPerBlobByte(LatestVersion)
rootulp marked this conversation as resolved.
Show resolved Hide resolved
)
Loading
Loading