graphman: Check if its safe to rewind by comparing block to earliest block #5423
Conversation
There was a problem hiding this comment.
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_ptrwe could add awhere block_ptr_to.num >= earliest_block + reorg_thresholdclause 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_connwe could check thatblock_ptr_tois 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.
Oh wow .. I didn't know you could use cfg! as a normal macro .. nice
There was a problem hiding this comment.
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.
Unfortunately no, there are some tests that fail when we se 1.
6cad4f6 to
c968fc5
Compare
Closes #5422