-
Notifications
You must be signed in to change notification settings - Fork 909
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
graphman: Check if its safe to rewind by comparing block to earliest block #5423
Conversation
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.
Looks good.
I think for extra safety, it would be good to also check within the store that we are not rewinding before the earliest block so that even if some other user of the rewind API doesn't check in the future, rewinds are safe.
There's two places where that would make sense:
- In
deployment::revert_block_ptr
we could add awhere block_ptr_to.num >= earliest_block + reorg_threshold
clause to the update statement and return an error if the update returns 0 (separate, but if we get anything but 1 from that update statement, that should be an error) - in
DeploymentStore::rewind_or_truncate_with_conn
we could check thatblock_ptr_to
is not too early
Of the two, the first one is the safest IMHO
…n earliest block and reorg threshold
Doing so broke the unit tests, since at many places we use the static var ENV_VAR to get the reorg threshold, This cannot be easily mocked without refactoring the code to take these env_vars as a field, Looking into it. Some tests call revert_ptr so it will fail with the new where clause since the reorg threshold is not overridable |
3c0615a
to
c968fc5
Compare
let reorg_threshold = | ||
inner | ||
.reorg_threshold | ||
.unwrap_or_else(|| if cfg!(debug_assertions) { 0 } else { 250 }); |
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.
Oh wow .. I didn't know you could use cfg!
as a normal macro .. nice
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.
One thought: will tests still fail if reorg_trheshold
is set to 1? It seems a little weird to use 0 here, but if that's what's needed to let tests pass, I am ok with it.
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.
Unfortunately no, there are some tests that fail when we se 1.
6cad4f6
to
c968fc5
Compare
Closes #5422