Skip to content

send-transaction-service: use message hash and blockhash to look up committed tx status#6585

Merged
jstarry merged 2 commits intoanza-xyz:masterfrom
jstarry:key-by-hash
Jun 16, 2025
Merged

send-transaction-service: use message hash and blockhash to look up committed tx status#6585
jstarry merged 2 commits intoanza-xyz:masterfrom
jstarry:key-by-hash

Conversation

@jstarry
Copy link
Copy Markdown

@jstarry jstarry commented Jun 13, 2025

Problem

The STS (used by the send transaction RPC method) uses a slow signature lookup approach to determine whether a transaction has been committed or not

Summary of Changes

  • Use tx blockhash to lookup committed transaction status faster (no need to scan through all blockhash keys in the status cache anymore)
  • Switch to using message hash for committed transaction status lookups

Fixes #

@mergify
Copy link
Copy Markdown

mergify Bot commented Jun 13, 2025

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/kit (example)

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

@jstarry jstarry requested a review from steveluscher June 13, 2025 21:32
steveluscher
steveluscher previously approved these changes Jun 13, 2025
Copy link
Copy Markdown

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

…uses a slow signature lookup approach…

This is because it looks in all the blockhash partitions, right, instead of choosing the right one straight away?

Comment thread runtime/src/bank.rs Outdated
Comment thread runtime/src/bank.rs Outdated
.map(|v| v.1)
}

pub fn get_committed_transaction_status_slot(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What if we skated where the puck was going here, and instead returned a boolean to say if the thing errored or not. This envisions a future where BankStatusCache no longer contains TransactionResult, but at most an indication of whether a seen transaction was a success or failure.

pub fn get_committed_transaction_status_and_slot(
    &self,
    message_hash: &Hash,
    transaction_blockhash: &Hash,
) -> Option<(Slot, boolean)> {
    let rcache = self.status_cache.read().unwrap();
    rcache
        .get_status(message_hash, transaction_blockhash, &self.ancestors)
        .map(|(slot, transaction_result)| (slot, transaction_result.is_ok())) 
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah sounds great. Changed in 0f5b099

@jstarry jstarry marked this pull request as ready for review June 13, 2025 22:06
@jstarry jstarry changed the title send-transaction-service: Use message hash to look up tx commitment status send-transaction-service: use message hash and blockhash to look up committed tx status Jun 13, 2025
@jstarry
Copy link
Copy Markdown
Author

jstarry commented Jun 13, 2025

This is because it looks in all the blockhash partitions, right, instead of choosing the right one straight away?

Yes, that's correct. I updated the title and description to say that explicitly

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 97.69231% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.8%. Comparing base (69e749e) to head (0f5b099).
Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #6585     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         849      849             
  Lines      379196   379301    +105     
=========================================
+ Hits       314029   314113     +84     
- Misses      65167    65188     +21     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

@jstarry jstarry merged commit 520f85a into anza-xyz:master Jun 16, 2025
28 checks passed
@jstarry jstarry deleted the key-by-hash branch June 16, 2025 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants