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

Revert "Make RPC server functions that read db thread safe" #8384

Merged
merged 1 commit into from
Jul 3, 2022

Conversation

j-berman
Copy link
Collaborator

@j-berman j-berman commented Jun 13, 2022

Reverts PR #7937. Related to #8383.

Incorrect sequence that PR #7937 introduces:

  1. db write to add block to chain begins
  2. notification that block added to chain is pushed out to consumer
  3. consumer makes request to RPC API expecting latest chain state in response
  4. RPC server grabs db read txn guard [PR 7937]
  5. db write from step 1 completes
  6. consumer of RPC API receives consistent but stale view of the chain from before the most recent block was added thanks to the db read txn guard from step 4 [PR 7937]

This PR removes db read txn guards added in #7937 to the RPC server to avoid the above problem, accepting that the RPC API can still possibly return inconsistent data as the lesser of 2 evils (which is current behavior).

A stronger long term fix to ensure consistent chain state reads for RPC functions that will still return the latest data is preferred, with care to ensure a consistently ordered API across all sources (RPC / ZMQ / notifications etc.). See one such proposed solution here. (edited to clear up imprecise explanation)

This reverts commit 50410d1, reversing
changes made to d054def.
Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

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

Correctly reverts PR 7937

Copy link
Contributor

@jeffro256 jeffro256 left a comment

Choose a reason for hiding this comment

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

Correctly reverts #7937, and I think the old behavior is less bad than current behavior

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.

4 participants