Bank::get_slot_history() is fallible#12598
Conversation
|
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @brooksprumo. |
| let slot_history = bank | ||
| .get_slot_history() | ||
| .expect("snapshot bank must have slot history"); |
There was a problem hiding this comment.
We're at startup here, and slot history must exist. So get_slot_history() must succeed.
If preferred, we could add an error case on VerifySlotDeltasError instead.
| let tower = Tower::restore(tower_storage, node_pubkey).and_then(|restored_tower| { | ||
| let root_bank = bank_forks.read().unwrap().root_bank(); | ||
| let slot_history = root_bank.get_slot_history(); | ||
| let slot_history = root_bank.get_slot_history().unwrap(); |
There was a problem hiding this comment.
Should this unwrap, or do something else?
There was a problem hiding this comment.
imagine if root banks were their own type and you didn't have to ask because we wouldn't have successfully constructed one without this account existing
at least expect(). somehow stuffing this condition into all of these error types seems more wrong than panicking the validator
There was a problem hiding this comment.
it's fine to expect here, the root advancing /snapshot happens via replay_stage so we don't have the same race
| let restored_tower = restored_tower.and_then(|tower| { | ||
| let root_bank = bank_forks.root_bank(); | ||
| let slot_history = root_bank.get_slot_history(); | ||
| let slot_history = root_bank.get_slot_history().unwrap(); |
There was a problem hiding this comment.
Same here. Should this unwrap, or do something else?
There was a problem hiding this comment.
Fine to unwrap/expect this code executes before the TVU (and replay stage) are started up, so the race is not present
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #12598 +/- ##
=========================================
- Coverage 83.4% 83.4% -0.1%
=========================================
Files 859 859
Lines 330742 330751 +9
=========================================
- Hits 276057 276016 -41
- Misses 54685 54735 +50 🚀 New features to boost your workflow:
|
| let tower = Tower::restore(tower_storage, node_pubkey).and_then(|restored_tower| { | ||
| let root_bank = bank_forks.read().unwrap().root_bank(); | ||
| let slot_history = root_bank.get_slot_history(); | ||
| let slot_history = root_bank.get_slot_history().unwrap(); |
There was a problem hiding this comment.
it's fine to expect here, the root advancing /snapshot happens via replay_stage so we don't have the same race
| let restored_tower = restored_tower.and_then(|tower| { | ||
| let root_bank = bank_forks.root_bank(); | ||
| let slot_history = root_bank.get_slot_history(); | ||
| let slot_history = root_bank.get_slot_history().unwrap(); |
There was a problem hiding this comment.
Fine to unwrap/expect this code executes before the TVU (and replay stage) are started up, so the race is not present
brooksprumo
force-pushed
the
get_slot_history
branch
from
c83bd08 to
fefb08c
Compare
|
|
|
Backports to release branches are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
(cherry picked from commit ddb592b) # Conflicts: # runtime/src/snapshot_bank_utils.rs
5 participants
Problem
Bank::get_slot_history()double unwraps, which causes a panic on error. Instead, let the caller decide if this error is fatal or not.Summary of Changes
Bank::get_slot_history()return an Option.unwrap()(this maintains the current behavior--happy to adjust!)