Skip to content
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

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

Merged
merged 8 commits into from
Nov 28, 2024
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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.18-0.20241128102604-90581ee84b60
github.com/multiversx/mx-chain-storage-go v1.0.18-0.20241128110709-156b1244e04f
github.com/multiversx/mx-chain-vm-common-go v1.5.16
github.com/multiversx/mx-chain-vm-go v1.5.37
github.com/multiversx/mx-chain-vm-v1_2-go v1.2.68
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -397,8 +397,8 @@ github.com/multiversx/mx-chain-logger-go v1.0.15 h1:HlNdK8etyJyL9NQ+6mIXyKPEBo+w
github.com/multiversx/mx-chain-logger-go v1.0.15/go.mod h1:t3PRKaWB1M+i6gUfD27KXgzLJJC+mAQiN+FLlL1yoGQ=
github.com/multiversx/mx-chain-scenario-go v1.4.4 h1:DVE2V+FPeyD/yWoC+KEfPK3jsFzHeruelESfpTlf460=
github.com/multiversx/mx-chain-scenario-go v1.4.4/go.mod h1:kI+TWR3oIEgUkbwkHCPo2CQ3VjIge+ezGTibiSGwMxo=
github.com/multiversx/mx-chain-storage-go v1.0.18-0.20241128102604-90581ee84b60 h1:XFO7MbjpdSJE/mC8wv3937iIWoAQC9YL2vwzO0bvmMg=
github.com/multiversx/mx-chain-storage-go v1.0.18-0.20241128102604-90581ee84b60/go.mod h1:eFDEOrG7Wiyk5I/ObpwcN2eoBlOnnfeEMTvTer1cymk=
github.com/multiversx/mx-chain-storage-go v1.0.18-0.20241128110709-156b1244e04f h1:sRPekt5fzNr+c7w2IzwufOeqANTT3Du6ciD3FX5mCvI=
github.com/multiversx/mx-chain-storage-go v1.0.18-0.20241128110709-156b1244e04f/go.mod h1:eFDEOrG7Wiyk5I/ObpwcN2eoBlOnnfeEMTvTer1cymk=
github.com/multiversx/mx-chain-vm-common-go v1.5.16 h1:g1SqYjxl7K66Y1O/q6tvDJ37fzpzlxCSfRzSm/woQQY=
github.com/multiversx/mx-chain-vm-common-go v1.5.16/go.mod h1:1rSkXreUZNXyPTTdhj47M+Fy62yjxbu3aAsXEtKN3UY=
github.com/multiversx/mx-chain-vm-go v1.5.37 h1:Iy3KCvM+DOq1f9UPA7uYK/rI3ZbBOXc2CVNO2/vm5zw=
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,11 @@ func TestShardShouldNotProposeAndExecuteTwoBlocksInSameRound(t *testing.T) {
}

// TestShardShouldProposeBlockContainingInvalidTransactions tests the following scenario:
// 1. generate 3 move balance transactions: one that can be executed, one that can not be executed but the account has
// the balance for the fee and one that is completely invalid (no balance left for it)
// 1. generate 3 move balance transactions: one that can be executed, one to be processed as invalid, and one that isn't executable (no balance left for fee).
// 2. proposer will have those 3 transactions in its pools and will propose a block
// 3. another node will be able to sync the proposed block (and request - receive) the 2 transactions that
// will end up in the block (one valid and one invalid)
// 4. the non-executable transaction will be removed from the proposer's pool
// 4. the non-executable transaction will not be immediately removed from the proposer's pool. See MX-16200.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MX-16200

func TestShardShouldProposeBlockContainingInvalidTransactions(t *testing.T) {
if testing.Short() {
t.Skip("this is not a short test")
Expand Down Expand Up @@ -195,7 +194,18 @@ func testStateOnNodes(t *testing.T, nodes []*integrationTests.TestProcessorNode,
testTxIsInMiniblock(t, proposer, hashes[txValidIdx], block.TxBlock)
testTxIsInMiniblock(t, proposer, hashes[txInvalidIdx], block.InvalidBlock)
testTxIsInNotInBody(t, proposer, hashes[txDeletedIdx])
testTxHashNotPresentInPool(t, proposer, hashes[txDeletedIdx])

// Removed from mempool.
_, ok := proposer.DataPool.Transactions().SearchFirstData(hashes[txValidIdx])
assert.False(t, ok)

// Removed from mempool.
_, ok = proposer.DataPool.Transactions().SearchFirstData(hashes[txInvalidIdx])
assert.False(t, ok)

// Not removed from mempool (see MX-16200).
_, ok = proposer.DataPool.Transactions().SearchFirstData(hashes[txDeletedIdx])
assert.True(t, ok)
}

func testSameBlockHeight(t *testing.T, nodes []*integrationTests.TestProcessorNode, idxProposer int, expectedHeight uint64) {
Expand All @@ -208,11 +218,6 @@ func testSameBlockHeight(t *testing.T, nodes []*integrationTests.TestProcessorNo
}
}

func testTxHashNotPresentInPool(t *testing.T, proposer *integrationTests.TestProcessorNode, hash []byte) {
txCache := proposer.DataPool.Transactions()
_, ok := txCache.SearchFirstData(hash)
assert.False(t, ok)
}

func testTxIsInMiniblock(t *testing.T, proposer *integrationTests.TestProcessorNode, hash []byte, bt block.Type) {
hdrHandler := proposer.BlockChain.GetCurrentBlockHeader()
Expand Down
90 changes: 85 additions & 5 deletions process/block/preprocess/selectionSession.go
Original file line number Diff line number Diff line change
@@ -1,32 +1,58 @@
package preprocess

import (
"bytes"
"errors"
"math/big"

"github.com/multiversx/mx-chain-core-go/core"
"github.com/multiversx/mx-chain-core-go/core/check"
"github.com/multiversx/mx-chain-core-go/data"
"github.com/multiversx/mx-chain-core-go/data/transaction"
"github.com/multiversx/mx-chain-core-go/marshal"
"github.com/multiversx/mx-chain-go/process"
"github.com/multiversx/mx-chain-go/state"
"github.com/multiversx/mx-chain-go/storage/txcache"
vmcommon "github.com/multiversx/mx-chain-vm-common-go"
"github.com/multiversx/mx-chain-vm-common-go/parsers"
)

type selectionSession struct {
accountsAdapter state.AccountsAdapter
transactionsProcessor process.TransactionProcessor
callArgumentsParser process.CallArgumentsParser
esdtTransferParser vmcommon.ESDTTransferParser
}

func newSelectionSession(accountsAdapter state.AccountsAdapter, transactionsProcessor process.TransactionProcessor) (*selectionSession, error) {
if check.IfNil(accountsAdapter) {
type argsSelectionSession struct {
accountsAdapter state.AccountsAdapter
transactionsProcessor process.TransactionProcessor
marshalizer marshal.Marshalizer
}

func newSelectionSession(args argsSelectionSession) (*selectionSession, error) {
if check.IfNil(args.accountsAdapter) {
return nil, process.ErrNilAccountsAdapter
}
if check.IfNil(transactionsProcessor) {
if check.IfNil(args.transactionsProcessor) {
return nil, process.ErrNilTxProcessor
}
if check.IfNil(args.marshalizer) {
return nil, process.ErrNilMarshalizer
}

argsParser := parsers.NewCallArgsParser()

esdtTransferParser, err := parsers.NewESDTTransferParser(args.marshalizer)
if err != nil {
return nil, err
}

return &selectionSession{
accountsAdapter: accountsAdapter,
transactionsProcessor: transactionsProcessor,
accountsAdapter: args.accountsAdapter,
transactionsProcessor: args.transactionsProcessor,
callArgumentsParser: argsParser,
esdtTransferParser: esdtTransferParser,
}, nil
}

Expand Down Expand Up @@ -73,6 +99,60 @@ func (session *selectionSession) IsIncorrectlyGuarded(tx data.TransactionHandler
return errors.Is(err, process.ErrTransactionNotExecutable)
}

// GetTransferredValue returns the value transferred by a transaction.
func (session *selectionSession) GetTransferredValue(tx data.TransactionHandler) *big.Int {
value := tx.GetValue()
hasValue := value != nil && value.Sign() != 0
if hasValue {
// Early exit (optimization): a transaction can either bear a regular value or be a "MultiESDTNFTTransfer".
return value
}

data := tx.GetData()
hasData := len(data) > 0
if !hasData {
// Early exit (optimization): no "MultiESDTNFTTransfer" to parse.
return tx.GetValue()
}

maybeMultiTransfer := bytes.HasPrefix(data, []byte(core.BuiltInFunctionMultiESDTNFTTransfer))
if !maybeMultiTransfer {
// Early exit (optimization).
return nil
}

function, args, err := session.callArgumentsParser.ParseData(string(data))
if err != nil {
return nil
}

if function != core.BuiltInFunctionMultiESDTNFTTransfer {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous (somehow)?

// Early exit (optimization).
return nil
}

esdtTransfers, err := session.esdtTransferParser.ParseESDTTransfers(tx.GetSndAddr(), tx.GetRcvAddr(), function, args)
if err != nil {
return nil
}

accumulatedNativeValue := big.NewInt(0)

for _, transfer := range esdtTransfers.ESDTTransfers {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this add a negligible overhead? A lot of transactions are with MultiESDTNFTTransfer
can the entire method be parallelized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good question, will benchmark ASAP 🙌

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// NOTE: 20% is also due to the require() / assert() calls.
// 0.012993s (TestBenchmarkSelectionSession_GetTransferredValue/numTransactions_=_5_000)
// 0.024580s (TestBenchmarkSelectionSession_GetTransferredValue/numTransactions_=_10_000)
// 0.048808s (TestBenchmarkSelectionSession_GetTransferredValue/numTransactions_=_20_000)

Thus, if we select ~10k, about 20 milliseconds (a bit much) will be spent in GetTransferredValue(). The selection duration is capped, though. There is still room for optimization.

We can move the call to GetTransferredValue() to the moment the transaction is added to the pool (additions are concurrent, also).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MX-16211

if transfer.ESDTTokenNonce != 0 {
continue
}
if string(transfer.ESDTTokenName) != vmcommon.EGLDIdentifier {
// We only care about native transfers.
continue
}

_ = accumulatedNativeValue.Add(accumulatedNativeValue, transfer.ESDTValue)
}

return accumulatedNativeValue
}

// IsInterfaceNil returns true if there is no value under the interface
func (session *selectionSession) IsInterfaceNil() bool {
return session == nil
Expand Down
Loading