-
Notifications
You must be signed in to change notification settings - Fork 2k
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
refactor get block generator #16335
Draft
almogdepaz
wants to merge
19
commits into
main
Choose a base branch
from
refactor_get_block_generator
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
refactor get block generator #16335
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
eeb8cbd
propegate forkpoint, reorder checks, fix fork typing allover
almogdepaz 72ded77
only read from disc in one place
almogdepaz a0425ff
remove leftover code
almogdepaz 9791d33
lint, fix test to use same genesis
almogdepaz 1544069
fix basic_chain test to use same genesis
almogdepaz 163564e
get all refs from db in one go
almogdepaz 489bb26
fix test_wallet_sync
almogdepaz 97c70ad
revert db optimization
almogdepaz 4b438a4
Merge branch 'main' into refactor_get_block_generator
almogdepaz e911183
get correct blocks for generators
almogdepaz b5e9ebf
pass fork point in short batch syncing
almogdepaz 777ccce
Merge branch 'main' into refactor_get_block_generator
almogdepaz f5d57d5
pass fork point in short batch syncing
almogdepaz c5682d5
revert fork point type change
almogdepaz 73ab429
remove redundant imports
almogdepaz fffc28f
fix redundant type conversion
almogdepaz a1c62a7
use additional blocks dict
almogdepaz 07cbdfe
pass reorg blocks to get_block_generator
almogdepaz c99c512
fix forkpoint in add block
almogdepaz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
we still need, at least, the height-to-hash mapping that
reorg_chain
provides. I don't think you can remove this whole seciontThere 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.
ok think i get what wrong here.
we traverse all the heights in the end anyway and fetch the missing blocks from db but we do that by height so might get the wrong one, so i just need the mapping to get the hashes this code still redundant, we dont need all the full blocks up to the fork we only need the referenced ones
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 what confused me is the if statement, we assume curr would be in the current chain ?
after that i get that we would get the blocks from the reorg_chain but why would we even get into the if, we dont expect block_records to contain blocks from the reorg
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.
by
block_records
, you mean theBlockchain
object, right? Theself
. It has access to all blocks, including orphaned blocks and other forks. If we're in a reorg, those blocks are in theBlockchain
object as well.I think the intention of that if-statement,
if self.contains_block(curr.prev_header_hash)
was to ask if we know of that block, not if we happen to have it in the cache right now. If we assume the intention was the latter, there's another block of code missing here to do the same thing but pull the block hashes from the database.the
additional_blocks
is just the current batch of block's we're validating. It does not contain the whole fork. The fork is still only stored in the database at this point, that's where we need to get the generators from.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.
then i dont get the point of this check, im pretty sure the initial intention was to see if its in the chain
otherwise we are just checking the store to contain something that we expect to be there, in that case i would have expected an assert or some exception being thrown, what is the case then when the block not in blockchain.blockrecords ?