Don't notify cache about nonces; pass the account nonce provider, instead #6603

andreibancioiu · 2024-11-13T21:13:08Z

Reasoning behind the pull request

This is an improvement upon Mempool: improve selection algorithm #6581.
In certain exotic cases, concurrent notifications about account nonces might have lead the mempool to hold stale account nonces (very minor issue, but still an issue).
The flow of forgetting all account nonces after reverting a block is suboptimal (and somehow over-engineered).
Transaction de-duplication (based on nonces) wasn't entirely correct.
Accounts with transactions recently proposed in scheduled blocks had some issues with following transactions (selection was delayed due to missing nonce notifications).

Proposed changes

Don't be notified about account nonces, don't store them; get them at selection-time mx-chain-storage-go#57
Also merged this one (PR was previously lost): Optimization: encode hash to hex only on log TRACE level set #6338
Also merged this one: fix spamming warnings from apiTransactionProcessor #6604
Drop txs.accountTxsShards
Fix old concurrency issue in requestHandler (found by @sstanculeanu and @miiu96): 5bb1b40.

Testing procedure

Standard testing
Heavy load, to trigger evictions from the mempool

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

Optimization: encode hash to hex only on log TRACE level set

…ACE.

…/mx-chain-go into MX-16107-no-more-notify

…tTransactions().

…ace(). Encoding will happen under the hood (if trace is active).

…otifications).

…rom account state is necessary).

andreibancioiu · 2024-11-21T08:13:51Z

genesis/process/shardGenesisBlockCreator.go

@@ -600,6 +600,7 @@ func createProcessorsForShardGenesisBlock(arg ArgsGenesisBlockCreator, enableEpo
 		disabledScheduledTxsExecutionHandler,
 		disabledProcessedMiniBlocksTracker,
 		arg.TxExecutionOrderHandler,
+		disabledGuardian.NewDisabledGuardedAccountHandler(),


Genesis components, we can pass this object.

andreibancioiu · 2024-11-21T08:16:10Z

process/block/preprocess/interfaces.go

 	IsInterfaceNil() bool
 }

 // TxCache defines the functionality for the transactions cache
 type TxCache interface {
-	SelectTransactions(gasRequested uint64, maxNum int) ([]*txcache.WrappedTransaction, uint64)
-	NotifyAccountNonce(accountKey []byte, nonce uint64)
+	SelectTransactions(accountStateProvider txcache.AccountStateProvider, gasRequested uint64, maxNum int, selectionLoopMaximumDuration time.Duration) ([]*txcache.WrappedTransaction, uint64)


Now, we pass the (wrapped) accounts adapter; the mempool will be much more informed during selection.

andreibancioiu · 2024-11-21T08:16:37Z

process/block/preprocess/miniBlockBuilder.go

@@ -136,15 +130,6 @@ func checkMiniBlocksBuilderArgs(args miniBlocksBuilderArgs) error {
 	return nil
 }

-func (mbb *miniBlocksBuilder) updateAccountShardsInfo(tx *transaction.Transaction, wtx *txcache.WrappedTransaction) {


Was used for mempool notifications.

andreibancioiu · 2024-11-21T09:05:48Z

process/block/preprocess/transactions_test.go

@@ -446,6 +448,26 @@ func TestTxsPreprocessor_NewTransactionPreprocessorNilProcessedMiniBlocksTracker
 	assert.Equal(t, process.ErrNilProcessedMiniBlocksTracker, err)
 }

+func TestTxsPreprocessor_NewTransactionPreprocessorNilTxExecutionOrderHandler(t *testing.T) {


Was missing before.

andreibancioiu · 2024-11-21T09:06:29Z

process/constants.go

@@ -143,3 +144,6 @@ const TxCacheSelectionGasRequested = 10_000_000_000

 // TxCacheSelectionMaxNumTxs defines the maximum number of transactions that should be selected from the cache.
 const TxCacheSelectionMaxNumTxs = 50000
+
+// TxCacheSelectionLoopMaximumDuration defines the maximum duration for the loop that selects transactions from the cache.
+const TxCacheSelectionLoopMaximumDuration = 250 * time.Millisecond


Question for review: all right?

sounds good for 6 second round time, can remain like this. should be much lower for sub second round times

andreibancioiu · 2024-11-21T09:19:30Z

state/accountsDB.go

-	log.Trace("accountsDB.saveAccountToTrie",
-		"address", hex.EncodeToString(accountHandler.AddressBytes()),
-	)
+	log.Trace("accountsDB.saveAccountToTrie", "address", accountHandler.AddressBytes())


Still sub-optimal (slice is copied by value when returned by the function), but a bit better.

…at a later time.

… PR.

dataRetriever/txpool/memorytests/memory_test.go

raduchis · 2024-11-26T16:30:10Z

go.mod

@@ -20,7 +20,7 @@ require (
 	github.com/multiversx/mx-chain-es-indexer-go v1.7.10
 	github.com/multiversx/mx-chain-logger-go v1.0.15
 	github.com/multiversx/mx-chain-scenario-go v1.4.4
-	github.com/multiversx/mx-chain-storage-go v1.0.17
+	github.com/multiversx/mx-chain-storage-go v1.0.18-0.20241124203643-93c958ad66cf


do not forget actual release

Will be set after all merges into the feat branch, right before preparing the release.

process/block/preprocess/transactions.go

process/block/preprocess/basePreProcess.go

raduchis · 2024-11-26T16:49:08Z

process/constants.go

@@ -143,3 +144,6 @@ const TxCacheSelectionGasRequested = 10_000_000_000

 // TxCacheSelectionMaxNumTxs defines the maximum number of transactions that should be selected from the cache.
 const TxCacheSelectionMaxNumTxs = 50000
+
+// TxCacheSelectionLoopMaximumDuration defines the maximum duration for the loop that selects transactions from the cache.
+const TxCacheSelectionLoopMaximumDuration = 250 * time.Millisecond


sounds good for 6 second round time, can remain like this. should be much lower for sub second round times

AdoAdoAdo · 2024-11-27T11:33:14Z

go.mod

@@ -20,7 +20,7 @@ require (
 	github.com/multiversx/mx-chain-es-indexer-go v1.7.10
 	github.com/multiversx/mx-chain-logger-go v1.0.15
 	github.com/multiversx/mx-chain-scenario-go v1.4.4
-	github.com/multiversx/mx-chain-storage-go v1.0.17
+	github.com/multiversx/mx-chain-storage-go v1.0.18-0.20241126200205-8a0cb502901f


don't forget to update after fixes.

We reference the latest commit from here: https://github.com/multiversx/mx-chain-storage-go/pull/57/commits.

A proper tag be set after all merges into the feat branch, right before preparing the release.

AdoAdoAdo · 2024-11-27T12:41:55Z

process/block/preprocess/transactions.go

@@ -34,15 +33,10 @@ var _ process.PreProcessor = (*transactions)(nil)
 var log = logger.GetOrCreate("process/block/preprocess")

 // 200% bandwidth to allow 100% overshooting estimations
-const selectionGasBandwidthIncreasePercent = 200
+const selectionGasBandwidthIncreasePercent = 400


maybe move these two constants in config?

We should, yes. Added MX-16201 (we have some other constants to move to config).

cristure and others added 6 commits July 26, 2024 18:21

calculate hash only on log trace level set.

6841919

add non-memory thrasher test.

85d93ac

Merge branch 'rc/v1.7.next1' into benchmark_trie

79eb08c

Merge branch 'rc/v1.7.next1' into benchmark_trie

1ec7c5c

Convert artificial test into a benchmark.

af01f4a

No more ForgetAllAccountNonces().

febe899

andreibancioiu changed the title ~~Don't notify cache about nonces; provide the account nonce provider, instead~~ Don't notify cache about nonces; pass the account nonce provider, instead Nov 13, 2024

andreibancioiu added 3 commits November 13, 2024 23:18

Remove NotifyAccountNonce().

535d51b

Properly invoke NewTxCache().

bca90d9

Reference new storage-go.

278acf9

andreibancioiu self-assigned this Nov 13, 2024

andreibancioiu added 3 commits November 14, 2024 09:45

Fix long test.

f533a2b

Newer storage-go.

4b8fb0a

Merge branch 'selection-by-ppu' into MX-16107-no-more-notify

a98f0a4

sasurobert previously approved these changes Nov 14, 2024

View reviewed changes

andreibancioiu and others added 5 commits November 14, 2024 16:17

Sketch CS test: TestMempoolWithChainSimulator_Eviction.

0cb7187

Merge branch 'rc/v1.7.next1' into benchmark_trie

bc70dcc

Merge pull request #6338 from multiversx/benchmark_trie

6a80857

Optimization: encode hash to hex only on log TRACE level set

Optimize GetExistingAccount(). Only encode address if log level is TR…

c4f2db5

…ACE.

Merge branch 'MX-16107-no-more-notify' of ssh://github.com/multiversx…

1cb7594

…/mx-chain-go into MX-16107-no-more-notify

raduchis previously approved these changes Nov 14, 2024

View reviewed changes

andreibancioiu added 9 commits November 15, 2024 15:30

Pass account nonce provider (backed by the accounts adapter) to Selec…

26ddc02

…tTransactions().

Optimized logging. Don't directly encode to hex before calling log.Tr…

38aeb55

…ace(). Encoding will happen under the hood (if trace is active).

Remove logic around "accountTxsShards" (was only needed for mempool n…

297e530

…otifications).

Remove code not needed anymore.

e1d4287

Minor refactoring around nonce account provider.

f84a3c6

Sketch additional CS tests (WIP).

9aec5b9

AccountNonceProvider becomes AccountStateProvider (more information f…

5b937db

…rom account state is necessary).

Merge branch 'selection-by-ppu' into MX-16107-no-more-notify

7fa9a6c

Fix tests, reference newer storage.

7741b4e

Fix tests.

e1cb834

multiversx deleted a comment from github-actions bot Nov 20, 2024

Reference new storage-go.

eca8029

andreibancioiu commented Nov 21, 2024

View reviewed changes

andreibancioiu added 2 commits November 21, 2024 14:30

Fix tests, fix after self-review.

9005e76

Remove chain simulator test (since it's not polished). Will be added …

cfd1ea8

…at a later time.

andreibancioiu marked this pull request as ready for review November 21, 2024 13:55

andreibancioiu added 4 commits November 22, 2024 11:22

In "interceptedHeader", handle concurrent access.

e02d607

Handle non-executable transactions related to guardians in a separate…

2d52815

… PR.

Fix linter issues.

2a160f3

(final) fix after self-review.

47159a5

raduchis requested changes Nov 26, 2024

View reviewed changes

Fix after review.

25b2594

andreibancioiu mentioned this pull request Nov 27, 2024

Mempool: handle not-executable transactions related to guardians #6628

Merged

andreibancioiu requested a review from raduchis November 27, 2024 08:16

andreibancioiu mentioned this pull request Nov 27, 2024

Mempool: completely handle fees and transferred balances within "detectWillFeeExceedBalance()" #6637

Merged

AdoAdoAdo reviewed Nov 27, 2024

View reviewed changes

andreibancioiu requested a review from AdoAdoAdo November 27, 2024 14:34

raduchis approved these changes Nov 27, 2024

View reviewed changes

AdoAdoAdo approved these changes Nov 27, 2024

View reviewed changes

sasurobert approved these changes Nov 27, 2024

View reviewed changes

andreibancioiu merged commit e04679e into feat/mempool Nov 27, 2024
5 checks passed

andreibancioiu deleted the MX-16107-no-more-notify branch November 27, 2024 17:15

andreibancioiu mentioned this pull request Nov 29, 2024

Mempool improvements #6640

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't notify cache about nonces; pass the account nonce provider, instead #6603

Don't notify cache about nonces; pass the account nonce provider, instead #6603

andreibancioiu commented Nov 13, 2024 •

edited

Loading

andreibancioiu Nov 21, 2024

andreibancioiu Nov 21, 2024

andreibancioiu Nov 21, 2024

andreibancioiu Nov 21, 2024

andreibancioiu Nov 21, 2024

raduchis Nov 26, 2024

andreibancioiu Nov 21, 2024

raduchis Nov 26, 2024

andreibancioiu Nov 26, 2024

raduchis Nov 26, 2024

AdoAdoAdo Nov 27, 2024

andreibancioiu Nov 27, 2024

AdoAdoAdo Nov 27, 2024

andreibancioiu Nov 27, 2024

Don't notify cache about nonces; pass the account nonce provider, instead #6603

Don't notify cache about nonces; pass the account nonce provider, instead #6603

Conversation

andreibancioiu commented Nov 13, 2024 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu commented Nov 13, 2024 •

edited

Loading