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

Improving remasc blocks retrieval; removing unused BlockStore method #1047

Merged
merged 1 commit into from
Nov 6, 2019

Conversation

ajlopezrsk
Copy link
Contributor

Remasc precompiled contract usually retrieved 4000 blocks in mainchain, each time. Now, the retrieve of the execution block (current block - maturity aprox) was replaced by an algorithm using in other uses cases. The old method used was removed.

lsebrie
lsebrie previously approved these changes Oct 28, 2019
Copy link
Contributor

@lsebrie lsebrie left a comment

Choose a reason for hiding this comment

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

LGTM only a minor comment

Block currentBlock = blockStore.getBlockByHashAndDepth(
executionBlock.getParentHash().getBytes(),
remascConstants.getMaturity() - 1 - uncleGenerationLimit
// this search is noe optimized if have certainty that the execution block is not in a fork
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change on the comment intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the algorithm supports the existence of a fork, but it could be degraded to the original algorithm performance if the fork involves > blocks than the remasc maturity parameter

Block currentBlock = blockStore.getBlockByHashAndDepth(
executionBlock.getParentHash().getBytes(),
remascConstants.getMaturity() - 1 - uncleGenerationLimit
// this search is noe optimized if have certainty that the execution block is not in a fork

Choose a reason for hiding this comment

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

Minor typo: noe should be replaced by not

@diegomasini
Copy link

diegomasini commented Nov 5, 2019

LGTM! I only found a minor typo, @ajlopez please fix it and I will approve the PR.

@lsebrie lsebrie merged commit b1eea6e into master Nov 6, 2019
@lsebrie lsebrie deleted the remascimprove branch November 6, 2019 18:20
@aeidelman aeidelman added this to the Papyrus v2.0.0 milestone Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants