-
Notifications
You must be signed in to change notification settings - Fork 204
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 optimization: on transaction addition, call "GetTransferredValue" and retain its value #6641
Conversation
if check.IfNil(args.txGasHandler) { | ||
return nil, dataRetriever.ErrNilTxGasHandler | ||
} | ||
if check.IfNil(args.marshalizer) { | ||
return nil, dataRetriever.ErrNilMarshalizer | ||
} |
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.
Somehow superfluous checks (dependencies are verified one layer above, as well), should I remove them?
} | ||
|
||
// GetTransferredValue returns the value transferred by a transaction. | ||
func (host *mempoolHost) GetTransferredValue(tx data.TransactionHandler) *big.Int { |
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.
Function moved from SelectionSession
, no other change.
require.NotNil(t, host) | ||
} | ||
|
||
func TestMempoolHost_GetTransferredValue(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.
Tests simply moved from selectionSession_test.go
.
argsParser := parsers.NewCallArgsParser() | ||
|
||
esdtTransferParser, err := parsers.NewESDTTransferParser(args.marshalizer) |
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 not get these two as arguments as well?
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 instantiate them within mempoolHost
, so that shardedTxPool
doesn't have knowledge on the components from VM common parsers
- somehow, a bit of information hiding (even though both mempoolHost
and shardedTxPool
live in the same package).
dataRetriever/txpool/mempoolHost.go
Outdated
maybeMultiTransfer := bytes.HasPrefix(data, []byte(core.BuiltInFunctionMultiESDTNFTTransfer)) | ||
if !maybeMultiTransfer { | ||
// Early exit (optimization). | ||
return 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.
wouldn't it be easier/safer to return big.NewInt(0) here? (and L79, L84, L88)
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.
Reasoning behind the pull request
MultiESDTNFTTransfer
transactions in order to recover transfers of the native currency incurs a non-negligible performance cost.Proposed changes
TransferredValue
when adding the transaction in the mempool (just like we do forFee
). Use the information at selection time.SelectionSession.GetTransferredValue()
toMempoolHost.GetTransferredValue()
.shardedTxPool
class.Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
feat
branch created?feat
branch merging, do all satellite projects have a proper tag insidego.mod
?