Fix: update microblock mining behavior #2981
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ccount, other changes
Codecov Report
@@ Coverage Diff @@
## develop #2981 +/- ##
=========================================
Coverage 82.65% 82.66%
=========================================
Files 242 242
Lines 195781 196143 +362
=========================================
+ Hits 161829 162136 +307
- Misses 33952 34007 +55
Continue to review full report at Codecov.
|
jcnelson
reviewed
Jan 31, 2022
| Error::BlockTooBigError => { | ||
| // done mining -- our execution budget is exceeded. | ||
| // Make the block from the transactions we did manage to get | ||
| test_debug!("Block budget exceeded on tx {}", &tx.txid()); |
There was a problem hiding this comment.
Can this test_debug! statement be merged with the ones in either if branch, so that there's just one debug line emitted here? Makes it easier to grep for this event in test output. Same below.
jcnelson
approved these changes
Jan 31, 2022
pavitthrap
changed the base branch from
feat/miner-structured-logging
to
develop
kantai
reviewed
Feb 7, 2022
kantai
approved these changes
Feb 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on #2975, I noticed some unexpected logic in the miner.
Here is a summary of changes in this PR:
mine_next_microblockto keep track ofblock_limit_hit. This variable toggles betweenNO_LIMIT_HIT,CONTRACT_LIMIT_HIT, andLIMIT_REACHED. Keeping track of this signals the microblock builder to stop processing transactions once the block limit is hit.mine_next_transaction(which is used bymine_next_microblock) to takeblock_limit_hitas an input. This allows the function to skip over appropriate transactions (for example whenblock_limit_hit=CONTRACT_LIMIT_HIT, contract call and smart contract payloads will be automatically skipped).build_anchored_blockso that it only gets executed if the transaction being considered was successfully mined. As it was before, these values were sometimes being updated even when the transaction processing failed.consideredset for the microblock mining flow. It was originally inmine_next_transaction, but I moved the logic out to the calling function because I thought it was (a) more readable, and (b) it now mirrors the logic for the anchored block mining flow.I need feedback on the following change:
block_limit_hitvariable moves onto its next state if a transaction errors with eitherBlockTooBigErrororTransactionTooBigError. I removed this transition for the latter case, since I felt that it didn't make sense to change the block's limit behavior for an invalid tx that consumes >80% of the block budget that we are anyway dropping from the mempool.Tests:
Ran existing tests.
Added 4 tests: