-
Notifications
You must be signed in to change notification settings - Fork 217
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] Fatal getSignaturesForAddress()
when Bigtable errors
#3700
base: master
Are you sure you want to change the base?
[rpc] Fatal getSignaturesForAddress()
when Bigtable errors
#3700
Conversation
getSignaturesForAddress()
when Bigtable errors
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @steveluscher. |
How would I go about testing/mocking a Bigtable failure. I wanted to at least write something like this: #[test]
fn test_signatures_for_address_blockstore_query_failure() {
let rpc = RpcHandler::start_with_config(JsonRpcConfig {
enable_rpc_transaction_history: true,
rpc_bigtable_config: # ???
..Default::default()
});
let pubkey = Pubkey::new_unique();
// This address is guaranteed to have no signatures, thereby forcing
// `getSignaturesForAddress` to go looking in long-term storage.
let address = pubkey.to_string();
let request = create_test_request(
"getSignaturesForAddress",
Some(json!([address, {"limit": 10}])),
);
let (code, _message) = parse_failure_response(rpc.handle_request_sync(request));
assert_eq!(code, JSON_RPC_SERVER_ERROR_LONG_TERM_STORAGE_UNREACHABLE);
} …but I can't figure out how to supply ‘a bigtable instance that will always fail.’ |
…NotFound` instead of `RowNotFound`
…ut can't be reached
if !self.config.enable_rpc_transaction_history { | ||
return Err(RpcCustomError::TransactionHistoryNotAvailable.into()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the meat of the unindenting. All of the code below is unchanged.
@@ -26,6 +26,7 @@ pub const JSON_RPC_SERVER_ERROR_UNSUPPORTED_TRANSACTION_VERSION: i64 = -32015; | |||
pub const JSON_RPC_SERVER_ERROR_MIN_CONTEXT_SLOT_NOT_REACHED: i64 = -32016; | |||
pub const JSON_RPC_SERVER_ERROR_EPOCH_REWARDS_PERIOD_ACTIVE: i64 = -32017; | |||
pub const JSON_RPC_SERVER_ERROR_SLOT_NOT_EPOCH_BOUNDARY: i64 = -32018; | |||
pub const JSON_RPC_SERVER_ERROR_LONG_TERM_STORAGE_UNREACHABLE: i64 = -32019; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will dutifully add this to @solana/web3.js
once landed.
@@ -244,6 +247,11 @@ impl From<RpcCustomError> for Error { | |||
), | |||
data: None, | |||
}, | |||
RpcCustomError::LongTermStorageUnreachable => Self { | |||
code: ErrorCode::ServerError(JSON_RPC_SERVER_ERROR_LONG_TERM_STORAGE_UNREACHABLE), | |||
message: "Failed to query long-term storage; please try again".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured I'd say ‘please try again’ to indicate that the failure is transient.
.map_err(|err| match err { | ||
bigtable::Error::RowNotFound => Error::SignatureNotFound, | ||
_ => err.into(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something similar was added to get_signature_status
in 71e9958 but we could use it here, to disambiguate between ‘couldn't find the thing’ and ‘o no bigtable down bad.’
@@ -1761,8 +1761,8 @@ impl JsonRpcRequestProcessor { | |||
bigtable_before = None; | |||
} | |||
Err(err) => { | |||
warn!("{:?}", err); | |||
return Ok(map_results(results)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, this code would catch all manner of Bigtable failure, including connection failures, and return whatever results we have so far.
warn!("{:?}", err); | ||
return Ok(map_results(results)); | ||
warn!("Failed to query Bigtable: {:?}", err); | ||
return Err(RpcCustomError::LongTermStorageUnreachable.into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that storage_bigtable/lib.rs
properly throws SignatureNotFound
errors, we can be sure that when this arm is matched, it's because of a Bigtable connection error.
@@ -1791,8 +1791,10 @@ impl JsonRpcRequestProcessor { | |||
} | |||
} | |||
} | |||
Err(StorageError::SignatureNotFound) => {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A failure to find the before
or until
signatures continues to behave like the old code: skip and move on.
Err(err) => { | ||
warn!("{:?}", err); | ||
warn!("Failed to query Bigtable: {:?}", err); | ||
return Err(RpcCustomError::LongTermStorageUnreachable.into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that storage_bigtable/lib.rs
properly throws SignatureNotFound errors, we can be sure that when this arm is matched, it's because of a Bigtable connection error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.map(|x| { | ||
let mut item: RpcConfirmedTransactionStatusWithSignature = x.into(); | ||
if item.slot <= highest_super_majority_root { | ||
item.confirmation_status = Some(TransactionConfirmationStatus::Finalized); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like there was wrong indentation, weird that cargo fmt didn't complain on the old code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through the code, looks like a really good change to me.
Problem
Consider a request to
getSignaturesForAddress()
. Imagine that there are no signatures in blockstore, but there are signatures in long-term storage (ie. Bigtable).Currently, if we fail to reach Bigtable – because of a timeout or a connection failure – we return whatever signatures we have. This means that people who query the RPC can't distinguish between the following cases:
Summary of Changes
get_confirmed_signatures_for_address
and thebefore
/until
can't be found, throw aSignatureNotFound
error instead ofRowNotFound
.SignatureNotFound
errors – return a JSON-RPC error in the event that long-term storage errors out.Test Plan
With the Bigtable emulator
Fetching a signature for an address that does not exist locally
Observe that the RPC goes out to Bigtable and fetches successfully (finds nothing)
Fetching a signature for an address and before signature that don't exist locally
Observe that Bigtable can't find the before signature, but doesn't fatal the request
Fetching a signature for an address that is completely available locally
Observe Bigtable is never contacted.
Fetching a signature for an address that does exist but with a bad
before
signatureObserve that Bigtable can't find the before signature, but doesn't fatal the request
Fetching a signature before the last signature, forcing the RPC to go to Bigtable where there is no data
Observe that the RPC goes out to Bigtable and fetches successfully (finds nothing)
With the Bigtable emulator shut down, simulating a connection failure
Fetching a signature for an address that does not exist locally
Fetching a signature for an address and before signature that don't exist locally
Observe that Bigtable experiences a connection error.
Fetching a signature for an address that is completely available locally
Observe Bigtable is never contacted.
Fetching a signature for an address that does exist but with a bad
before
signatureObserve that Bigtable experiences a connection error.
Fetching a signature before the last signature, forcing the RPC to go to Bigtable where there is no data
Observe that Bigtable experiences a connection error.
Fixes #3696