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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

incrypto32
Copy link
Member

@incrypto32 incrypto32 commented May 16, 2024

Closes #5422

@fordN fordN requested a review from lutter May 16, 2024 15:47
Copy link
Collaborator

@lutter lutter 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.

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 a where 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 that block_ptr_to is not too early

Of the two, the first one is the safest IMHO

node/src/manager/commands/rewind.rs Outdated Show resolved Hide resolved
@incrypto32
Copy link
Member Author

incrypto32 commented May 20, 2024

  • In deployment::revert_block_ptr we could add a where 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)

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

@incrypto32 incrypto32 self-assigned this May 20, 2024
let reorg_threshold =
inner
.reorg_threshold
.unwrap_or_else(|| if cfg!(debug_assertions) { 0 } else { 250 });
Copy link
Collaborator

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

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more checks for rewinds for subgraphs that have ongoing pruning enabled
2 participants