Don't be notified about account nonces, don't store them; get them at selection-time #57
Conversation
andreibancioiu
changed the title
txcache/selection.go
Outdated
| } | ||
|
|
||
| // Item was popped from the heap, but not used downstream. | ||
| // Therefore, the sender is completely ignored in the current selection session. |
There was a problem hiding this comment.
It makes sense to not select any transaction of a given sender, if we've detected an initial gap.
txcache/selection.go
Outdated
| @@ -69,3 +104,22 @@ func selectTransactionsFromBunches(bunches []bunchOfTransactions, gasRequested u | |||
|
|
|||
| return selectedTransactions, accumulatedGas | |||
| } | |||
|
|
|||
| func (cache *TxCache) askAboutAccountNonceIfNecessary(item *transactionsHeapItem) { | |||
There was a problem hiding this comment.
requestAccountNonceIfNecessary ?
There was a problem hiding this comment.
Indeed, renamed.
…g it in constructor. This way, we can receive the "real" processing accounts adapter (not the slower API accounts adapter).
…rom account state is necessary).
…tions on missing balance for fees. Refactor.
| @@ -99,7 +99,12 @@ func (cache *TxCache) GetByTxHash(txHash []byte) (*WrappedTransaction, bool) { | |||
|
|
|||
| // SelectTransactions selects the best transactions to be included in the next miniblock. | |||
| // It returns up to "maxNum" transactions, with total gas <= "gasRequested". | |||
| func (cache *TxCache) SelectTransactions(gasRequested uint64, maxNum int) ([]*WrappedTransaction, uint64) { | |||
| func (cache *TxCache) SelectTransactions(accountStateProvider AccountStateProvider, gasRequested uint64, maxNum int, selectionLoopMaximumDuration time.Duration) ([]*WrappedTransaction, uint64) { | |||
There was a problem hiding this comment.
Now, we also pass a wrapped accounts adapter, so that we can learn the nonce, balance etc. of a sender with pending transactions. This allows us to perform a much more informed selection.
Additionally, we parametrize the selection with selectionLoopMaximumDuration (safety mechanism).
txcache/wrappedTransaction.go
Outdated
| wrappedTx.Fee.Store(fee) | ||
| wrappedTx.PricePerUnit.Store(fee.Uint64() / gasLimit) | ||
|
|
||
| txAsGuardedTransaction, ok := wrappedTx.Tx.(data.GuardedTransactionHandler) |
There was a problem hiding this comment.
See MX-16157.
| @@ -41,8 +41,8 @@ github.com/mr-tron/base58 v1.2.0 h1:T/HDJBh4ZCPbU39/+c3rRvE0uKBQlU27+QI8LJ4t64o= | |||
| github.com/mr-tron/base58 v1.2.0/go.mod h1:BinMc/sQntlIE1frQmRFPUoPA1Zkr8VRgBdjWI2mNwc= | |||
| github.com/multiversx/concurrent-map v0.1.4 h1:hdnbM8VE4b0KYJaGY5yJS2aNIW9TFFsUYwbO0993uPI= | |||
| github.com/multiversx/concurrent-map v0.1.4/go.mod h1:8cWFRJDOrWHOTNSqgYCUvwT7c7eFQ4U2vKMOp4A/9+o= | |||
| github.com/multiversx/mx-chain-core-go v1.2.21 h1:+XVKznPTlUU5EFS1A8chtS8fStW60upRIyF4Pgml19I= | |||
| github.com/multiversx/mx-chain-core-go v1.2.21/go.mod h1:B5zU4MFyJezmEzCsAHE9YNULmGCm2zbPHvl9hazNxmE= | |||
| github.com/multiversx/mx-chain-core-go v1.2.23 h1:8WlCGqJHR2HQ0vN4feJwb7W4VrCwBGIzPPHunOOg5Wc= | |||
There was a problem hiding this comment.
Forgot to update before.
| heap.Push(transactionsHeap, item) | ||
| } | ||
| } | ||
|
|
||
| return selectedTransactions, accumulatedGas | ||
| } | ||
|
|
||
| func detectSkippableSender(item *transactionsHeapItem) bool { |
There was a problem hiding this comment.
Skippable from now on, within the current selection session.
| for i := 0; i < 100; i++ { | ||
| fmt.Println("Add / remove", i) | ||
| cache.Remove([]byte("alice-x-1")) | ||
| cache.AddTx(expensiveTransaction) |
There was a problem hiding this comment.
Same objects aren't re-added to the pool (ever).
| return | ||
| } | ||
|
|
||
| item.accumulatedFee.Add(item.accumulatedFee, fee) |
| return hasMiddleGap | ||
| } | ||
|
|
||
| func (item *transactionsHeapItem) detectWillFeeExceedBalance() bool { |
andreibancioiu
changed the base branch from
selection-by-ppu
to
feat/mempool
andreibancioiu
dismissed
raduchis’s stale review
The base branch was changed.
| PricePerUnit atomic.Uint64 | ||
| // These fields are only set within "precomputeFields". | ||
| // We don't need to protect them with a mutex, since "precomputeFields" is called only once for each transaction. | ||
| // Additional note: "WrappedTransaction" objects are created by the Node, in dataRetriever/txpool/shardedTxPool.go. |
There was a problem hiding this comment.
Some (older) technical debt around this matter, we can improve the design in the future.
| if item.latestSelectedTransaction != nil { | ||
| return false | ||
| } | ||
| if item.senderState == nil { |
There was a problem hiding this comment.
is this for optimistic reasons? if senderState is not loaded, it has no initialGap?
There was a problem hiding this comment.
In practice, the body of that check is dead code.
The if item.senderState == nil is more like a "defensive programming" artifact (pessimistic).
There shouldn't be any case where account state is not retrievable. If that happens, the error is logged and the sender is skipped. Thus, for senders that we handle, sender state is never nil. requestAccountStateIfNecessary always precedes calls of detect*().
Do you recommend to drop these superfluous checks about senderState?
There was a problem hiding this comment.
nope, they can remain like this because of the "defensive programming" checks
Improvement upon #55.
In the mempool, do not receive notifications about current account nonces, do not be asked to forget known nonces.
Instead, receive an account nonce provider in the constructor of
TxCache, and use it when selecting transactions.This way, we improve the robustness of:
Other notes: