-
Notifications
You must be signed in to change notification settings - Fork 96
test(systemtests): add appside mempool e2e test #580
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
base: main
Are you sure you want to change the base?
Conversation
mempool/mempool.go
Outdated
if m.HasEventBus() { | ||
if m.eventBus != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we make this change? seems out of scope for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, It seems that when I merge conflict from main branch, something wrong happens.
I'll rollback that changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed! (d089213)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, lovely test setup! I think this will make it significantly easier to write E2E tests in the future, which is something I applaud you for doing.
I left a few comments, mostly regarding the separation of Pending
and Committed
as end-states, as transactions get gossiped differently in the consensus phase (commit) vs. the mempool phase (unconfirmed).
Queued map[string]map[string]*RPCTransaction `json:"queued"` | ||
} | ||
|
||
type RPCTransaction struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this type here. Let's use that instead of copying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this, maybe this PR (#600) should be merged first.
You can see this line (go.mod)
This is temporary fix for resolving Issue #601.
But after temporary fix, when I refer to some cosmos/evm package (to refer to rpctypes.RPCTransaction
), I faced build error with dependency problem like below.
# github.com/cosmos/evm/x/vm/types
../../x/vm/types/opcodes_hooks.go:11:5: undefined: vm.OpCodeHooks
../../x/vm/types/config.go:37:15: undefined: vm.ExtendActivators
../../x/vm/types/logs.go:122:23: log.BlockTimestamp undefined (type *"github.com/ethereum/go-ethereum/core/types".Log has no field or method BlockTimestamp)
FAIL github.com/cosmos/evm/tests/systemtests [build failed]
FAIL github.com/cosmos/evm/tests/systemtests/clients [build failed]
FAIL github.com/cosmos/evm/tests/systemtests/mempool [build failed]
FAIL github.com/cosmos/evm/tests/systemtests/suite [build failed]
Maybe this is because of temporary replacing cosmos/evm package.
So, we need to merge PR #600 and remove replacement of cosmos/evm pacakges.
PrivKey *ethsecp256k1.PrivKey | ||
} | ||
|
||
type TxPoolResult struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, same file as the RPCTransaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto (#580 (comment))
cosmossdk.io/systemtests v1.3.0 | ||
github.com/ethereum/go-ethereum v1.15.5 | ||
github.com/cometbft/cometbft/v2 v2.0.0-rc1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using V2 and RCs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is because cosmossdk.io/systemtests v1.3.0 is dependent with cometbft v2.0.0-rc-1.
Actually, I tried to downgrade systemtests to v1.2.1 that is dependent with cometbft v0.38.17.
But I got issue.
I remember that some flags currently being used do not work.
type TestSuite interface { | ||
// Test Lifecycle | ||
BeforeEach(t *testing.T) | ||
AfterEach(t *testing.T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AfterEach(t *testing.T) | |
AfterEachCase(t *testing.T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied (e566f7a)
// Test Lifecycle | ||
BeforeEach(t *testing.T) | ||
AfterEach(t *testing.T) | ||
JustAfterEach(t *testing.T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JustAfterEach(t *testing.T) | |
AfterEachAction(t *testing.T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied (e566f7a)
func(s TestSuite) { | ||
tx1, err := s.SendCosmosTx(t, s.Node(0), "acc0", 0, s.BaseFeeX2(), big.NewInt(1)) | ||
require.NoError(t, err, "failed to send tx") | ||
_, err = s.SendEthTx(t, s.Node(1), "acc0", 0, s.BaseFee(), nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto (#580 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied (58e97d8)
func(s TestSuite) { | ||
_, err := s.SendEthTx(t, s.Node(0), "acc0", 0, s.BaseFee(), nil) | ||
require.NoError(t, err, "failed to send tx") | ||
tx2, err := s.SendCosmosTx(t, s.Node(1), "acc0", 0, s.BaseFeeX2(), big.NewInt(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto (#580 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied (58e97d8)
func(s TestSuite) { | ||
tx1, err := s.SendEthTx(t, s.Node(0), "acc0", 0, s.BaseFeeX2(), big.NewInt(1)) | ||
require.NoError(t, err, "failed to send tx") | ||
_, err = s.SendCosmosTx(t, s.Node(1), "acc0", 0, s.BaseFee(), nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto (#580 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied (58e97d8)
}, | ||
}, | ||
{ | ||
name: "legacy should always not tx replace dynamic fee tx", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name: "legacy should always not tx replace dynamic fee tx", | |
name: "legacy should never replace dynamic fee tx", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied! (58e97d8)
tests/systemtests/suite/query.go
Outdated
} | ||
|
||
// CheckPendingOrCommitted checks if the given tx is either pending or committed within the timeout duration | ||
func (s *SystemTestSuite) CheckPendingOrCommitted( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do CheckPending
and CheckCommitted
as two separate calls as they function differently in Comet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree!, I'll handle this comment tomorrow!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just removed checking for committed tx from CheckPendingOrCommitted
and made checking operation concurrent logic. (23d361d)
It is because, lets say there are tx1
, tx2
, tx3
, for some test cases, 3 txs are committed in different block.
But for the other cases, tx1 and tx2 can be committed in one block. and tx3 in the other. In this case, if I check pending txs sequentially, we succeed to check tx1 but fail to check tx2 because tx2 can be already committed.
To solve this situation, I modified this verification method as concurrent function.
FYI, we don't have to add CheckCommitted
. Checking commit status is already conducted in AfterEachCase. (ref)
go func(tx *TxInfo) { //nolint:gosec // Concurrency is intentional for parallel tx checking | ||
defer wg.Done() | ||
err := s.CheckTxPending(tx.DstNodeID, tx.TxHash, tx.TxType, time.Second*30) | ||
if err != nil { | ||
mu.Lock() | ||
errors = append(errors, fmt.Errorf("tx %s is not pending or committed: %v", tx.TxHash, err)) | ||
mu.Unlock() | ||
} | ||
}(txInfo) |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note test
Description
Summary
This PR implements comprehensive end-to-end testing for appside mempool functionality, adding robust test infrastructure and extensive test cases to verify mempool behavior across different transaction types and node configurations.
Closes #581
System Tests Directory Structure
Test Mechanism
2.1 Persistent Node Architecture
2.2 Common Verification Framework
verification steps
(or committed)
on target nodes only
pending transactions to commit
Test Coverage Checklist
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
main
branch