From 25cb17524414b0a325297df4b2b9bc74e06109ac Mon Sep 17 00:00:00 2001 From: Nguyen Nhu Viet Date: Wed, 9 Aug 2023 12:25:49 +0200 Subject: [PATCH 1/6] fix: packageNaming fix for txsim (#2234) ## Overview This fix is intended to fix the creation of the package for txsim ## Checklist - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords --- .github/workflows/docker-build-publish.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/docker-build-publish.yml b/.github/workflows/docker-build-publish.yml index 1c0abb9d18..5d5e63b623 100644 --- a/.github/workflows/docker-build-publish.yml +++ b/.github/workflows/docker-build-publish.yml @@ -28,3 +28,4 @@ jobs: uses: celestiaorg/.github/.github/workflows/reusable_dockerfile_pipeline.yml@v0.2.2 with: dockerfile: docker/Dockerfile_txsim + packageName: txsim From 1725857297d92df3f43465fec1ae3d49504b8489 Mon Sep 17 00:00:00 2001 From: Biswarghya Biswas Date: Wed, 9 Aug 2023 18:24:52 +0530 Subject: [PATCH 2/6] fix: incorrect byte units in comments (#2230) ## Overview closes #2228 ## Checklist - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords --- app/test/integration_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/test/integration_test.go b/app/test/integration_test.go index 585c1b3347..fe340dc4f5 100644 --- a/app/test/integration_test.go +++ b/app/test/integration_test.go @@ -93,15 +93,15 @@ func (s *IntegrationTestSuite) TestMaxBlockSize() { } // Tendermint's default tx size limit is 1 MiB, so we get close to that by - // generating transactions of size 600 KiB because 3 blobs per transaction * - // 200,000 bytes each = 600,000 total bytes = 600 KiB per transaction. + // generating transactions of size 600 KB because 3 blobs per transaction * + // 200,000 bytes each = 600,000 total bytes = 600 KB per transaction. randMultiBlob1MbTxGen := func(c client.Context) []coretypes.Tx { return blobfactory.RandBlobTxsWithAccounts( s.ecfg.TxConfig.TxEncoder(), tmrand.NewRand(), s.cctx.Keyring, c.GRPCClient, - 200000, // 200 KiB + 200000, // 200 KB 3, false, s.cctx.ChainID, @@ -109,7 +109,7 @@ func (s *IntegrationTestSuite) TestMaxBlockSize() { ) } - // Generate 80 randomly sized txs (max size == 50 KiB). Generate these + // Generate 80 randomly sized txs (max size == 50 KB). Generate these // transactions using some of the same accounts as the previous generator to // ensure that the sequence number is being utilized correctly in blob // txs From 29f7f509e9109b46dc7cfd40b9c68de68b4e0fdc Mon Sep 17 00:00:00 2001 From: Sanaz Taheri <35961250+staheri14@users.noreply.github.com> Date: Wed, 9 Aug 2023 15:59:21 +0300 Subject: [PATCH 3/6] test: tests tx inclusion in the first block (#2220) ## Overview As part of #1899, we need to test the network/validator's behavior for transaction inclusion in the first block (more precisely, when the first block has not been committed yet). This PR introduces a new test that utilizes a local consensus config option, namely `timeout_commit`, to slow down block production. This delay allows transactions to arrive at the time of block height 1. It is important to note that the current test demonstrates that transaction inclusion fails due to the absence of commit info for block height 0. There are two theories regarding this behavior: 1) Theory 1: This is expected behavior as the initial state or genesis state might not be available by the protocol definition. Consequently, the first block remains empty because there is no valid state available to validate transactions against. (Our fork of Cosmos SDK is not in parity with the mainline Cosmos SDK, however, with the initial inspection, it seems the initial state does not get committed in the mainline Cosmos SDK too, hence it is suspected that the same problem exists there as well. However, further tests should be developed to confirm this) 2) Theory 2: This is not an expected behavior. The genesis state should be committed when initializing the chain (in `abci.InitChain`). In this case, the root cause should be found and the fix should be implemented. In either case, subsequent PRs will be addressing this issue. ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords --- app/test/block_production_test.go | 57 +++++++++++++++++++++++++++++++ test/util/testnode/config.go | 6 ++++ 2 files changed, 63 insertions(+) create mode 100644 app/test/block_production_test.go diff --git a/app/test/block_production_test.go b/app/test/block_production_test.go new file mode 100644 index 0000000000..cc50f41951 --- /dev/null +++ b/app/test/block_production_test.go @@ -0,0 +1,57 @@ +package app + +import ( + "testing" + "time" + + appns "github.com/celestiaorg/celestia-app/pkg/namespace" + "github.com/celestiaorg/celestia-app/test/util/testnode" + "github.com/cosmos/cosmos-sdk/client/flags" + "github.com/stretchr/testify/suite" + tmrand "github.com/tendermint/tendermint/libs/rand" +) + +func TestBlockProductionTestSuite(t *testing.T) { + if testing.Short() { + t.Skip("skipping block production test suite in short mode.") + } + suite.Run(t, new(BlockProductionTestSuite)) +} + +type BlockProductionTestSuite struct { + suite.Suite + + accounts []string + cctx testnode.Context +} + +func (s *BlockProductionTestSuite) SetupSuite() { + t := s.T() + + accounts := make([]string, 40) + for i := 0; i < 40; i++ { + accounts[i] = tmrand.Str(10) + } + + cfg := testnode.DefaultConfig(). + WithAccounts(accounts). + WithTimeoutCommit(10 * time.Second) + + cctx, _, _ := testnode.NewNetwork(t, cfg) + s.cctx = cctx + s.accounts = accounts +} + +// Test_BlockOneTransactionNonInclusion tests that no transactions can be included in the first block. +func (s *BlockProductionTestSuite) Test_BlockOneTransactionNonInclusion() { + require := s.Require() + _, err := s.cctx.PostData(s.accounts[0], flags.BroadcastBlock, appns.RandomBlobNamespace(), tmrand.Bytes(100000)) + + // since the block production is delayed by 10 seconds, the transactions + // posted arrive when the node is still at height 0 (not started height 1 + // yet) this makes the post data fail with the following error: rpc error: + // code = Unknown desc = codespace sdk code 18: invalid request: + // failed to load state at height 0; no commit info found (latest height: 0) + require.Error(err) // change this to require.NoError(err) to see the error + require.ErrorContains(err, "rpc error: code = Unknown desc = codespace sdk code 18: invalid request: failed to load state at height 0; no commit info found (latest height: 0)") +} diff --git a/test/util/testnode/config.go b/test/util/testnode/config.go index cca78ac0e8..b6453e804c 100644 --- a/test/util/testnode/config.go +++ b/test/util/testnode/config.go @@ -99,6 +99,12 @@ func (c *Config) WithGensisTime(t time.Time) *Config { return c } +// WithTimeoutCommit sets the CommitTimeout and returns the Config. +func (c *Config) WithTimeoutCommit(d time.Duration) *Config { + c.TmConfig.Consensus.TimeoutCommit = d + return c +} + func DefaultConfig() *Config { tmcfg := DefaultTendermintConfig() tmcfg.Consensus.TimeoutCommit = 1 * time.Millisecond From 9344a055be74170197977d124dee0cf5756c927b Mon Sep 17 00:00:00 2001 From: Matthew Sevey Date: Wed, 9 Aug 2023 10:30:29 -0400 Subject: [PATCH 4/6] Update mergify.yml to fix label (#2236) I think this is the bug in mergify --- .github/mergify.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/mergify.yml b/.github/mergify.yml index 336a45f9b7..c78c248ae8 100644 --- a/.github/mergify.yml +++ b/.github/mergify.yml @@ -2,7 +2,7 @@ pull_request_rules: - name: backport patches to v1.x branch conditions: - base=main - - label=S:backport:v1.x + - label=backport:v1.x actions: backport: branches: From 58cc2790ab18acf941869375c5ba969952039b1a Mon Sep 17 00:00:00 2001 From: Callum Waters Date: Wed, 9 Aug 2023 17:25:48 +0200 Subject: [PATCH 5/6] feat: add handling functions for nonce mismatches (#2235) --- .../{errors.go => insufficient_gas_price.go} | 0 ...test.go => insufficient_gas_price_test.go} | 0 app/errors/nonce_mismatch.go | 30 ++++++++++ app/errors/nonce_mismatch_test.go | 56 +++++++++++++++++++ 4 files changed, 86 insertions(+) rename app/errors/{errors.go => insufficient_gas_price.go} (100%) rename app/errors/{errors_test.go => insufficient_gas_price_test.go} (100%) create mode 100644 app/errors/nonce_mismatch.go create mode 100644 app/errors/nonce_mismatch_test.go diff --git a/app/errors/errors.go b/app/errors/insufficient_gas_price.go similarity index 100% rename from app/errors/errors.go rename to app/errors/insufficient_gas_price.go diff --git a/app/errors/errors_test.go b/app/errors/insufficient_gas_price_test.go similarity index 100% rename from app/errors/errors_test.go rename to app/errors/insufficient_gas_price_test.go diff --git a/app/errors/nonce_mismatch.go b/app/errors/nonce_mismatch.go new file mode 100644 index 0000000000..2726d61060 --- /dev/null +++ b/app/errors/nonce_mismatch.go @@ -0,0 +1,30 @@ +package errors + +import ( + "errors" + "fmt" + "strconv" + + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +// IsNonceMismatch checks if the error is due to a sequence mismatch. +func IsNonceMismatch(err error) bool { + return errors.Is(err, sdkerrors.ErrWrongSequence) +} + +// ParseNonceMismatch extracts the expected sequence number from the +// ErrWrongSequence error. +func ParseNonceMismatch(err error) (uint64, error) { + if !IsNonceMismatch(err) { + return 0, errors.New("error is not a sequence mismatch") + } + + numbers := regexpInt.FindAllString(err.Error(), -1) + if len(numbers) != 2 { + return 0, fmt.Errorf("unexpected wrong sequence error: %w", err) + } + + // the first number is the expected sequence number + return strconv.ParseUint(numbers[0], 10, 64) +} diff --git a/app/errors/nonce_mismatch_test.go b/app/errors/nonce_mismatch_test.go new file mode 100644 index 0000000000..b6509df3a0 --- /dev/null +++ b/app/errors/nonce_mismatch_test.go @@ -0,0 +1,56 @@ +package errors_test + +import ( + "fmt" + "testing" + + "github.com/celestiaorg/celestia-app/app" + "github.com/celestiaorg/celestia-app/app/encoding" + apperr "github.com/celestiaorg/celestia-app/app/errors" + "github.com/celestiaorg/celestia-app/pkg/appconsts" + "github.com/celestiaorg/celestia-app/pkg/namespace" + testutil "github.com/celestiaorg/celestia-app/test/util" + blob "github.com/celestiaorg/celestia-app/x/blob/types" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/x/auth/ante" + "github.com/stretchr/testify/require" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" +) + +// This will detect any changes to the DeductFeeDecorator which may cause a +// different error message that does not match the regexp. +func TestNonceMismatchIntegration(t *testing.T) { + account := "test" + testApp, kr := testutil.SetupTestAppWithGenesisValSet(app.DefaultConsensusParams(), account) + encCfg := encoding.MakeConfig(app.ModuleEncodingRegisters...) + minGasPrice, err := sdk.ParseDecCoins(fmt.Sprintf("%v%s", appconsts.DefaultMinGasPrice, app.BondDenom)) + require.NoError(t, err) + ctx := testApp.NewContext(true, tmproto.Header{}).WithMinGasPrices(minGasPrice) + signer := blob.NewKeyringSigner(kr, account, testutil.ChainID) + // set the sequence to an incorrect value + signer.SetSequence(2) + builder := signer.NewTxBuilder() + + address, err := signer.GetSignerInfo().GetAddress() + require.NoError(t, err) + + b, err := blob.NewBlob(namespace.RandomNamespace(), []byte("hello world"), 0) + require.NoError(t, err) + + pfb, err := blob.NewMsgPayForBlobs(address.String(), b) + require.NoError(t, err, address) + + tx, err := signer.BuildSignedTx(builder, pfb) + require.NoError(t, err) + + decorator := ante.NewSigVerificationDecorator(testApp.AccountKeeper, encCfg.TxConfig.SignModeHandler()) + anteHandler := sdk.ChainAnteDecorators(decorator) + + // We set simulate to true here to bypass having to initialize the + // accounts public key. + _, err = anteHandler(ctx, tx, true) + require.True(t, apperr.IsNonceMismatch(err), err) + expectedNonce, err := apperr.ParseNonceMismatch(err) + require.NoError(t, err) + require.EqualValues(t, 0, expectedNonce, err) +} From b70048b275a0e16e0509634c2175c99e2aad7bd1 Mon Sep 17 00:00:00 2001 From: Evan Forbes <42654277+evan-forbes@users.noreply.github.com> Date: Wed, 9 Aug 2023 18:59:56 +0200 Subject: [PATCH 6/6] fix: round up when calculating fee in txsim (#2239) ## Overview this is a quick bug fix that rounds up when calculating fees in txsim. Without doing this, the fees used are too low by a single utia ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords --- test/txsim/account.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/txsim/account.go b/test/txsim/account.go index 2634382fe7..99e63fc78b 100644 --- a/test/txsim/account.go +++ b/test/txsim/account.go @@ -3,6 +3,7 @@ package txsim import ( "context" "fmt" + "math" "strings" "sync" @@ -183,9 +184,9 @@ func (am *AccountManager) Submit(ctx context.Context, op Operation) error { } else { builder.SetGasLimit(op.GasLimit) if op.GasPrice > 0 { - builder.SetFeeAmount(types.NewCoins(types.NewInt64Coin(app.BondDenom, int64(float64(op.GasLimit)*op.GasPrice)))) + builder.SetFeeAmount(types.NewCoins(types.NewInt64Coin(app.BondDenom, int64(math.Ceil(float64(op.GasLimit)*op.GasPrice))))) } else { - builder.SetFeeAmount(types.NewCoins(types.NewInt64Coin(app.BondDenom, int64(float64(op.GasLimit)*appconsts.DefaultMinGasPrice)))) + builder.SetFeeAmount(types.NewCoins(types.NewInt64Coin(app.BondDenom, int64(math.Ceil(float64(op.GasLimit)*appconsts.DefaultMinGasPrice))))) } }