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

Bugfix/added gasLimit check when creating cross shard miniBlocks #350

Conversation

sasurobert
Copy link
Contributor

Added gasLimit check when creating a cross shard miniblock. Example of the problem: shard 0 creates a miniblock with 1000 smart contract calls, all the calls are for a SC in shard 1. shard 1 when reading the from metachain the needed miniblocks to process, tries to do it, but fails as he has no time to process all those transactions.

process/block/preprocess/transactions.go Show resolved Hide resolved
process/block/preprocess/transactions.go Outdated Show resolved Hide resolved
process/block/preprocess/transactions_test.go Show resolved Hide resolved
integrationTests/testInitializer.go Outdated Show resolved Hide resolved
round = IncrementAndPrintRound(round)

for _, idProposer := range idxProposers {
proposedNode := nodes[idProposer]
Copy link
Contributor

Choose a reason for hiding this comment

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

proposerNode?

as no one is proposing a node for anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

startTime := time.Now()
ProposeBlock(nodes, idxProposers, round)
elapsedTime := time.Since(startTime)
fmt.Printf("Block Created in %s\n", elapsedTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this block propose & sync is repeated below as well, can we extract this in a function?

Also why is this done in a loop here and also in another loop below?
It is not clear why this is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the loop below is needed, to propagate all the modifications through all the shards. After the last transactions were proposed, it needs 5 rounds to reach the destination shard, because of finality and asynchronous processing through metachain.

Copy link
Contributor

Choose a reason for hiding this comment

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

then add a comment for the second loop why it is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

) uint64 {

// propose until pool is cleared
for i := numInTxs; i != 0; {
Copy link
Contributor

Choose a reason for hiding this comment

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

these for loops are confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is not repeat until, or do while. So the propose is needed to be done at least once. Than to continue proposing until the pools are empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comment

SyncBlock(t, nodes, idxProposers, round)
round = IncrementAndPrintRound(round)

for _, idProposer := range idxProposers {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this trying to do?

From what I see you are selecting the first node with no transaction with its shard as destination(or a nil tx pool), from a consecutive list of proposing nodes.

You are also modifying the i here which is used in the above for loop, so you are exiting both for loops when you have found one proposing node with no transactions. Is this a waiting loop until one proposer is found that has no remaining txs for his shard in pool? can you extract in a function and make it clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a function to continue proposing blocks until there are no more transactions in the pool.

process/block/preprocess/transactions.go Outdated Show resolved Hide resolved
process/block/preprocess/transactions.go Show resolved Hide resolved
process/block/preprocess/transactions_test.go Outdated Show resolved Hide resolved
…ntract-calls-in-the-same-miniblock' into bugfix/en-3496-too-many-smart-contract-calls-in-the-same-miniblock
@iulianpascalau iulianpascalau added the type:feature New feature or request label Aug 11, 2019
Copy link
Contributor

@raduchis raduchis left a comment

Choose a reason for hiding this comment

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

Should you be able to have gasLimit more the the maxGasLimitPerMiniblock?

raduchis
raduchis previously approved these changes Aug 12, 2019
@sasurobert sasurobert merged commit 0ec30d9 into development Aug 12, 2019
@sasurobert sasurobert deleted the bugfix/en-3496-too-many-smart-contract-calls-in-the-same-miniblock branch August 12, 2019 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants