Cleanup rpc #10910
Cleanup rpc #10910
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10910 +/- ##
=======================================
Coverage 81.9% 81.9%
=======================================
Files 310 310
Lines 71844 71829 -15
=======================================
- Hits 58903 58899 -4
+ Misses 12941 12930 -11 |
| } | ||
| } | ||
| }; | ||
| r_bank_forks.get(slot).cloned().unwrap() |
There was a problem hiding this comment.
@garious , looks to me like this will panic when a cluster root doesn't exist on a node, instead of returning the rpc error 😬
There was a problem hiding this comment.
So there was no test for this and my best guess was that the genesis block was the default cluster root. Is it not? Should it be?
There was a problem hiding this comment.
Technically, slot 0 is the default cluster root when the largest cluster root is unknown, yes. But the genesis Bank isn't retained in BankForks forever. Should it be?
This case also comes up when a node knows what the largest cluster root is, but that slot does not exist on that node. We've seen this come up on restarts (#10061) but there is a workaround in place. Right now, I think this case would only happen if a node is way off on its own fork, so arguably something is going wrong with the node or consensus, but I don't think panicking on an rpc request is the right behavior.
There was a problem hiding this comment.
A panic is only appropriate if it indicates a bug in the code. In this case, I think it does. It sounds like BankForks should always retain a largest cluster root. If there's a problem on restarts, I wonder, what information is missing from the snapshot, such that BankForks isn't restored?
Problem
Can't share code between jsonrpc_core and tarpc services. Most JsonRpcRequestProcessor methods return a jsonrpc_core-specific Result even though their only error cases are unreachable.
Summary of Changes
Assume the genesis block is the default largest confirmed root. Remove all the Result wrappers. There were no tests for those unreachable branches, so no test changes.
Still not sharing code between jsonrpc_core and tarpc services, but one step closer.