Airgap`TransactionError` type from RPC by steveluscher · Pull Request #6435 · anza-xyz/agave

steveluscher · 2025-06-05T21:22:33Z

Problem

At present, we leak the serialization format of TransactionError straight out through the RPC. Any change to the serialization format of TransactionError (this being an example) could change the response format of the RPC API. Application clients that expect responses to be in a certain format (eg. { "InstructionError": [number, string | { [name: string]: unknown }] }) could break if the serialization ever changed (eg. { "InstructionError": { index: number, err: { __type: string } & Record<string, unknown> }).

Summary of Changes

In this PR, we create ‘airgaps’ between TransactionError and the type that is emitted from the RPC API:

UiTransactionError
UiTransactionResult

Those provide a locus for custom serialization logic. Now you can conceive of a change to the structure of TransactionError that can be handled by the custom serialization logic to produce JSON that is backward compatible with existing clients.

We also add tests to verify that the current JSON output for named, struct, tuple, and particularly InstructionError variants of TransactionError don't change without a test breaking.

This is a prerequisite for #6083

mergify · 2025-06-05T21:23:06Z

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

Make sure it includes a complementary update to rpc-client/ (example)
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 @steveluscher.

codecov-commenter · 2025-06-05T22:59:52Z

Codecov Report

❌ Patch coverage is 80.71429% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.8%. Comparing base (d849be8) to head (992a206).
⚠️ Report is 3097 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #6435     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         849      849             
  Lines      379208   379314    +106     
=========================================
+ Hits       314085   314161     +76     
- Misses      65123    65153     +30

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joncinque

Looks great overall, mostly small things

joncinque · 2025-06-09T21:26:58Z

 }

+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct UiTransactionError(pub TransactionError);


To really airgap this, it would be best to keep the inner type private. That way, users must use from / into to go between the two types

Yeah, this was all in service of this wild match:

match e.kind() { ErrorKind::Io(_) | ErrorKind::Reqwest(_) => { // fall through on io error, we will retry the transaction } ErrorKind::TransactionError(TransactionError::BlockhashNotFound) | ErrorKind::RpcError(RpcError::RpcResponseError { data: RpcResponseErrorData::SendTransactionPreflightFailure( RpcSimulateTransactionResult { err: Some(UiTransactionError(TransactionError::BlockhashNotFound)), .. }, ), .. }) => { // fall through so that we will resend with another blockhash } ErrorKind::TransactionError(transaction_error) | ErrorKind::RpcError(RpcError::RpcResponseError { data: RpcResponseErrorData::SendTransactionPreflightFailure( RpcSimulateTransactionResult { err: Some(UiTransactionError(transaction_error)), .. }, ), .. }) => { // if we get other than blockhash not found error the transaction is invalid context.error_map.insert(index, transaction_error.clone()); } _ => { return Err(TpuSenderError::from(e)); } }

Which produced this error when I made the inner type private:

error[E0532]: cannot match against a tuple struct which contains private fields --> client/src/send_and_confirm_transactions_in_parallel.rs:293:43 | 293 | ... err: Some(UiTransactionError(TransactionError::BlockhashNotFound)), | ^^^^^^^^^^^^^^^^^^ | note: constructor is not visible here due to private fields --> client/src/send_and_confirm_transactions_in_parallel.rs:293:62 | 293 | ... err: Some(UiTransactionError(TransactionError::BlockhashNotFound)), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ private field

I've forced it to work by changing the match.

joncinque

Just one last little thing, then this is good to go from my side!

joncinque · 2025-06-11T23:39:16Z

 solana-tps-client = { workspace = true }
 solana-tpu-client = { workspace = true }
 solana-transaction = { workspace = true }
+solana-transaction-error = { workspace = true }


Was this intentional?

joncinque · 2025-06-11T23:43:46Z

                    transaction,
                    get_transaction_error,
-                    err: transaction_status.err.clone(),
+                    err: transaction_status.err.clone().map(Into::into),


Since we're making breaking changes with 3.0, it might be worth having rpc-client return the new error type in follow-up work

Should we? I think people interacting with the rpc-client want to feed the TransactionError they get into {some code} where it's appropriate to feed it the ‘raw’ thing. UiTransactionError really just exists as a wrapper in which to locate stupid serialization logic like ‘you have to produce exactly this JSON for {reasons}.’ I think it's fine and appropriate to keep the Rust RPC client speaking in the base Rust types?

buffalojoec

Lgtm from SVM side.

buffalojoec · 2025-06-13T11:03:35Z

+                let outer_instruction_index: u8 = arr[0]
+                    .as_u64()
+                    .expect("Expected the first element to be the `outer_instruction_index`")
+                    as u8;
+                let err = InstructionError::deserialize(&arr[1])
+                    .expect("Expected the second element to deserialize as an `InstructionError`");


IMO, this function is already fallible, so it doesn't hurt to make these index accessors (arr[0]) as well as the expect calls fallible as well, rather than panics.

I struggled with this @buffalojoec. Is that what you had in mind? a9273c0

Yes! Just wanted to avoid the crashes. Calling .expect() or .unwrap() will crash the program if the value returns empty.

If you want, you can take a look at .and_then(), which could help you stack Result values together and simplify some of that. Up to you.

It works, and now I'm afraid to touch it.

* Changed CLI types to use `UiTransactionError` * Eliminated `UiTransactionResult` in favour of `Result<(), UiTransactionError>` * Implemented `std::error::Error` on `UiTransactionError` which just forwards to `TransactionError` * Implemented `fmt::Display` on `UiTransactionError` which just forwards to `TransactionError` * Made the inner error private on `UiTransactionError` * Eliminated `Deref` implementation, requiring explicit conversion

* Make the `InstructionError` deserialize actually work by taking all of the bytes

steveluscher · 2025-06-16T21:45:44Z

+                        DeserializeError::custom("Expected the first element to be a u64")
+                    })? as u8;
+                let rest_bytes: Vec<u8> = arr
+                    .get(1..)


Fixed this. Completely didn't work before.

buffalojoec

SVM stamp. 🤝

Please have a look at @joncinque's two comments.

steveluscher · 2025-06-24T19:16:04Z

Oof. I don't know what I was thinking with that deserializer update. Pushed 992a206 just now to simply take the second element in the array and pass it to from_value to do with what it will.

buffalojoec

Much cleaner!

* Add `UiTransactionError` and `UiTransactionResult` * Fixup callsites and tests * Review feedback * Changed CLI types to use `UiTransactionError` * Eliminated `UiTransactionResult` in favour of `Result<(), UiTransactionError>` * Implemented `std::error::Error` on `UiTransactionError` which just forwards to `TransactionError` * Implemented `fmt::Display` on `UiTransactionError` which just forwards to `TransactionError` * Made the inner error private on `UiTransactionError` * Eliminated `Deref` implementation, requiring explicit conversion * * Results, all the way down * Make the `InstructionError` deserialize actually work by taking all of the bytes * Remove stray dependency * Fix the deserializer

joncinque · 2025-10-23T00:02:53Z

To go along with the changes in #8625, we'll need to backport this PR to v2.3 for forward compatibility with v3 of InstructionError.

The airgapped types allow us to modify deserialization so that v2.3 clients / RPCs can deserialize the new error type into the old one. Otherwise RPCs are blocked from upgrading their fleet.

I tested this locally, and it works.

steveluscher requested review from joncinque and jstarry June 5, 2025 21:22

steveluscher force-pushed the airgap-transaction-error-type-from-rpc branch 5 times, most recently from 6f9a63c to 12c3fc7 Compare June 5, 2025 21:42

steveluscher requested a review from a team as a code owner June 5, 2025 21:42

steveluscher force-pushed the airgap-transaction-error-type-from-rpc branch from 12c3fc7 to 04e1299 Compare June 6, 2025 18:27

joncinque reviewed Jun 9, 2025

View reviewed changes

steveluscher requested a review from joncinque June 10, 2025 19:34

steveluscher force-pushed the airgap-transaction-error-type-from-rpc branch from 23b3c33 to 474dcb3 Compare June 10, 2025 19:35

joncinque reviewed Jun 12, 2025

View reviewed changes

steveluscher mentioned this pull request Jun 12, 2025

Eliminate Signature-to-TransactionResult cache from Bank #6546

Open

buffalojoec reviewed Jun 13, 2025

View reviewed changes

steveluscher added 4 commits June 16, 2025 21:30

Add UiTransactionError and UiTransactionResult

84d5f06

Fixup callsites and tests

688dbbe

* Results, all the way down

a9273c0

* Make the `InstructionError` deserialize actually work by taking all of the bytes

steveluscher force-pushed the airgap-transaction-error-type-from-rpc branch from 474dcb3 to a9273c0 Compare June 16, 2025 21:30

steveluscher requested a review from buffalojoec June 16, 2025 21:30

steveluscher commented Jun 16, 2025

View reviewed changes

buffalojoec previously approved these changes Jun 18, 2025

View reviewed changes

Remove stray dependency

84616b1

steveluscher dismissed buffalojoec’s stale review via 84616b1 June 23, 2025 18:54

steveluscher requested review from buffalojoec and joncinque June 23, 2025 18:54

buffalojoec previously approved these changes Jun 24, 2025

View reviewed changes

Fix the deserializer

992a206

steveluscher dismissed buffalojoec’s stale review via 992a206 June 24, 2025 19:15

steveluscher force-pushed the airgap-transaction-error-type-from-rpc branch from b78abe1 to 992a206 Compare June 24, 2025 19:15

steveluscher requested a review from buffalojoec June 24, 2025 19:16

buffalojoec approved these changes Jun 25, 2025

View reviewed changes

steveluscher merged commit e72266c into anza-xyz:master Jun 25, 2025
39 checks passed

steveluscher deleted the airgap-transaction-error-type-from-rpc branch June 25, 2025 23:09

joncinque mentioned this pull request Oct 23, 2025

transaction-status-client-types: Fix deserializer for SDKv2 #8625

Merged

Conversation

steveluscher commented Jun 5, 2025

Problem

Summary of Changes

Uh oh!

mergify Bot commented Jun 5, 2025

Uh oh!

codecov-commenter commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joncinque left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buffalojoec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buffalojoec Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buffalojoec left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveluscher commented Jun 24, 2025

Uh oh!

buffalojoec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joncinque commented Oct 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jun 5, 2025 •

edited

Loading

buffalojoec Jun 17, 2025 •

edited

Loading

buffalojoec left a comment •

edited

Loading