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
Hardfork: execute pending txs in correct order #2471
Hardfork: execute pending txs in correct order #2471
Conversation
SebastianMarian
commented
Nov 13, 2020
- Implemented a fix in Hardfork mechanism, which would execute all pending transactions in the correct order
- Exported/Imported unFinished metaBlocks as well, as they are needed for execution of pending transactions in the correct order
- Extracted common used code in a new file
- Refactored some prints and some methods names
…nding transactions in the correct order * Exported/Imported unFinished metaBlocks as well, as they are needed for execution of pending transactions in the correct order * Extracted common used code in a new file * Refactored some prints and some methods names
update/common.go
Outdated
} | ||
|
||
// CreateNonceToHashMap creates a map of nonce to hash from all the given metaBlocks | ||
func CreateNonceToHashMap(unFinishedMetaBlocks map[string]*block.MetaBlock) map[uint64]string { |
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.
do we need all these functions exported?
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.
No, only GetPendingMiniBlocks, I will make them unexported
} | ||
|
||
for _, mbHdr := range metaBlock.MiniBlockHeaders { | ||
if mbHdr.ReceiverShardID == destShardID && mbHdr.SenderShardID != destShardID { |
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.
isForMe := (mbHdr.ReceiverShardID == destShardID || mbHdr.ReceiverShardID == core.AllShardId) && mbHdr.SenderShardID != destShardID
as we have to take into account all shard ID value
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.
Actually core.AllShardId refers to PeerBlock right now, and PendingTransactionProcessor does not know to process this type of miniblock. More than this, in the commented code above from line 49 this type of miniblock was also avoided in the original code. Actually, the whole file is just a copy-paste of the original one with slightly renaming here and there. We should discuss/analyze if this type of miniblock which could appear only in the start of epoch metablock (hardfork meta block) should be executed or not in the import phase or it would be executed in a normal way afterwards.
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.
I know, more mental mapping to have as to not use core.AllShardId on miniblocks other than peer miniblocks because there are some edgecases.
func (p *pendingProcessor) ProcessTransactionsDstMe(mapTxs map[string]data.TransactionHandler) (block.MiniBlockSlice, error) { | ||
sortedTxs := getSortedSliceFromTxsMap(mapTxs) | ||
|
||
func (p *pendingProcessor) ProcessTransactionsDstMe(txsInfo []*update.TxInfo) (block.MiniBlockSlice, error) { |
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.
👍
update/process/shardBlock.go
Outdated
return nil, update.ErrTransactionNotFoundInImportedMap | ||
} | ||
|
||
txsInfo = append(txsInfo, &update.TxInfo{TxHash: txHash, Tx: tx}) |
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.
multiline?
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.
Done
update/process/shardBlock.go
Outdated
@@ -161,6 +164,45 @@ func (s *shardBlockCreator) createBody() (*block.Body, error) { | |||
}, nil | |||
} | |||
|
|||
func (s *shardBlockCreator) getPendingTxsInCorrectOrder() ([]*update.TxInfo, error) { |
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.
I think we might go one miniblock at a time - in order to protect against out of memory issues
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.
Are you refering to OOM in this method or afterwards when we call with the []*update.TxInfo result the method ProcessTransactionsDstMe ?
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.
OOM in for all when you have millions of transactions.
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.
System test passed