Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Return root when bank not found #11188

Merged
merged 2 commits into from
Jul 24, 2020
Merged

Return root when bank not found #11188

merged 2 commits into from
Jul 24, 2020

Conversation

garious
Copy link
Contributor

@garious garious commented Jul 23, 2020

Problem

The bank at a given CommitmentLevel may not be in BankForks. See #11078

Summary of Changes

  • Band-aid: Return the root bank instead of panicking
  • Add a comment explaining how to remove the band-aid

@carllin
Copy link
Contributor

carllin commented Jul 23, 2020

If we wanted to be extra careful, we could also disable the Commitment::Max until the "cluster confirmed root" queries are actually supported. If not, for that commitment level at least (just b/c of the implications) it feels safer to return a failure rather than a local root, as critical clients that are expecting Commitment::Max getting a local root are getting confidence than they asked for.

Local root and cluster confirmed root should only diverge if there's some critical bug/attack that causes a few validators to commit down the wrong path. It hasn't happened to date, but just being extra careful here.

Maybe @mvines can illuminate if there are further implications for restart any current partners using this call?

@mvines
Copy link
Contributor

mvines commented Jul 23, 2020

Maybe @mvines can illuminate if there are further implications for restart any current partners using this call?

Turning "max" into "root" for a time will be transparent to downstream users AFAICT. We made the distinction unilaterally to improve the safely of the results that "max" returns, not due to any partner request

@carllin
Copy link
Contributor

carllin commented Jul 23, 2020

Maybe @mvines can illuminate if there are further implications for restart any current partners using this call?

Turning "max" into "root" for a time will be transparent to downstream users AFAICT. We made the distinction unilaterally to improve the safely of the results that "max" returns, not due to any partner request

Awesome, thanks!

carllin
carllin previously approved these changes Jul 23, 2020
@codecov
Copy link

codecov bot commented Jul 23, 2020

Codecov Report

Merging #11188 into master will increase coverage by 0.0%.
The diff coverage is 16.6%.

@@           Coverage Diff           @@
##           master   #11188   +/-   ##
=======================================
  Coverage    82.1%    82.1%           
=======================================
  Files         318      318           
  Lines       74002    74007    +5     
=======================================
+ Hits        60804    60816   +12     
+ Misses      13198    13191    -7     

CriesofCarrots
CriesofCarrots previously approved these changes Jul 23, 2020
core/src/rpc.rs Outdated Show resolved Hide resolved
@garious garious added the automerge Merge this Pull Request automatically once CI passes label Jul 23, 2020
@mergify mergify bot dismissed stale reviews from carllin and CriesofCarrots July 23, 2020 23:47

Pull request has been modified.

@mergify mergify bot merged commit 7484202 into solana-labs:master Jul 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants