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

rpc: don't lock consensus state #3161

Closed
ebuchman opened this issue Jan 19, 2019 · 1 comment · Fixed by #4844
Closed

rpc: don't lock consensus state #3161

ebuchman opened this issue Jan 19, 2019 · 1 comment · Fixed by #4844
Assignees
Labels
C:consensus Component: Consensus C:rpc Component: JSON RPC, gRPC good first issue Contributions Welcome!!

Comments

@ebuchman
Copy link
Contributor

ebuchman commented Jan 19, 2019

RPC calls which grab the ConsensusState may deadlock if the consensus is halted (#1772) and will deadlock if called from an ABCI app during execution (#3148 (comment)).

This includes: /validators, /consensus_params, /status, /dump_consensus_state, /consensus_state,

While HTTP calls from ABCI apps during execution isn't generally encouraged (due to non-determinism), in many cases it's to avoid redundant storage of information (like historical validator sets) in the application. This data is deterministic itself, and if Tendermint can't deterministically respond, there's probably a critical failure in the node anyways. Unless or until we support queries from the App to Tendermint over ABCI directly, it seems beneficial to enable ABCI apps to access this information.

For /validators, /consensus_params, /status, we only need the latest height, so we can easily use blockStore.Height() + 1 instead of locking the consensusState to make them available to applications.

The *consensus_state methods are non-deterministic, so they're off limits to applications in block execution. We could consider making them independent of the main consensus lock by memoizing the relevant JSON every time there's a change to the consensus state, but that's probably not worth it at this point.

Thus, we should:

  • Specify in the RPC and ABCI docs which RPC methods are safe during block execution
  • Make /validators, /consensus_params, /status safe during block execution

Replaces #3151

@cmwaters
Copy link
Contributor

cmwaters commented Mar 5, 2020

What's the current status for this issue? I see that #1772 immunised /status to consensus halts. What about /validators and /consensus_params

@melekes melekes self-assigned this May 12, 2020
melekes added a commit that referenced this issue May 15, 2020
in /validators, /consensus_params and /status

Closes #3161
@mergify mergify bot closed this as completed in #4844 May 15, 2020
mergify bot pushed a commit that referenced this issue May 15, 2020
in /validators, /consensus_params and /status

Closes #3161
tessr pushed a commit that referenced this issue May 20, 2020
in /validators, /consensus_params and /status

Closes #3161
melekes added a commit that referenced this issue May 27, 2020
in /validators, /consensus_params and /status

Closes #3161
melekes added a commit that referenced this issue May 27, 2020
in /validators, /consensus_params and /status

Closes #3161
tessr pushed a commit that referenced this issue May 28, 2020
in /validators, /consensus_params and /status

Closes #3161
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:consensus Component: Consensus C:rpc Component: JSON RPC, gRPC good first issue Contributions Welcome!!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants