Skip to content

fix(solana_test_utils): reject confirmed-but-failed airdrops, redact datasource credentials - #458

Open
shahan-khatchadourian-anchorage wants to merge 3 commits into
mainfrom
shahankhatch/surfpool-airdrop-confirmation
Open

fix(solana_test_utils): reject confirmed-but-failed airdrops, redact datasource credentials#458
shahan-khatchadourian-anchorage wants to merge 3 commits into
mainfrom
shahankhatch/surfpool-airdrop-confirmation

Conversation

@shahan-khatchadourian-anchorage

Copy link
Copy Markdown
Contributor

Closes #291.

What

SurfpoolManager::airdrop reports a confirmed-but-failed airdrop as success.
get_signature_status returns Result<Option<Result<(), TransactionError>>>,
so a confirmed failure is still a status, and matching Ok(Some(_status))
discards the inner result.

Three commits:

1. Reject confirmed-but-failed airdrops. All four cases are matched
explicitly: only Ok(Some(Ok(()))) returns the signature, a confirmed
TransactionError returns Err naming it, Ok(None) retries, and an
RPC-level error retries while retaining the error for the timeout message.
Both blocking RPC calls run under spawn_blocking behind an Arc<RpcClient>
the blocking client calls block_in_place internally, which panics on a
current_thread runtime, so the previous direct call made airdrop unusable
from a plain #[tokio::test].

2. Keep datasource credentials out of logs. SurfpoolConfig::default()
builds rpc_url from HELIUS_API_KEY, and two log sites render it: the
config's Debug output and the spawn argument list. redact_url_credentials
reduces a URL to scheme and host, covering credentials in the query string,
the path, and the userinfo. A hand-written Debug for SurfpoolConfig routes
rpc_url through it, so every {:?} is covered rather than one call site.

3. Cover every confirmation outcome offline. airdrop_with takes the
client and retry budget as arguments, so RpcClient::new_mock can drive each
outcome without a validator. Four tests pin all four arms and run in normal CI
in milliseconds.

Verification

  • cargo test -p solana_test_utils --lib — 11 passed (4 confirmation arms,
    7 redaction).
  • Mutation-checked: reintroducing if let Ok(Some(_status)) fails the two
    confirmed-failure tests and leaves success and timeout passing, so the
    regression cannot return silently.
  • cargo test -p solana_test_utils --test airdrop -- --ignored passes against
    a live surfpool mainnet fork.
  • cargo clippy -p solana_test_utils --all-targets -- -D warnings and
    cargo fmt --check clean under the pinned 1.88.0 toolchain.

Notes for review

  • The credential redaction is a separate concern from the airdrop fix, kept as
    its own commit so it can be read independently. Neither log site currently
    emits anywhere — nothing in these tests installs a tracing subscriber — so
    this is defense-in-depth for whenever one is present, not a live leak.
  • tests/airdrop.rs is #[ignore] and no CI job runs it: the only --ignored
    runner is scoped to -p visualsign-solana --test surfpool_fuzz. The four
    offline tests cover the logic; wiring the live test into
    surfpool-solana.yml is left for after test(surfpool): native cargo runner via build.rs + per-IDL macro #285 rewrites that workflow, to avoid
    authoring a conflict.
  • The live test compares a balance delta rather than an exact balance:
    Pubkey::new_unique is a counter, not a random source, so the target address
    is identical on every run.
  • A follow-up worth considering, out of scope here:
    solana_client::nonblocking::rpc_client::RpcClient would delete the Arc,
    both spawn_blocking hops, and the nested per-client runtime — but
    rpc_client() must keep returning the blocking client for existing callers,
    so adopting it means two client types in one file.

🤖 Generated with Claude Code

`get_signature_status` returns `Result<Option<Result<(), TransactionError>>>`,
so a confirmed failure is still a status. Matching `Ok(Some(_status))` treated
`Ok(Some(Err(tx_err)))` as success, so a failed airdrop reported `Ok`.

Match all four cases explicitly: only `Ok(Some(Ok(())))` returns the signature,
a confirmed `TransactionError` returns `Err` naming it, `Ok(None)` retries, and
an RPC-level error retries while retaining the error for the timeout message.

Both blocking RPC calls now run under `spawn_blocking` behind an
`Arc<RpcClient>`, matching the reasoning `wait_ready` documents for its own
probe, so a `current_thread` caller is not stalled.

`tests/airdrop.rs` covers the success path against a live surfpool fork:
the status is `Some(Ok(()))` and the account is credited. `#[ignore]`, since
it needs the `surfpool` binary and network access.

Closes #291

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`SurfpoolConfig::default()` builds `rpc_url` from `HELIUS_API_KEY`, so the URL
carries a credential. Two log sites render it: the config `Debug` output in
`start`, and the argument list in the spawn `debug!`.

`redact_url_credentials` reduces a URL to scheme and host, covering all three
places a credential hides: the query string, the path, and the userinfo
(`user:pass@host`). A hand-written `Debug` for `SurfpoolConfig` routes
`rpc_url` through it, so every `{:?}` on a config is covered rather than one
call site, and the spawn log redacts any argument that looks like a URL.

Unit tests cover each of those three placements, schemeless input, and
credential-free URLs, plus a guard asserting a key cannot appear in `Debug`
output.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…line

The confirmed-failure path had no test: reverting the match to
`if let Ok(Some(_status))` left the whole suite green, so the bug this crate
just fixed could return unnoticed.

`airdrop_with` takes the client and retry budget as arguments, which lets
`RpcClient::new_mock` drive each outcome without a validator. `MockSender`
keys its canned `getSignatureStatuses` response off the URL, so
`account_in_use` and `instruction_error` confirm with a `TransactionError`,
`sig_not_found` never confirms, and anything else confirms successfully. Four
tests pin all four arms and run in normal CI in milliseconds. Reintroducing the
old match fails the two confirmed-failure tests and leaves success and timeout
passing.

The retry budget moves into `AIRDROP_MAX_ATTEMPTS` and `AIRDROP_POLL_INTERVAL`,
the timeout message reports the wall-clock budget, the RPC error keeps its
source chain via `context`, failed probes log per attempt as `wait_ready` does,
and the last iteration no longer sleeps before giving up.

The live-fork test compares a balance delta: `Pubkey::new_unique` is a counter
rather than a random source, so the target address is identical on every run
and an exact balance would depend on it never being funded upstream.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 6, 2026 23:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes solana_test_utils Surfpool airdrop confirmation to correctly fail on confirmed TransactionError, prevents datasource credentials from being logged, and adds both offline and live (ignored) coverage for the new behavior.

Changes:

  • Refactors SurfpoolManager::airdrop to explicitly handle all get_signature_status outcomes and avoid blocking-client panics by using spawn_blocking.
  • Redacts RPC datasource credentials from SurfpoolConfig Debug output and from Surfpool spawn-arg logging.
  • Adds offline unit tests for all confirmation outcomes and an ignored live test that validates the end-to-end RPC path.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/solana_test_utils/src/surfpool/manager.rs Fixes airdrop confirmation semantics; adds retry/error reporting and blocking-client isolation; redacts logged spawn args.
src/solana_test_utils/src/surfpool/config.rs Introduces redact_url_credentials and a custom Debug impl to prevent leaking datasource credentials; adds unit tests.
src/solana_test_utils/tests/airdrop.rs Adds an ignored live integration test that validates credited lamports and confirmed-success status against a real surfpool fork.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +61 to +71
// The datasource URL is one of the args and carries credentials.
let loggable_args: Vec<String> = args
.iter()
.map(|arg| {
if arg.contains("://") {
redact_url_credentials(arg)
} else {
arg.clone()
}
})
.collect();
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(solana_test_utils): SurfpoolManager::airdrop returns Ok on confirmed-but-failed transactions

2 participants