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.

Sign up for GitHub

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

Jump to bottom

Move the consensus related logic from chainstate to a separate crate #351

Merged
merged 3 commits into from
Aug 15, 2022
Merged

Move the consensus related logic from chainstate to a separate crate #351

merged 3 commits into from
Aug 15, 2022

Conversation

stanislav-tkach
Copy link
Contributor

There should be no changes in the logic, only moving some modules and just a bit of refactoring.

stanislav-tkach
stanislav-tkach commented Aug 8, 2022
View reviewed changes
chainstate-types/src/error.rs Show resolved Hide resolved
chainstate-types/src/lib.rs Show resolved Hide resolved
common/src/chain/block/block_reward.rs Show resolved Hide resolved
@stanislav-tkach stanislav-tkach mentioned this pull request Aug 8, 2022
Merged
iljakuklic
iljakuklic approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

Looks good

azarovh
azarovh approved these changes Aug 9, 2022
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@stanislav-tkach
Copy link
Contributor Author

I'm still not sure that moving out PropertyQueryError to the chainstate_types crate is a bad idea. This error is contained in the public ChainstateError type. In the new consensus crate it is also used as part of the ConsensusVerificationError:PrevBlockLoadError error.

Additionally, the implementations of the TransactionIndexHandle and BlockIndexHandle traits heavily rely on chainstate methods that return PropertyQueryError.

@TheQuantumPhysicist
Copy link
Contributor

Looks good. Please fix the PoS branch after merging this to master because it has pos dir beside pow, which is now moved.

@stanislav-tkach stanislav-tkach merged commit 71c0929 into master Aug 15, 2022
@stanislav-tkach stanislav-tkach deleted the move-consensus-from-chainstate-to-separate-crate branch August 15, 2022 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iljakuklic iljakuklic iljakuklic approved these changes

@muursh muursh muursh approved these changes

@TheQuantumPhysicist TheQuantumPhysicist TheQuantumPhysicist approved these changes

@azarovh azarovh azarovh approved these changes

@erubboli erubboli Awaiting requested review from erubboli erubboli is a code owner

@altonen altonen Awaiting requested review from altonen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stanislav-tkach @TheQuantumPhysicist @iljakuklic @muursh @azarovh