-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Roll CoinStore
, BlockStore
and BlockHeightMap
into ConsensusStore
#19949
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
Open
richardkiss
wants to merge
30
commits into
Chia-Network:main
Choose a base branch
from
richardkiss:consensus_store
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.
Open
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
0da0073
`ConsensusStore`
richardkiss e853f48
ConsensusStoreSQLite3
richardkiss 5cd7fb9
fix tests
richardkiss 30a2e63
more tests
richardkiss b91960c
Move `coin_store_protocol.py` to `chia/full_node`
richardkiss b238338
tach
richardkiss 4abbe5c
first crack at async context manager for ConsensusStore
richardkiss 6706d6a
Handle nested context manager
richardkiss 4f19c4c
More protocol
richardkiss 9f1447b
more
richardkiss fda5a08
mypy
richardkiss 081ba0e
mypy
richardkiss f742272
ruff
richardkiss 00f76ce
simplify
richardkiss 15f0839
simplify
richardkiss 65642be
dataclass
richardkiss e1c728e
CoinStoreProtocol
richardkiss 034018b
shrink diff
richardkiss b7499a8
Undo test changes and silly api
richardkiss 246f3c6
Defer to `DBWrapper2`
richardkiss 24ad6d4
coverage
richardkiss 9f4f99b
Factor `_load_chain_from_store` into pieces
richardkiss eafd0c4
Don't use double underscore.
richardkiss c285715
simplify diffs
richardkiss f617564
minimize diffs
richardkiss a6c9748
more minimize diffs
richardkiss 5e394bb
Remove comments
richardkiss fd1edd5
Self
richardkiss 4f7e799
final
richardkiss acfd7d5
Remove `ConsensusStoreWriterSQLite3.writer` method
richardkiss 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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.
this function checks the invariant of the sqlite database. I don't think it makes sense to make it generic to check the invariant of any block store. The right hand, generic, version looks a bit odd.
get_block_heights_in_main_chain()
sounds like it would returnrange(0, peak.height)
.That's not really what this invariant check is ensuring. I think you should leave this function as-is, but only operate on the sqlite block store. The future RocksDB block store will most likely have different invariants to check
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.
It seems like a very specific invariant, one that should probably be true for any block store that supports
in_main_chain
(and I'm actually quite skeptical about the usefulness of keeping and constantly updating this flag, since it's a bit redundant with the in-RAMBlockHeightMap
). I'm fine returning this test to its previous state and removing the rather silly API I created.The test will have to check that
bc.block_store
actually exists (and when it's eventually replaced withBlockArchiveStore
, have dig down to get the implementation details).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.
This test is now much closer to its original form.
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.
The in RAM map is not sustainable and will need to be removed eventually
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 do think the api to
ConsensusStore
seems unreasonably large, with a lot of redundancy. It could be an interesting future project to simplify the api; we could, at the same time, remove the annoying non-async methods thatBlockHeightMap
currently provides, and that caching moved to eitherFullNode
orConsensusStore
.