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] Fatal getSignaturesForAddress() when Bigtable errors #3700

Merged

Conversation

steveluscher
Copy link

@steveluscher steveluscher commented Nov 19, 2024

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:

  • No signatures were found because there are none, or
  • No signatures were found because Bigtable was temporarily unavailable

Summary of Changes

  1. When doing a range query in get_confirmed_signatures_for_address and the before/until can't be found, throw a SignatureNotFound error instead of RowNotFound.
  2. Now that we can match on actual connection errors – separate from SignatureNotFound errors – return a JSON-RPC error in the event that long-term storage errors out.

Test Plan

With the Bigtable emulator

> gcloud beta emulators bigtable start
> ./init-bigtable.sh
> tail -f validator.log | grep [Bb]ig[Tt]able
[2024-11-19T23:06:05.994676924Z INFO  solana_rpc::rpc_service] rpc configuration: JsonRpcConfig { enable_rpc_transaction_history: true, enable_extended_tx_metadata_storage: true, faucet_addr: Some(0.0.0.0:9900), health_check_slot_distance: 0, skip_preflight_health_check: false, rpc_bigtable_config: Some(RpcBigtableConfig { enable_bigtable_ledger_upload: false, bigtable_instance_name: "solana-ledger", bigtable_app_profile_id: "default", timeout: None, max_message_size: 67108864 }), max_multiple_accounts: None, account_indexes: AccountSecondaryIndexes { keys: None, indexes: {} }, rpc_threads: 0, rpc_niceness_adj: 0, full_api: true, rpc_scan_and_fix_roots: false, max_request_body_size: None, disable_health_check: true }
[2024-11-19T23:06:05.995145895Z INFO  solana_storage_bigtable::bigtable] Connecting to bigtable emulator at localhost:8086
[2024-11-19T23:06:05.995411312Z INFO  solana_rpc::rpc_service] BigTable ledger storage initialized
Fetching a signature for an address that does not exist locally

Observe that the RPC goes out to Bigtable and fetches successfully (finds nothing)

curl localhost:8899 -d '{
  "id": "1",
  "jsonrpc": "2.0",
  "method": "getSignaturesForAddress",
  "params": [
    "BzszvNFvokG4v4qb1ipYSDGtKuhen8PMtYjevfZeAkKZ"
  ]
}' --header 'Content-Type: application/json'
{"jsonrpc":"2.0","result":[],"id":"1"}
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

curl localhost:8899 -d '{
  "id": "1",
  "jsonrpc": "2.0",
  "method": "getSignaturesForAddress",
  "params": [
    "BzszvNFvokG4v4qb1ipYSDGtKuhen8PMtYjevfZeAkKZ",
    {"before":"31AUDAUXgD4B5DqqtFZZe8udgHUfbJX9dQbiXQGmNiRqeKRkzRTYitrRbJDtpt4DMd4P3G8haaXMJ8TU6wwQmf3h"}
  ]
}' --header 'Content-Type: application/json'
{"jsonrpc":"2.0","result":[],"id":"1"}
Fetching a signature for an address that is completely available locally

Observe Bigtable is never contacted.

curl localhost:8899 -d '{
  "id": "1",
  "jsonrpc": "2.0",
  "method": "getSignaturesForAddress",
  "params": [
    "jZ2L6sHvF862QMKYxD7ho7JQAX5m1rT9VzVKvzSSbin",
    {"limit": 1}
  ]
}' --header 'Content-Type: application/json'
{"jsonrpc":"2.0","result":[{"blockTime":1732058393,"confirmationStatus":"finalized","err":null,"memo":null,"signature":"3mZUQjhaGA7ytwb3g9PnjYL4d1ErsWf7nheMCqnSuwW6x2hF8tx4DuuvG1u7j6qVCC2J9JqpRHbJ7z57ayhJZyW2","slot":9284}],"id":"1"}
Fetching a signature for an address that does exist but with a bad before signature

Observe that Bigtable can't find the before signature, but doesn't fatal the request

curl localhost:8899 -d '{
  "id": "1",
  "jsonrpc": "2.0",
  "method": "getSignaturesForAddress",
  "params": [
    "jZ2L6sHvF862QMKYxD7ho7JQAX5m1rT9VzVKvzSSbin",
    {"before":"419mQJ2ZpM1uJ2VJyHckWNzmEpRKkyuirvRQH9NfgKvzQJxTcTa5v8U7tXJ4VkqEH8GsTtG7iqSsQBJe8VxxC2XQ","limit": 1}
  ]
}' --header 'Content-Type: application/json'
{"jsonrpc":"2.0","result":[],"id":"1"}
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)

curl localhost:8899 -d '{
  "id": "1",
  "jsonrpc": "2.0",
  "method": "getSignaturesForAddress",
  "params": [
    "jZ2L6sHvF862QMKYxD7ho7JQAX5m1rT9VzVKvzSSbin",
    {
      "before":"45z1V6qdGUXRHid4YDzSSqV13pegyP2naBaA3aG4ZoS4xMdKjC3eBvhwTkbXrqEXJT38grfRJ11wCTn5Qop67AEv",
      "limit": 1
    }
  ]
}' --header 'Content-Type: application/json'
{"jsonrpc":"2.0","result":[],"id":"1"}

With the Bigtable emulator shut down, simulating a connection failure

Fetching a signature for an address that does not exist locally
curl localhost:8899 -d '{
  "id": "1",
  "jsonrpc": "2.0",
  "method": "getSignaturesForAddress",
  "params": [
    "BzszvNFvokG4v4qb1ipYSDGtKuhen8PMtYjevfZeAkKZ"
  ]
}' --header 'Content-Type: application/json'
{"jsonrpc":"2.0","error":{"code":-32019,"message":"Failed to query long-term storage; please try again"},"id":"1"}
Fetching a signature for an address and before signature that don't exist locally

Observe that Bigtable experiences a connection error.

curl localhost:8899 -d '{
  "id": "1",
  "jsonrpc": "2.0",
  "method": "getSignaturesForAddress",
  "params": [
    "BzszvNFvokG4v4qb1ipYSDGtKuhen8PMtYjevfZeAkKZ",
    {"before":"31AUDAUXgD4B5DqqtFZZe8udgHUfbJX9dQbiXQGmNiRqeKRkzRTYitrRbJDtpt4DMd4P3G8haaXMJ8TU6wwQmf3h"}
  ]
}' --header 'Content-Type: application/json'
{"jsonrpc":"2.0","error":{"code":-32019,"message":"Failed to query long-term storage; please try again"},"id":"1"}
Fetching a signature for an address that is completely available locally

Observe Bigtable is never contacted.

curl localhost:8899 -d '{
  "id": "1",
  "jsonrpc": "2.0",
  "method": "getSignaturesForAddress",
  "params": [
    "jZ2L6sHvF862QMKYxD7ho7JQAX5m1rT9VzVKvzSSbin",
    {"limit": 1}
  ]
}' --header 'Content-Type: application/json'
{"jsonrpc":"2.0","result":{"jsonrpc":"2.0","result":[{"blockTime":1732058393,"confirmationStatus":"finalized","err":null,"memo":null,"signature":"3mZUQjhaGA7ytwb3g9PnjYL4d1ErsWf7nheMCqnSuwW6x2hF8tx4DuuvG1u7j6qVCC2J9JqpRHbJ7z57ayhJZyW2","slot":9284}],"id":"1"}
Fetching a signature for an address that does exist but with a bad before signature

Observe that Bigtable experiences a connection error.

curl localhost:8899 -d '{
  "id": "1",
  "jsonrpc": "2.0",
  "method": "getSignaturesForAddress",
  "params": [
    "jZ2L6sHvF862QMKYxD7ho7JQAX5m1rT9VzVKvzSSbin",
    {"before":"419mQJ2ZpM1uJ2VJyHckWNzmEpRKkyuirvRQH9NfgKvzQJxTcTa5v8U7tXJ4VkqEH8GsTtG7iqSsQBJe8VxxC2XQ","limit": 1}
  ]
}' --header 'Content-Type: application/json'
{"jsonrpc":"2.0","error":{"code":-32019,"message":"Failed to query long-term storage; please try again"},"id":"1"}
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.

curl localhost:8899 -d '{
  "id": "1",
  "jsonrpc": "2.0",
  "method": "getSignaturesForAddress",
  "params": [
    "jZ2L6sHvF862QMKYxD7ho7JQAX5m1rT9VzVKvzSSbin",
    {
      "before":"45z1V6qdGUXRHid4YDzSSqV13pegyP2naBaA3aG4ZoS4xMdKjC3eBvhwTkbXrqEXJT38grfRJ11wCTn5Qop67AEv",
      "limit": 1
    }
  ]
}' --header 'Content-Type: application/json'
{"jsonrpc":"2.0","error":{"code":-32019,"message":"Failed to query long-term storage; please try again"},"id":"1"}

Fixes #3696

@steveluscher steveluscher changed the title Fatal when there is no bigtable [rpc] Fatal getSignaturesForAddress() when Bigtable errors Nov 19, 2024
Copy link

mergify bot commented Nov 19, 2024

If this PR represents a change to the public RPC API:

  1. Make sure it includes a complementary update to rpc-client/ (example)
  2. Open a follow-up PR to update the JavaScript client @solana/web3.js (example)

Thank you for keeping the RPC clients in sync with the server API @steveluscher.

@steveluscher
Copy link
Author

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.’

Comment on lines +1692 to +1694
if !self.config.enable_rpc_transaction_history {
return Err(RpcCustomError::TransactionHistoryNotAvailable.into());
}
Copy link
Author

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;
Copy link
Author

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(),
Copy link
Author

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.

Comment on lines +800 to +802
.map_err(|err| match err {
bigtable::Error::RowNotFound => Error::SignatureNotFound,
_ => err.into(),
Copy link
Author

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));
Copy link
Author

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());
Copy link
Author

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) => {}
Copy link
Author

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());
Copy link
Author

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.

@steveluscher steveluscher marked this pull request as ready for review November 19, 2024 23:49
Copy link

@godmodegalactus godmodegalactus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@steveluscher steveluscher added the rust Pull requests that update Rust code label Nov 20, 2024
.map(|x| {
let mut item: RpcConfirmedTransactionStatusWithSignature = x.into();
if item.slot <= highest_super_majority_root {
item.confirmation_status = Some(TransactionConfirmationStatus::Finalized);

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.

Copy link

@KirillLykov KirillLykov left a 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.

@steveluscher steveluscher merged commit 52f132c into anza-xyz:master Nov 25, 2024
42 checks passed
@steveluscher steveluscher deleted the fatal-when-there-is-no-bigtable branch November 25, 2024 17:45
@steveluscher steveluscher added v2.0 Backport to v2.0 branch v2.1 Backport to v2.1 branch labels Jan 13, 2025
Copy link

mergify bot commented Jan 13, 2025

Backports to the stable branch 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.

Copy link

mergify bot commented Jan 13, 2025

Backports to the beta branch 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.

mergify bot pushed a commit that referenced this pull request Jan 13, 2025
* Unindent code in `get_signatures_for_address`

* Add a custom JSON-RPC error to throw when long-term storage (ie. Bigtable) can't be reached

* When the `before`/`until` signatures can't be found, throw `SignatureNotFound` instead of `RowNotFound`

* Fatal `getSignaturesForAddress` calls when Bigtable must be queried but can't be reached

(cherry picked from commit 52f132c)
mergify bot pushed a commit that referenced this pull request Jan 13, 2025
* Unindent code in `get_signatures_for_address`

* Add a custom JSON-RPC error to throw when long-term storage (ie. Bigtable) can't be reached

* When the `before`/`until` signatures can't be found, throw `SignatureNotFound` instead of `RowNotFound`

* Fatal `getSignaturesForAddress` calls when Bigtable must be queried but can't be reached

(cherry picked from commit 52f132c)
steveluscher added a commit that referenced this pull request Jan 16, 2025
…ackport of #3700) (#4443)

[rpc] Fatal `getSignaturesForAddress()` when Bigtable errors (#3700)

* Unindent code in `get_signatures_for_address`

* Add a custom JSON-RPC error to throw when long-term storage (ie. Bigtable) can't be reached

* When the `before`/`until` signatures can't be found, throw `SignatureNotFound` instead of `RowNotFound`

* Fatal `getSignaturesForAddress` calls when Bigtable must be queried but can't be reached

(cherry picked from commit 52f132c)

Co-authored-by: Steven Luscher <[email protected]>
steveluscher added a commit that referenced this pull request Jan 16, 2025
…ackport of #3700) (#4442)

[rpc] Fatal `getSignaturesForAddress()` when Bigtable errors (#3700)

* Unindent code in `get_signatures_for_address`

* Add a custom JSON-RPC error to throw when long-term storage (ie. Bigtable) can't be reached

* When the `before`/`until` signatures can't be found, throw `SignatureNotFound` instead of `RowNotFound`

* Fatal `getSignaturesForAddress` calls when Bigtable must be queried but can't be reached

(cherry picked from commit 52f132c)

Co-authored-by: Steven Luscher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Pull requests that update Rust code v2.0 Backport to v2.0 branch v2.1 Backport to v2.1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getSignaturesForAddress return [] (empty result) instead of json error, if BigTable connection is broken
3 participants