From 12d38b8e5e50cba8daafd9732a1050884aa7bada Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Negovanovi=C4=87?= <93934272+Stefan-Ethernal@users.noreply.github.com> Date: Mon, 2 Oct 2023 11:24:13 +0200 Subject: [PATCH] Provide race and shuffle flags when running unit tests (#1925) * Provide race and shuffle flags * Fix race condition in TestBlockchain_VerifyBlockParent test * Fix race conditions in Test_TxPool_validateTx * Capture loop variable * Fix TestAddTxsInOrder * Linter fixes * Fix TestBlockchain_VerifyBlockBody race conditions * Fix Test_VerifySignature_NegativeCases * Fix TestDropKnownGossipTx * Retrieve tx hash in a thread safe manner * Fix TestStatusPubSub race condition * Fix peers_test * Fix Test_newTracer * Provide race flag when building binary and running fuzz tests * Fix data races in fund and deposit_erc20 commands * Fix TestEventSubscription_ProcessedEvents * Fix TestSimpleGossip (hopefully) * Fix data race in e2e tests (txRelayer.SendTransactionLocal) * Avoid race conditions in ibft/signer package * Increase timeout in TestEventSubscription_ProcessedEvents * Fix TestResetAccounts_Enqueued flakiness * Remove race flags from e2e and property tests * Update command/bridge/deposit/erc20/deposit_erc20.go Co-authored-by: Victor Castell * Fix transaction.Copy function --------- Co-authored-by: Victor Castell --- Makefile | 4 +- blockchain/blockchain.go | 4 +- blockchain/blockchain_test.go | 21 +++++----- chain/params.go | 23 +++++++++++ command/bridge/deposit/erc20/deposit_erc20.go | 2 + command/rootchain/fund/fund.go | 5 ++- consensus/ibft/signer/extra_test.go | 20 --------- consensus/ibft/signer/signer_test.go | 4 -- consensus/polybft/block_builder.go | 4 +- consensus/polybft/fsm.go | 10 +++-- consensus/polybft/polybft.go | 8 ++-- consensus/polybft/signer/signature_test.go | 22 ++++++---- forkmanager/fork.go | 17 ++++++++ gasprice/gasprice.go | 2 +- jsonrpc/debug_endpoint.go | 2 +- jsonrpc/filter_manager.go | 4 +- jsonrpc/types.go | 4 +- network/e2e_testing.go | 10 ++--- network/gossip_test.go | 40 +++++++----------- scripts/fuzzAll | 2 +- state/runtime/tracer/structtracer/tracer.go | 6 +++ syncer/client_test.go | 3 +- syncer/peers_test.go | 26 ++++++------ txpool/event_subscription_test.go | 2 +- txpool/operator.go | 2 +- txpool/queue_account.go | 8 ++++ txpool/txpool.go | 21 +++++----- txpool/txpool_test.go | 41 ++++++++++--------- txrelayer/txrelayer.go | 25 +++++++---- types/transaction.go | 34 ++++++++++++++- types/transaction_fork_hash.go | 8 +++- 31 files changed, 234 insertions(+), 150 deletions(-) diff --git a/Makefile b/Makefile index 71e00bc10b..d90e876939 100644 --- a/Makefile +++ b/Makefile @@ -42,7 +42,7 @@ build: check-go check-git $(eval COMMIT_HASH = $(shell git rev-parse HEAD)) $(eval BRANCH = $(shell git rev-parse --abbrev-ref HEAD | tr -d '\040\011\012\015\n')) $(eval TIME = $(shell date)) - go build -o polygon-edge -ldflags="\ + go build -race -o polygon-edge -ldflags="\ -X 'github.com/0xPolygon/polygon-edge/versioning.Version=$(LATEST_VERSION)' \ -X 'github.com/0xPolygon/polygon-edge/versioning.Commit=$(COMMIT_HASH)'\ -X 'github.com/0xPolygon/polygon-edge/versioning.Branch=$(BRANCH)'\ @@ -59,7 +59,7 @@ generate-bsd-licenses: check-git .PHONY: test test: check-go - go test -coverprofile coverage.out -timeout 20m `go list ./... | grep -v e2e` + go test -race -shuffle=on -coverprofile coverage.out -timeout 20m `go list ./... | grep -v e2e` .PHONY: fuzz-test fuzz-test: check-go diff --git a/blockchain/blockchain.go b/blockchain/blockchain.go index 13acc0de75..ad36b3584d 100644 --- a/blockchain/blockchain.go +++ b/blockchain/blockchain.go @@ -1002,7 +1002,7 @@ func (b *Blockchain) writeBody(batchWriter *storage.BatchWriter, block *types.Bl // Write txn lookups (txHash -> block) for _, txn := range block.Transactions { - batchWriter.PutTxLookup(txn.Hash, block.Hash()) + batchWriter.PutTxLookup(txn.GetHash(), block.Hash()) } return nil @@ -1046,7 +1046,7 @@ func (b *Blockchain) recoverFromFieldsInTransactions(transactions []*types.Trans sender, err := b.txSigner.Sender(tx) if err != nil { - b.logger.Warn("failed to recover from address in Tx", "hash", tx.Hash, "err", err) + b.logger.Warn("failed to recover from address in Tx", "hash", tx.GetHash(), "err", err) continue } diff --git a/blockchain/blockchain_test.go b/blockchain/blockchain_test.go index 33b624d9cb..5157c2b9cc 100644 --- a/blockchain/blockchain_test.go +++ b/blockchain/blockchain_test.go @@ -1096,7 +1096,7 @@ func TestBlockchain_VerifyBlockParent(t *testing.T) { // Set up the storage callback storageCallback := func(storage *storage.MockStorage) { storage.HookReadHeader(func(hash types.Hash) (*types.Header, error) { - return emptyHeader, nil + return emptyHeader.Copy(), nil }) } @@ -1110,7 +1110,7 @@ func TestBlockchain_VerifyBlockParent(t *testing.T) { // Create a dummy block whose parent hash will // not match the computed parent hash block := &types.Block{ - Header: emptyHeader, + Header: emptyHeader.Copy(), } assert.ErrorIs(t, blockchain.verifyBlockParent(block), ErrParentHashMismatch) @@ -1122,7 +1122,7 @@ func TestBlockchain_VerifyBlockParent(t *testing.T) { // Set up the storage callback storageCallback := func(storage *storage.MockStorage) { storage.HookReadHeader(func(hash types.Hash) (*types.Header, error) { - return emptyHeader, nil + return emptyHeader.Copy(), nil }) } @@ -1149,7 +1149,7 @@ func TestBlockchain_VerifyBlockParent(t *testing.T) { // Set up the storage callback storageCallback := func(storage *storage.MockStorage) { storage.HookReadHeader(func(hash types.Hash) (*types.Header, error) { - return emptyHeader, nil + return emptyHeader.Copy(), nil }) } @@ -1164,7 +1164,7 @@ func TestBlockchain_VerifyBlockParent(t *testing.T) { block := &types.Block{ Header: &types.Header{ Number: 10, - ParentHash: emptyHeader.Hash, + ParentHash: emptyHeader.Copy().Hash, }, } @@ -1180,7 +1180,7 @@ func TestBlockchain_VerifyBlockParent(t *testing.T) { // Set up the storage callback storageCallback := func(storage *storage.MockStorage) { storage.HookReadHeader(func(hash types.Hash) (*types.Header, error) { - return emptyHeader, nil + return emptyHeader.Copy(), nil }) } @@ -1286,7 +1286,7 @@ func TestBlockchain_VerifyBlockBody(t *testing.T) { storageCallback := func(storage *storage.MockStorage) { // This is used for parent fetching storage.HookReadHeader(func(hash types.Hash) (*types.Header, error) { - return emptyHeader, nil + return emptyHeader.Copy(), nil }) } @@ -1326,7 +1326,7 @@ func TestBlockchain_VerifyBlockBody(t *testing.T) { storageCallback := func(storage *storage.MockStorage) { // This is used for parent fetching storage.HookReadHeader(func(hash types.Hash) (*types.Header, error) { - return emptyHeader, nil + return emptyHeader.Copy(), nil }) } @@ -1385,12 +1385,13 @@ func TestBlockchain_CalculateBaseFee(t *testing.T) { } for i, test := range tests { + i := i test := test - t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + t.Run(fmt.Sprintf("test case #%d", i+1), func(t *testing.T) { t.Parallel() - blockchain := Blockchain{ + blockchain := &Blockchain{ config: &chain.Chain{ Params: &chain.Params{ Forks: &chain.Forks{ diff --git a/chain/params.go b/chain/params.go index 199dc5d9d5..23666b8df6 100644 --- a/chain/params.go +++ b/chain/params.go @@ -132,6 +132,16 @@ func (f *Forks) At(block uint64) ForksInTime { } } +// Copy creates a deep copy of Forks map +func (f Forks) Copy() *Forks { + copiedForks := make(Forks, len(f)) + for key, value := range f { + copiedForks[key] = value.Copy() + } + + return &copiedForks +} + type Fork struct { Block uint64 `json:"block"` Params *forkmanager.ForkParams `json:"params,omitempty"` @@ -145,6 +155,19 @@ func (f Fork) Active(block uint64) bool { return block >= f.Block } +// Copy creates a deep copy of Fork +func (f Fork) Copy() Fork { + var fp *forkmanager.ForkParams + if f.Params != nil { + fp = f.Params.Copy() + } + + return Fork{ + Block: f.Block, + Params: fp, + } +} + // ForksInTime should contain all supported forks by current edge version type ForksInTime struct { Homestead, diff --git a/command/bridge/deposit/erc20/deposit_erc20.go b/command/bridge/deposit/erc20/deposit_erc20.go index 95c8258236..6ff0f6225a 100644 --- a/command/bridge/deposit/erc20/deposit_erc20.go +++ b/command/bridge/deposit/erc20/deposit_erc20.go @@ -195,6 +195,8 @@ func runCommand(cmd *cobra.Command, _ []string) { return fmt.Errorf("failed to create tx input: %w", err) } + var receipt *ethgo.Receipt + receipt, err = txRelayer.SendTransaction(depositTxn, depositorKey) if err != nil { return fmt.Errorf("receiver: %s, amount: %s, error: %w", receiver, amount, err) diff --git a/command/rootchain/fund/fund.go b/command/rootchain/fund/fund.go index c1a2b9b3aa..03238494d3 100644 --- a/command/rootchain/fund/fund.go +++ b/command/rootchain/fund/fund.go @@ -120,7 +120,10 @@ func runCommand(cmd *cobra.Command, _ []string) { fundAddr := ethgo.Address(validatorAddr) txn := helper.CreateTransaction(ethgo.ZeroAddress, &fundAddr, nil, params.amountValues[i], true) - var receipt *ethgo.Receipt + var ( + receipt *ethgo.Receipt + err error + ) if params.deployerPrivateKey != "" { receipt, err = txRelayer.SendTransaction(txn, deployerKey) diff --git a/consensus/ibft/signer/extra_test.go b/consensus/ibft/signer/extra_test.go index 49b11bc9da..55622d38e8 100644 --- a/consensus/ibft/signer/extra_test.go +++ b/consensus/ibft/signer/extra_test.go @@ -21,8 +21,6 @@ func JSONMarshalHelper(t *testing.T, extra *IstanbulExtra) string { } func TestIstanbulExtraMarshalAndUnmarshal(t *testing.T) { - t.Parallel() - tests := []struct { name string extra *IstanbulExtra @@ -99,8 +97,6 @@ func TestIstanbulExtraMarshalAndUnmarshal(t *testing.T) { test := test t.Run(test.name, func(t *testing.T) { - t.Parallel() - // create original data originalExtraJSON := JSONMarshalHelper(t, test.extra) @@ -119,8 +115,6 @@ func TestIstanbulExtraMarshalAndUnmarshal(t *testing.T) { } func Test_packProposerSealIntoExtra(t *testing.T) { - t.Parallel() - newProposerSeal := []byte("new proposer seal") tests := []struct { @@ -197,8 +191,6 @@ func Test_packProposerSealIntoExtra(t *testing.T) { test := test t.Run(test.name, func(t *testing.T) { - t.Parallel() - originalProposerSeal := test.extra.ProposerSeal // create expected data @@ -233,8 +225,6 @@ func Test_packProposerSealIntoExtra(t *testing.T) { } func Test_packCommittedSealsAndRoundNumberIntoExtra(t *testing.T) { - t.Parallel() - tests := []struct { name string extra *IstanbulExtra @@ -331,8 +321,6 @@ func Test_packCommittedSealsAndRoundNumberIntoExtra(t *testing.T) { test := test t.Run(test.name, func(t *testing.T) { - t.Parallel() - originalCommittedSeals := test.extra.CommittedSeals // create expected data @@ -370,8 +358,6 @@ func Test_packCommittedSealsAndRoundNumberIntoExtra(t *testing.T) { } func Test_unmarshalRLPForParentCS(t *testing.T) { - t.Parallel() - tests := []struct { name string extra *IstanbulExtra @@ -423,8 +409,6 @@ func Test_unmarshalRLPForParentCS(t *testing.T) { test := test t.Run(test.name, func(t *testing.T) { - t.Parallel() - bytesData := test.extra.MarshalRLPTo(nil) assert.NoError(t, test.targetExtra.unmarshalRLPForParentCS(bytesData)) @@ -440,8 +424,6 @@ func Test_unmarshalRLPForParentCS(t *testing.T) { } func Test_putIbftExtra(t *testing.T) { - t.Parallel() - tests := []struct { name string header *types.Header @@ -493,8 +475,6 @@ func Test_putIbftExtra(t *testing.T) { test := test t.Run(test.name, func(t *testing.T) { - t.Parallel() - putIbftExtra(test.header, test.extra) expectedExtraHeader := make([]byte, IstanbulExtraVanity) diff --git a/consensus/ibft/signer/signer_test.go b/consensus/ibft/signer/signer_test.go index c85ad29164..36d89bba3d 100644 --- a/consensus/ibft/signer/signer_test.go +++ b/consensus/ibft/signer/signer_test.go @@ -737,8 +737,6 @@ func TestVerifyCommittedSeal(t *testing.T) { } func TestSignerWriteCommittedSeals(t *testing.T) { - t.Parallel() - var round0 uint64 = 0 tests := []struct { @@ -849,8 +847,6 @@ func TestSignerWriteCommittedSeals(t *testing.T) { test := test t.Run(test.name, func(t *testing.T) { - t.Parallel() - signer := newTestSingleKeyManagerSigner(test.keyManager) header, err := signer.WriteCommittedSeals(test.header, test.roundNumber, test.sealMap) diff --git a/consensus/polybft/block_builder.go b/consensus/polybft/block_builder.go index 67e2e7d54d..5058654995 100644 --- a/consensus/polybft/block_builder.go +++ b/consensus/polybft/block_builder.go @@ -140,7 +140,7 @@ func (b *BlockBuilder) Build(handler func(h *types.Header)) (*types.FullBlock, e // WriteTx applies given transaction to the state. If transaction apply fails, it reverts the saved snapshot. func (b *BlockBuilder) WriteTx(tx *types.Transaction) error { if tx.Gas > b.params.GasLimit { - b.params.Logger.Info("Transaction gas limit exceedes block gas limit", "hash", tx.Hash, + b.params.Logger.Info("Transaction gas limit exceedes block gas limit", "hash", tx.GetHash(), "tx gas limit", tx.Gas, "block gas limt", b.params.GasLimit) return txpool.ErrBlockLimitExceeded @@ -171,7 +171,7 @@ write: // execute transactions one by one finished, err := b.writeTxPoolTransaction(tx) if err != nil { - b.params.Logger.Debug("Fill transaction error", "hash", tx.Hash, "err", err) + b.params.Logger.Debug("Fill transaction error", "hash", tx.GetHash(), "err", err) } if finished { diff --git a/consensus/polybft/fsm.go b/consensus/polybft/fsm.go index 9a6176a68c..56d35f4f17 100644 --- a/consensus/polybft/fsm.go +++ b/consensus/polybft/fsm.go @@ -426,24 +426,26 @@ func (f *fsm) VerifyStateTransactions(transactions []*types.Transaction) error { continue } + txHash := tx.GetHash() + decodedStateTx, err := decodeStateTransaction(tx.Input) if err != nil { - return fmt.Errorf("unknown state transaction: tx = %v, err = %w", tx.Hash, err) + return fmt.Errorf("unknown state transaction: tx = %v, err = %w", txHash, err) } switch stateTxData := decodedStateTx.(type) { case *CommitmentMessageSigned: if !f.isEndOfSprint { - return fmt.Errorf("found commitment tx in block which should not contain it (tx hash=%s)", tx.Hash) + return fmt.Errorf("found commitment tx in block which should not contain it (tx hash=%s)", txHash) } if commitmentTxExists { - return fmt.Errorf("only one commitment tx is allowed per block (tx hash=%s)", tx.Hash) + return fmt.Errorf("only one commitment tx is allowed per block (tx hash=%s)", txHash) } commitmentTxExists = true - if err = verifyBridgeCommitmentTx(f.Height(), tx.Hash, stateTxData, f.validators); err != nil { + if err = verifyBridgeCommitmentTx(f.Height(), txHash, stateTxData, f.validators); err != nil { return err } case *contractsapi.CommitEpochValidatorSetFn: diff --git a/consensus/polybft/polybft.go b/consensus/polybft/polybft.go index e8aaac59d4..603bf5a336 100644 --- a/consensus/polybft/polybft.go +++ b/consensus/polybft/polybft.go @@ -749,21 +749,23 @@ func (p *Polybft) PreCommitState(block *types.Block, _ *state.Transition) error continue } + txHash := tx.GetHash() + decodedStateTx, err := decodeStateTransaction(tx.Input) if err != nil { - return fmt.Errorf("unknown state transaction: tx=%v, error: %w", tx.Hash, err) + return fmt.Errorf("unknown state transaction: tx=%v, error: %w", txHash, err) } if signedCommitment, ok := decodedStateTx.(*CommitmentMessageSigned); ok { if commitmentTxExists { - return fmt.Errorf("only one commitment state tx is allowed per block: %v", tx.Hash) + return fmt.Errorf("only one commitment state tx is allowed per block: %v", txHash) } commitmentTxExists = true if err := verifyBridgeCommitmentTx( block.Number(), - tx.Hash, + txHash, signedCommitment, validator.NewValidatorSet(validators, p.logger)); err != nil { return err diff --git a/consensus/polybft/signer/signature_test.go b/consensus/polybft/signer/signature_test.go index 3de35c682a..ef2b62831e 100644 --- a/consensus/polybft/signer/signature_test.go +++ b/consensus/polybft/signer/signature_test.go @@ -47,20 +47,26 @@ func Test_VerifySignature_NegativeCases(t *testing.T) { require.True(t, signature.Verify(blsKey.PublicKey(), validTestMsg, DomainValidatorSet)) + rawSig, err := signature.Marshal() + require.NoError(t, err) + t.Run("Wrong public key", func(t *testing.T) { t.Parallel() + sigTemp, err := UnmarshalSignature(rawSig) + require.NoError(t, err) + for i := 0; i < 100; i++ { x, randomG2, err := bn256.RandomG2(rand.Reader) require.NoError(t, err) publicKey := blsKey.PublicKey() publicKey.g2.Add(publicKey.g2, randomG2) // change public key g2 point - require.False(t, signature.Verify(publicKey, validTestMsg, DomainValidatorSet)) + require.False(t, sigTemp.Verify(publicKey, validTestMsg, DomainValidatorSet)) publicKey = blsKey.PublicKey() publicKey.g2.ScalarMult(publicKey.g2, x) // change public key g2 point - require.False(t, signature.Verify(publicKey, validTestMsg, DomainValidatorSet)) + require.False(t, sigTemp.Verify(publicKey, validTestMsg, DomainValidatorSet)) } }) @@ -70,11 +76,14 @@ func Test_VerifySignature_NegativeCases(t *testing.T) { msgCopy := make([]byte, len(validTestMsg)) copy(msgCopy, validTestMsg) + sigTemp, err := UnmarshalSignature(rawSig) + require.NoError(t, err) + for i := 0; i < len(msgCopy); i++ { b := msgCopy[i] msgCopy[i] = b + 1 - require.False(t, signature.Verify(blsKey.PublicKey(), msgCopy, DomainValidatorSet)) + require.False(t, sigTemp.Verify(blsKey.PublicKey(), msgCopy, DomainValidatorSet)) msgCopy[i] = b } }) @@ -86,16 +95,13 @@ func Test_VerifySignature_NegativeCases(t *testing.T) { x, randomG1, err := bn256.RandomG1(rand.Reader) require.NoError(t, err) - raw, err := signature.Marshal() - require.NoError(t, err) - - sigCopy, err := UnmarshalSignature(raw) + sigCopy, err := UnmarshalSignature(rawSig) require.NoError(t, err) sigCopy.g1.Add(sigCopy.g1, randomG1) // change signature require.False(t, sigCopy.Verify(blsKey.PublicKey(), validTestMsg, DomainValidatorSet)) - sigCopy, err = UnmarshalSignature(raw) + sigCopy, err = UnmarshalSignature(rawSig) require.NoError(t, err) sigCopy.g1.ScalarMult(sigCopy.g1, x) // change signature diff --git a/forkmanager/fork.go b/forkmanager/fork.go index b55d40e882..3e95a8a7cd 100644 --- a/forkmanager/fork.go +++ b/forkmanager/fork.go @@ -40,6 +40,23 @@ type ForkParams struct { BlockTimeDrift *uint64 `json:"blockTimeDrift,omitempty"` } +// Copy creates a deep copy of ForkParams +func (fp *ForkParams) Copy() *ForkParams { + maxValSetSize := *fp.MaxValidatorSetSize + epochSize := *fp.EpochSize + sprintSize := *fp.SprintSize + blockTime := *fp.BlockTime + blockTimeDrift := *fp.BlockTimeDrift + + return &ForkParams{ + MaxValidatorSetSize: &maxValSetSize, + EpochSize: &epochSize, + SprintSize: &sprintSize, + BlockTime: &blockTime, + BlockTimeDrift: &blockTimeDrift, + } +} + // forkHandler defines one custom handler type forkHandler struct { // Handler should be active from block `FromBlockNumber`` diff --git a/gasprice/gasprice.go b/gasprice/gasprice.go index 7c04dfd299..0f9817083c 100644 --- a/gasprice/gasprice.go +++ b/gasprice/gasprice.go @@ -159,7 +159,7 @@ func (g *GasHelper) MaxPriorityFeePerGas() (*big.Int, error) { sender, err := signer.Sender(tx) if err != nil { - return fmt.Errorf("could not get sender of transaction: %s. Error: %w", tx.Hash, err) + return fmt.Errorf("could not get sender of transaction: %s. Error: %w", tx.GetHash(), err) } if sender != blockMiner { diff --git a/jsonrpc/debug_endpoint.go b/jsonrpc/debug_endpoint.go index 577b9a1622..644ed52065 100644 --- a/jsonrpc/debug_endpoint.go +++ b/jsonrpc/debug_endpoint.go @@ -168,7 +168,7 @@ func (d *Debug) TraceTransaction( defer cancel() - return d.store.TraceTxn(block, tx.Hash, tracer) + return d.store.TraceTxn(block, tx.GetHash(), tracer) }, ) } diff --git a/jsonrpc/filter_manager.go b/jsonrpc/filter_manager.go index e618e80d8a..281b3d0c41 100644 --- a/jsonrpc/filter_manager.go +++ b/jsonrpc/filter_manager.go @@ -503,7 +503,7 @@ func (f *FilterManager) getLogsFromBlock(query *LogQuery, block *types.Block) ([ Data: log.Data, BlockNumber: argUint64(block.Header.Number), BlockHash: block.Header.Hash, - TxHash: block.Transactions[idx].Hash, + TxHash: block.Transactions[idx].GetHash(), TxIndex: argUint64(idx), LogIndex: argUint64(logIdx), }) @@ -813,7 +813,7 @@ func (f *FilterManager) appendLogsToFilters(header *block) error { for indx, receipt := range receipts { if receipt.TxHash == types.ZeroHash { // Extract tx Hash - receipt.TxHash = block.Transactions[indx].Hash + receipt.TxHash = block.Transactions[indx].GetHash() } // check the logs with the filters for _, log := range receipt.Logs { diff --git a/jsonrpc/types.go b/jsonrpc/types.go index eab7ee9929..859db2736b 100644 --- a/jsonrpc/types.go +++ b/jsonrpc/types.go @@ -68,7 +68,7 @@ func toTransaction( V: argBig(*t.V), R: argBig(*t.R), S: argBig(*t.S), - Hash: t.Hash, + Hash: t.GetHash(), From: t.From, Type: argUint64(t.Type), BlockNumber: blockNumber, @@ -180,7 +180,7 @@ func toBlock(b *types.Block, fullTx bool) *block { } else { res.Transactions = append( res.Transactions, - transactionHash(txn.Hash), + transactionHash(txn.GetHash()), ) } } diff --git a/network/e2e_testing.go b/network/e2e_testing.go index 91569c8d10..b94edf6b84 100644 --- a/network/e2e_testing.go +++ b/network/e2e_testing.go @@ -340,9 +340,9 @@ func MeshJoin(servers ...*Server) []error { var wg sync.WaitGroup - for indx := 0; indx < numServers; indx++ { - for innerIndx := 0; innerIndx < numServers; innerIndx++ { - if innerIndx > indx { + for sourceIdx := 0; sourceIdx < numServers; sourceIdx++ { + for destIdx := 0; destIdx < numServers; destIdx++ { + if destIdx > sourceIdx { wg.Add(1) go func(src, dest int) { @@ -354,9 +354,9 @@ func MeshJoin(servers ...*Server) []error { DefaultBufferTimeout, DefaultJoinTimeout, ); joinErr != nil { - appendJoinError(fmt.Errorf("unable to join peers, %w", joinErr)) + appendJoinError(fmt.Errorf("unable to join peers %d -> %d, %w", src, dest, joinErr)) } - }(indx, innerIndx) + }(sourceIdx, destIdx) } } } diff --git a/network/gossip_test.go b/network/gossip_test.go index 9655c7762d..114f634c89 100644 --- a/network/gossip_test.go +++ b/network/gossip_test.go @@ -9,6 +9,7 @@ import ( testproto "github.com/0xPolygon/polygon-edge/network/proto" "github.com/libp2p/go-libp2p/core/peer" + "github.com/stretchr/testify/require" ) func NumSubscribers(srv *Server, topic string) int { @@ -30,13 +31,11 @@ func WaitForSubscribers(ctx context.Context, srv *Server, topic string, expected } func TestSimpleGossip(t *testing.T) { - numServers := 10 + numServers := 9 sentMessage := fmt.Sprintf("%d", time.Now().UTC().Unix()) - servers, createErr := createServers(numServers, nil) - if createErr != nil { - t.Fatalf("Unable to create servers, %v", createErr) - } + servers, createErr := createServers(numServers, nil) + require.NoError(t, createErr, "Unable to create servers") messageCh := make(chan *testproto.GenericMessage) @@ -46,32 +45,25 @@ func TestSimpleGossip(t *testing.T) { }) joinErrors := MeshJoin(servers...) - if len(joinErrors) != 0 { - t.Fatalf("Unable to join servers [%d], %v", len(joinErrors), joinErrors) - } + require.Empty(t, joinErrors, "Unable to join servers [%d], %v", len(joinErrors), joinErrors) topicName := "msg-pub-sub" serverTopics := make([]*Topic, numServers) for i := 0; i < numServers; i++ { topic, topicErr := servers[i].NewTopic(topicName, &testproto.GenericMessage{}) - if topicErr != nil { - t.Fatalf("Unable to create topic, %v", topicErr) - } + require.NoError(t, topicErr, "Unable to create topic") serverTopics[i] = topic - if subscribeErr := topic.Subscribe(func(obj interface{}, _ peer.ID) { + subscribeErr := topic.Subscribe(func(obj interface{}, _ peer.ID) { // Everyone should relay they got the message genericMessage, ok := obj.(*testproto.GenericMessage) - if !ok { - t.Fatalf("invalid type assert") - } + require.True(t, ok, "invalid type assert") messageCh <- genericMessage - }); subscribeErr != nil { - t.Fatalf("Unable to subscribe to topic, %v", subscribeErr) - } + }) + require.NoError(t, subscribeErr, "Unable to subscribe to topic") } publisher := servers[0] @@ -80,16 +72,14 @@ func TestSimpleGossip(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - if waitErr := WaitForSubscribers(ctx, publisher, topicName, len(servers)-1); waitErr != nil { - t.Fatalf("Unable to wait for subscribers, %v", waitErr) - } + err := WaitForSubscribers(ctx, publisher, topicName, len(servers)-1) + require.NoError(t, err, "Unable to wait for subscribers") - if publishErr := publisherTopic.Publish( + err = publisherTopic.Publish( &testproto.GenericMessage{ Message: sentMessage, - }); publishErr != nil { - t.Fatalf("Unable to publish message, %v", publishErr) - } + }) + require.NoError(t, err, "Unable to publish message") messagesGossiped := 0 diff --git a/scripts/fuzzAll b/scripts/fuzzAll index 9f49af24d2..cdca89749e 100755 --- a/scripts/fuzzAll +++ b/scripts/fuzzAll @@ -13,6 +13,6 @@ do do echo "Fuzzing $func in $file" parentDir=$(dirname $file) - go test $parentDir -run=$func -fuzz=$func -fuzztime=${fuzzTime}s + go test $parentDir -race -run=$func -fuzz=$func -fuzztime=${fuzzTime}s done done diff --git a/state/runtime/tracer/structtracer/tracer.go b/state/runtime/tracer/structtracer/tracer.go index 5136fe84a6..54dfcff20f 100644 --- a/state/runtime/tracer/structtracer/tracer.go +++ b/state/runtime/tracer/structtracer/tracer.go @@ -80,6 +80,9 @@ func (t *StructTracer) cancelled() bool { } func (t *StructTracer) Clear() { + t.cancelLock.Lock() + defer t.cancelLock.Unlock() + t.reason = nil t.interrupt = false t.logs = t.logs[:0] @@ -328,6 +331,9 @@ type StructTraceResult struct { } func (t *StructTracer) GetResult() (interface{}, error) { + t.cancelLock.RLock() + defer t.cancelLock.RUnlock() + if t.reason != nil { return nil, t.reason } diff --git a/syncer/client_test.go b/syncer/client_test.go index 9a35d68998..37b91eced1 100644 --- a/syncer/client_test.go +++ b/syncer/client_test.go @@ -45,6 +45,7 @@ func newTestSyncPeerClient(network Network, blockchain Blockchain) *syncPeerClie id: network.AddrInfo().ID.String(), peerStatusUpdateCh: make(chan *NoForkPeer, 1), peerConnectionUpdateCh: make(chan *event.PeerEvent, 1), + closeCh: make(chan struct{}), } // need to register protocol @@ -211,7 +212,7 @@ func TestStatusPubSub(t *testing.T) { assert.NoError(t, err) // close channel and wait for events - close(client.peerConnectionUpdateCh) + close(client.closeCh) wg.Wait() diff --git a/syncer/peers_test.go b/syncer/peers_test.go index 765fcfe776..0dcb19aeef 100644 --- a/syncer/peers_test.go +++ b/syncer/peers_test.go @@ -9,8 +9,8 @@ import ( "github.com/stretchr/testify/assert" ) -var ( - peers = []*NoForkPeer{ +func getAllTestPeers() []*NoForkPeer { + return []*NoForkPeer{ { ID: peer.ID("A"), Number: 10, @@ -27,7 +27,7 @@ var ( Distance: big.NewInt(1), }, } -) +} func cloneNoForkPeers(peers []*NoForkPeer) []*NoForkPeer { clone := make([]*NoForkPeer, len(peers)) @@ -75,14 +75,11 @@ func peerMapToPeers(peerMap *PeerMap) []*NoForkPeer { func TestConstructor(t *testing.T) { t.Parallel() - peers := peers - + peers := getAllTestPeers() peerMap := NewPeerMap(peers) - expected := sortNoForkPeers( cloneNoForkPeers(peers), ) - actual := peerMapToPeers(peerMap) assert.Equal( @@ -95,8 +92,9 @@ func TestConstructor(t *testing.T) { func TestPutPeer(t *testing.T) { t.Parallel() - initialPeers := peers[:1] - peers := peers[1:] + allPeers := getAllTestPeers() + initialPeers := allPeers[:1] + peers := allPeers[1:] peerMap := NewPeerMap(initialPeers) @@ -118,6 +116,8 @@ func TestPutPeer(t *testing.T) { func TestBestPeer(t *testing.T) { t.Parallel() + allPeers := getAllTestPeers() + tests := []struct { name string skipList map[peer.ID]bool @@ -127,8 +127,8 @@ func TestBestPeer(t *testing.T) { { name: "should return best peer", skipList: nil, - peers: peers, - result: peers[2], + peers: allPeers, + result: allPeers[2], }, { name: "should return null in case of empty map", @@ -141,8 +141,8 @@ func TestBestPeer(t *testing.T) { skipList: map[peer.ID]bool{ peer.ID("C"): true, }, - peers: peers, - result: peers[1], + peers: allPeers, + result: allPeers[1], }, } diff --git a/txpool/event_subscription_test.go b/txpool/event_subscription_test.go index 4a0abb8389..5b0a0f9f0e 100644 --- a/txpool/event_subscription_test.go +++ b/txpool/event_subscription_test.go @@ -141,7 +141,7 @@ func TestEventSubscription_ProcessedEvents(t *testing.T) { } wg.Wait() - eventWaitCtx, eventWaitFn := context.WithTimeout(context.Background(), time.Second*5) + eventWaitCtx, eventWaitFn := context.WithTimeout(context.Background(), 10*time.Second) defer eventWaitFn() if _, err := tests.RetryUntilTimeout(eventWaitCtx, func() (interface{}, bool) { return nil, atomic.LoadInt64(&processed) < int64(testCase.expectedProcessed) diff --git a/txpool/operator.go b/txpool/operator.go index 4d7bdb5186..a11c1aa449 100644 --- a/txpool/operator.go +++ b/txpool/operator.go @@ -43,7 +43,7 @@ func (p *TxPool) AddTxn(ctx context.Context, raw *proto.AddTxnReq) (*proto.AddTx } return &proto.AddTxnResp{ - TxHash: txn.Hash.String(), + TxHash: txn.GetHash().String(), }, nil } diff --git a/txpool/queue_account.go b/txpool/queue_account.go index 78b938199f..10801d5f2d 100644 --- a/txpool/queue_account.go +++ b/txpool/queue_account.go @@ -104,6 +104,14 @@ func (q *accountQueue) length() uint64 { return uint64(q.queue.Len()) } +// lengthWithLock returns the number of transactions in the queue (thread-safe) +func (q *accountQueue) lengthWithLock() uint64 { + q.lock(false) + defer q.unlock() + + return q.length() +} + // transactions sorted by nonce (ascending) type minNonceQueue []*types.Transaction diff --git a/txpool/txpool.go b/txpool/txpool.go index 7305acec62..232b280084 100644 --- a/txpool/txpool.go +++ b/txpool/txpool.go @@ -441,7 +441,7 @@ func (p *TxPool) dropAccount(account *account, nextNonce uint64, tx *types.Trans dropped = account.enqueued.clear() clearAccountQueue(dropped) - p.eventManager.signalEvent(proto.EventType_DROPPED, tx.Hash) + p.eventManager.signalEvent(proto.EventType_DROPPED, tx.GetHash()) if p.logger.IsDebug() { p.logger.Debug("dropped account txs", @@ -475,7 +475,7 @@ func (p *TxPool) Demote(tx *types.Transaction) { account.incrementDemotions() - p.eventManager.signalEvent(proto.EventType_DEMOTED, tx.Hash) + p.eventManager.signalEvent(proto.EventType_DEMOTED, tx.GetHash()) } // ResetWithHeaders processes the transactions from the new @@ -769,7 +769,7 @@ func (p *TxPool) pruneAccountsWithNonceHoles() { // (only once) and an enqueueRequest is signaled. func (p *TxPool) addTx(origin txOrigin, tx *types.Transaction) error { if p.logger.IsDebug() { - p.logger.Debug("add tx", "origin", origin.String(), "hash", tx.Hash.String()) + p.logger.Debug("add tx", "origin", origin.String(), "hash", tx.GetHash().String()) } // validate incoming tx @@ -816,7 +816,7 @@ func (p *TxPool) addTx(origin txOrigin, tx *types.Transaction) error { // try to find if there is transaction with same nonce for this account oldTxWithSameNonce := account.nonceToTx.get(tx.Nonce) if oldTxWithSameNonce != nil { - if oldTxWithSameNonce.Hash == tx.Hash { + if oldTxWithSameNonce.GetHash() == tx.GetHash() { metrics.IncrCounter([]string{txPoolMetrics, "already_known_tx"}, 1) return ErrAlreadyKnown @@ -870,13 +870,14 @@ func (p *TxPool) addTx(origin txOrigin, tx *types.Transaction) error { } func (p *TxPool) invokePromotion(tx *types.Transaction, callPromote bool) { - p.eventManager.signalEvent(proto.EventType_ADDED, tx.Hash) + txHash := tx.GetHash() + p.eventManager.signalEvent(proto.EventType_ADDED, txHash) if p.logger.IsDebug() { - p.logger.Debug("enqueue request", "hash", tx.Hash.String()) + p.logger.Debug("enqueue request", "hash", txHash.String()) } - p.eventManager.signalEvent(proto.EventType_ENQUEUED, tx.Hash) + p.eventManager.signalEvent(proto.EventType_ENQUEUED, txHash) if callPromote { select { @@ -942,13 +943,13 @@ func (p *TxPool) addGossipTx(obj interface{}, _ peer.ID) { if err := p.addTx(gossip, tx); err != nil { if errors.Is(err, ErrAlreadyKnown) { if p.logger.IsDebug() { - p.logger.Debug("rejecting known tx (gossip)", "hash", tx.Hash.String()) + p.logger.Debug("rejecting known tx (gossip)", "hash", tx.GetHash().String()) } return } - p.logger.Error("failed to add broadcast tx", "err", err, "hash", tx.Hash.String()) + p.logger.Error("failed to add broadcast tx", "err", err, "hash", tx.GetHash().String()) } } @@ -1070,7 +1071,7 @@ func (p *TxPool) Length() uint64 { // toHash returns the hash(es) of given transaction(s) func toHash(txs ...*types.Transaction) (hashes []types.Hash) { for _, tx := range txs { - hashes = append(hashes, tx.Hash) + hashes = append(hashes, tx.GetHash()) } return diff --git a/txpool/txpool_test.go b/txpool/txpool_test.go index db43b6d551..7bd23c254c 100644 --- a/txpool/txpool_test.go +++ b/txpool/txpool_test.go @@ -34,14 +34,6 @@ const ( validGasLimit uint64 = 4712350 ) -var ( - forks = (&chain.Forks{ - chain.Homestead: chain.NewFork(0), - chain.Istanbul: chain.NewFork(0), - chain.London: chain.NewFork(0), - }) -) - // addresses used in tests var ( addr1 = types.Address{0x1} @@ -91,7 +83,7 @@ func newTestPoolWithSlots(maxSlots uint64, mockStore ...store) (*TxPool, error) return NewTxPool( hclog.NewNullLogger(), - forks, + getDefaultEnabledForks(), storeToUse, nil, nil, @@ -2137,7 +2129,7 @@ func Test_TxPool_validateTx(t *testing.T) { t.Run("tx input larger than the TxPoolMaxInitCodeSize", func(t *testing.T) { t.Parallel() pool := setupPool() - pool.forks = chain.AllForksEnabled + pool.forks = chain.AllForksEnabled.Copy() input := make([]byte, state.TxPoolMaxInitCodeSize+1) _, err := rand.Read(input) @@ -2156,7 +2148,7 @@ func Test_TxPool_validateTx(t *testing.T) { t.Run("tx input the same as TxPoolMaxInitCodeSize", func(t *testing.T) { t.Parallel() pool := setupPool() - pool.forks = chain.AllForksEnabled + pool.forks = chain.AllForksEnabled.Copy() input := make([]byte, state.TxPoolMaxInitCodeSize) _, err := rand.Read(input) @@ -2165,6 +2157,7 @@ func Test_TxPool_validateTx(t *testing.T) { tx := newTx(defaultAddr, 0, 1) tx.To = nil tx.Input = input + tx.GasPrice = new(big.Int).SetUint64(pool.GetBaseFee()) assert.NoError(t, pool.validateTx(signTx(tx)), @@ -2279,7 +2272,7 @@ func Test_TxPool_validateTx(t *testing.T) { t.Run("eip-1559 tx placed without eip-1559 fork enabled", func(t *testing.T) { t.Parallel() pool := setupPool() - pool.forks = chain.AllForksEnabled + pool.forks = chain.AllForksEnabled.Copy() pool.forks.RemoveFork(chain.London) tx := newTx(defaultAddr, 0, 1) @@ -2613,10 +2606,10 @@ func TestResetAccounts_Enqueued(t *testing.T) { newTx(addr1, 4, 1), }, addr2: { - newTx(addr2, 3, 1), - newTx(addr2, 4, 1), - newTx(addr2, 5, 1), - newTx(addr2, 6, 1), + newTx(addr2, 3, 3), + newTx(addr2, 4, 3), + newTx(addr2, 5, 3), + newTx(addr2, 6, 3), }, addr3: { newTx(addr3, 7, 1), @@ -3528,7 +3521,7 @@ func TestAddTxsInOrder(t *testing.T) { wg.Wait() - time.Sleep(time.Second * 2) + time.Sleep(100 * time.Millisecond) pool.Close() @@ -3536,8 +3529,8 @@ func TestAddTxsInOrder(t *testing.T) { acc := pool.accounts.get(addrtx.addr) require.NotNil(t, acc) - assert.Equal(t, uint64(0), acc.enqueued.length()) - assert.Equal(t, len(acc.nonceToTx.mapping), int(acc.promoted.length())) + assert.Equal(t, uint64(0), acc.enqueued.lengthWithLock()) + assert.Len(t, acc.nonceToTx.mapping, int(acc.promoted.length())) } } @@ -3698,6 +3691,16 @@ func TestAddTx_TxReplacement(t *testing.T) { assert.Equal(t, ac2.enqueued.queue[0], tx1) } +// getDefaultEnabledForks returns hardcoded set of forks +// that are enabled by default from the genesis block +func getDefaultEnabledForks() *chain.Forks { + return &chain.Forks{ + chain.Homestead: chain.NewFork(0), + chain.Istanbul: chain.NewFork(0), + chain.London: chain.NewFork(0), + } +} + func BenchmarkAddTxTime(b *testing.B) { b.Run("benchmark add one tx", func(b *testing.B) { signer := crypto.NewEIP155Signer(100, true) diff --git a/txrelayer/txrelayer.go b/txrelayer/txrelayer.go index 9517a65a99..daec6397ac 100644 --- a/txrelayer/txrelayer.go +++ b/txrelayer/txrelayer.go @@ -207,31 +207,38 @@ func (t *TxRelayerImpl) sendTransactionLocked(txn *ethgo.Transaction, key ethgo. // SendTransactionLocal sends non-signed transaction // (this function is meant only for testing purposes and is about to be removed at some point) func (t *TxRelayerImpl) SendTransactionLocal(txn *ethgo.Transaction) (*ethgo.Receipt, error) { - accounts, err := t.client.Eth().Accounts() + txnHash, err := t.sendTransactionLocalLocked(txn) if err != nil { return nil, err } + return t.waitForReceipt(txnHash) +} + +func (t *TxRelayerImpl) sendTransactionLocalLocked(txn *ethgo.Transaction) (ethgo.Hash, error) { + t.lock.Lock() + defer t.lock.Unlock() + + accounts, err := t.client.Eth().Accounts() + if err != nil { + return ethgo.ZeroHash, err + } + if len(accounts) == 0 { - return nil, errNoAccounts + return ethgo.ZeroHash, errNoAccounts } txn.From = accounts[0] gasLimit, err := t.client.Eth().EstimateGas(ConvertTxnToCallMsg(txn)) if err != nil { - return nil, err + return ethgo.ZeroHash, err } txn.Gas = gasLimit txn.GasPrice = defaultGasPrice - txnHash, err := t.client.Eth().SendTransaction(txn) - if err != nil { - return nil, err - } - - return t.waitForReceipt(txnHash) + return t.client.Eth().SendTransaction(txn) } func (t *TxRelayerImpl) waitForReceipt(hash ethgo.Hash) (*ethgo.Receipt, error) { diff --git a/types/transaction.go b/types/transaction.go index 126545f585..7d613702fd 100644 --- a/types/transaction.go +++ b/types/transaction.go @@ -3,6 +3,7 @@ package types import ( "fmt" "math/big" + "sync" "sync/atomic" "github.com/0xPolygon/polygon-edge/helper/common" @@ -65,6 +66,8 @@ type Transaction struct { ChainID *big.Int + lock sync.RWMutex + // Cache size atomic.Pointer[uint64] } @@ -83,7 +86,16 @@ func (t *Transaction) ComputeHash(blockNumber uint64) *Transaction { func (t *Transaction) Copy() *Transaction { tt := new(Transaction) - *tt = *t + tt.Nonce = t.Nonce + tt.From = t.From + tt.Gas = t.Gas + tt.Type = t.Type + tt.Hash = t.Hash + + if t.To != nil { + newAddress := *t.To + tt.To = &newAddress + } tt.GasPrice = new(big.Int) if t.GasPrice != nil { @@ -117,12 +129,32 @@ func (t *Transaction) Copy() *Transaction { tt.S = new(big.Int).Set(t.S) } + if t.ChainID != nil { + tt.ChainID = new(big.Int).Set(t.ChainID) + } + tt.Input = make([]byte, len(t.Input)) copy(tt.Input[:], t.Input[:]) return tt } +// GetHash reads transaction hash in a thread-safe manner +func (t *Transaction) GetHash() Hash { + t.lock.RLock() + defer t.lock.RUnlock() + + return t.Hash +} + +// SetHash sets transaction hash in a thread-safe manner +func (t *Transaction) SetHash(hash Hash) { + t.lock.Lock() + defer t.lock.Unlock() + + t.Hash = hash +} + // Cost returns gas * gasPrice + value func (t *Transaction) Cost() *big.Int { var factor *big.Int diff --git a/types/transaction_fork_hash.go b/types/transaction_fork_hash.go index 6c84461e5d..77fb6ac0fc 100644 --- a/types/transaction_fork_hash.go +++ b/types/transaction_fork_hash.go @@ -47,7 +47,9 @@ func (th *TransactionHashForkV1) ComputeHash(t *Transaction) { t.ChainID = big.NewInt(0) v := t.MarshalRLPWith(ar) - hash.WriteRlp(t.Hash[:0], v) + hashTmp := ZeroHash + hash.WriteRlp(hashTmp[:0], v) + t.SetHash(hashTmp) t.ChainID = chainID @@ -64,7 +66,9 @@ func (th *TransactionHashForkV2) SerializeForRootCalculation(t *Transaction, _ * func (th *TransactionHashForkV2) ComputeHash(t *Transaction) { hash := keccak.DefaultKeccakPool.Get() - hash.WriteFn(t.Hash[:0], t.MarshalRLPTo) + hashTmp := ZeroHash + hash.WriteFn(hashTmp[:0], t.MarshalRLPTo) + t.SetHash(hashTmp) keccak.DefaultKeccakPool.Put(hash) }