-
Notifications
You must be signed in to change notification settings - Fork 5
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
STR-949 chainstate manager v2 and minor chainstate restructures #633
base: main
Are you sure you want to change the base?
Conversation
crates/primitives/src/epoch.rs
Outdated
/// Checks if the epoch is sane. | ||
/// | ||
/// Ie. if the terminal blkid is zero then the last slot and epoch number | ||
/// are also zero. | ||
fn sanity_check(&self) -> bool { | ||
if self.is_null() { | ||
self.last_slot == 0 && self.epoch == 0 | ||
} else { | ||
self.last_slot != 0 | ||
} | ||
} | ||
} |
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.
Why not include this check in the constructor?
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.
Maybe I should just get rid of the sanity check altogether. This special casing isn't well-defined at the moment since it's not actually used in this context.
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.
Would it work to avoid the sentinel value and use an Option
instead? Or does that screw up the commitment call...
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'd be nice to always have a well-defined finalized block so that we don't have to do special casing just for the genesis. Maybe we can rework the interfaces/definitions around this.
Really happy with the general refactoring I did to the CSM logic, this will hopefully make it simpler to make work when rebasing the changes in the other branch. |
Still have to simplify the actual db impls to remove the now-unnecessary chainstate tables/fields. |
Description
This is a second attempt at doing the chainstate manager now with some better idea of the context, plus some additional types to support work in the other PR. I expect some more things will be copied over that are related and minimally invasive.
Type of Change
Notes to Reviewers
Checklist
Related Issues