Research/parallel-tx/Refactor validblock #1794
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
If edges are in order, last edge decides if any was out of bounds
First changed to prev = -1, then added test that failed, then set back prev = 0
javicid
reviewed
Jun 7, 2022
|
|
||
| if (edges == null) { | ||
| if (edges == null || edges.length == 0) { |
There was a problem hiding this comment.
Why remove the second check?
There was a problem hiding this comment.
What did I remove? 🤔 If length is zero why would we iterate and all that.
Constants.getMaxTransactionExecutionThreads() must be >1
rskj-core/src/main/java/co/rsk/validators/ValidTxExecutionListsEdgesRule.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/co/rsk/validators/ValidTxExecutionListsEdgesRule.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/co/rsk/validators/ValidTxExecutionListsEdgesRule.java
Outdated
Show resolved
Hide resolved
rskj-core/src/main/java/co/rsk/validators/ValidTxExecutionListsEdgesRule.java
Outdated
Show resolved
Hide resolved
ilanolkies
changed the title
ilanolkies
commented
Jun 27, 2022
| } | ||
|
|
||
| if (edges[edges.length-1] > block.getTransactionsList().size()) { |
There was a problem hiding this comment.
Suggested change
| if (edges[edges.length-1] > block.getTransactionsList().size()) { | |
| if (edges[edges.length-1] > block.getTransactionsList().size() - 1) { |
This would keep the REMASC tx, that is the last tx, outside of the parallel buckets
* store and return a copy of txEdges * merged two ifs in the BlockHeaderBuilder * DummyTracker updated so it returns a copy of the maps * Unused hashmap import deleted from ProgramTraceProcessor * BlockResult returns and stores a copy of txEdges * big refactor in BlockExecutor * Refactor ParallelizeTransactionHandler * renaming Bucket to Sublist * renaming txExecutionListsEdges to txExecutionSublistEdges * simplification of Handler's code * InterruptedException solved properly in the BlockExecutor
* Avoid tracking write for add balance 0 * Avoid tracking when writing the same value * Rename assertion * Remove condition for add balance zero It is considered in "write the same value" * Revert breaks
…tx-refactor-validblock
julianlen
force-pushed
the
research/main_parallel_tx
branch
4 times, most recently
from
63a5857
to
4b204f5
Compare
8 tasks
…ect index (#1822) * Fix isRemascTransaction validation and pass correct index * Remove -1 * Rename local data * Add tests for isRemascTransaction * Add tests for remasc transaction * Fix txindex Added test for block with rejected tx * Add missing txindex++
…tx-refactor-validblock
|
Kudos, SonarCloud Quality Gate passed! |
javicid
approved these changes
Jul 19, 2022
julianlen
added a commit
that referenced
this pull request
Sep 26, 2022
* Refactor repeated code * Refactor repeated edges mocking * Modify test for too many edges - it was also hitting out of bounds * Refactor out of bounds validation If edges are in order, last edge decides if any was out of bounds * Rename test * Add case where first edge is zero First changed to prev = -1, then added test that failed, then set back prev = 0 * Refactor order verification * Research/refactoring code (#1817) * store and return a copy of txEdges * merged two ifs in the BlockHeaderBuilder * DummyTracker updated so it returns a copy of the maps * Unused hashmap import deleted from ProgramTraceProcessor * BlockResult returns and stores a copy of txEdges * big refactor in BlockExecutor * Refactor ParallelizeTransactionHandler * renaming Bucket to Sublist * renaming txExecutionListsEdges to txExecutionSublistEdges * simplification of Handler's code * InterruptedException solved properly in the BlockExecutor * Research/parallel-tx/optimize writes (#1811) * Avoid tracking write for add balance 0 * Avoid tracking when writing the same value * Rename assertion * Remove condition for add balance zero It is considered in "write the same value" * Revert breaks * Add test refactor back * Make remasc transaction not be in a parallelized sublist * Research/parallel-tx/Fix isRemascTransaction validation and pass correct index (#1822) * Fix isRemascTransaction validation and pass correct index * Remove -1 * Rename local data * Add tests for isRemascTransaction * Add tests for remasc transaction * Fix txindex Added test for block with rejected tx * Add missing txindex++ * Fix merge error Co-authored-by: julianlen <[email protected]>
julianlen
added a commit
that referenced
this pull request
Nov 16, 2022
* Refactor repeated code * Refactor repeated edges mocking * Modify test for too many edges - it was also hitting out of bounds * Refactor out of bounds validation If edges are in order, last edge decides if any was out of bounds * Rename test * Add case where first edge is zero First changed to prev = -1, then added test that failed, then set back prev = 0 * Refactor order verification * Research/refactoring code (#1817) * store and return a copy of txEdges * merged two ifs in the BlockHeaderBuilder * DummyTracker updated so it returns a copy of the maps * Unused hashmap import deleted from ProgramTraceProcessor * BlockResult returns and stores a copy of txEdges * big refactor in BlockExecutor * Refactor ParallelizeTransactionHandler * renaming Bucket to Sublist * renaming txExecutionListsEdges to txExecutionSublistEdges * simplification of Handler's code * InterruptedException solved properly in the BlockExecutor * Research/parallel-tx/optimize writes (#1811) * Avoid tracking write for add balance 0 * Avoid tracking when writing the same value * Rename assertion * Remove condition for add balance zero It is considered in "write the same value" * Revert breaks * Add test refactor back * Make remasc transaction not be in a parallelized sublist * Research/parallel-tx/Fix isRemascTransaction validation and pass correct index (#1822) * Fix isRemascTransaction validation and pass correct index * Remove -1 * Rename local data * Add tests for isRemascTransaction * Add tests for remasc transaction * Fix txindex Added test for block with rejected tx * Add missing txindex++ * Fix merge error Co-authored-by: julianlen <[email protected]>
julianlen
added a commit
that referenced
this pull request
Nov 28, 2022
* Refactor repeated code * Refactor repeated edges mocking * Modify test for too many edges - it was also hitting out of bounds * Refactor out of bounds validation If edges are in order, last edge decides if any was out of bounds * Rename test * Add case where first edge is zero First changed to prev = -1, then added test that failed, then set back prev = 0 * Refactor order verification * Research/refactoring code (#1817) * store and return a copy of txEdges * merged two ifs in the BlockHeaderBuilder * DummyTracker updated so it returns a copy of the maps * Unused hashmap import deleted from ProgramTraceProcessor * BlockResult returns and stores a copy of txEdges * big refactor in BlockExecutor * Refactor ParallelizeTransactionHandler * renaming Bucket to Sublist * renaming txExecutionListsEdges to txExecutionSublistEdges * simplification of Handler's code * InterruptedException solved properly in the BlockExecutor * Research/parallel-tx/optimize writes (#1811) * Avoid tracking write for add balance 0 * Avoid tracking when writing the same value * Rename assertion * Remove condition for add balance zero It is considered in "write the same value" * Revert breaks * Add test refactor back * Make remasc transaction not be in a parallelized sublist * Research/parallel-tx/Fix isRemascTransaction validation and pass correct index (#1822) * Fix isRemascTransaction validation and pass correct index * Remove -1 * Rename local data * Add tests for isRemascTransaction * Add tests for remasc transaction * Fix txindex Added test for block with rejected tx * Add missing txindex++ * Fix merge error Co-authored-by: julianlen <[email protected]>
Vovchyk
pushed a commit
that referenced
this pull request
Feb 2, 2023
* Refactor repeated code * Refactor repeated edges mocking * Modify test for too many edges - it was also hitting out of bounds * Refactor out of bounds validation If edges are in order, last edge decides if any was out of bounds * Rename test * Add case where first edge is zero First changed to prev = -1, then added test that failed, then set back prev = 0 * Refactor order verification * Research/refactoring code (#1817) * store and return a copy of txEdges * merged two ifs in the BlockHeaderBuilder * DummyTracker updated so it returns a copy of the maps * Unused hashmap import deleted from ProgramTraceProcessor * BlockResult returns and stores a copy of txEdges * big refactor in BlockExecutor * Refactor ParallelizeTransactionHandler * renaming Bucket to Sublist * renaming txExecutionListsEdges to txExecutionSublistEdges * simplification of Handler's code * InterruptedException solved properly in the BlockExecutor * Research/parallel-tx/optimize writes (#1811) * Avoid tracking write for add balance 0 * Avoid tracking when writing the same value * Rename assertion * Remove condition for add balance zero It is considered in "write the same value" * Revert breaks * Add test refactor back * Make remasc transaction not be in a parallelized sublist * Research/parallel-tx/Fix isRemascTransaction validation and pass correct index (#1822) * Fix isRemascTransaction validation and pass correct index * Remove -1 * Rename local data * Add tests for isRemascTransaction * Add tests for remasc transaction * Fix txindex Added test for block with rejected tx * Add missing txindex++ * Fix merge error Co-authored-by: julianlen <[email protected]>
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.
Refactored tests and a little bit of the implementation. Also corrected one test that hit two cases at the same time, now hits one of the two.
Things we need to further look at:
edges. If it is validating edges it means it is already marked as post-RSKIP144, so we should asume edges existedges[edges.length-1]be<= txListSize-1? Thus validating the last edge does not include REMASC that is the last txAnother additional refactor that we could consider:
Constants.getMaxTransactionExecutionThreads()incorporates an external dependency toValidTxExecutionListsEdgesRule. I would consider having that value set in the constructor to prevent side effects on future changes