Skip to content

Add the responsible program's account index and inner instruction index to each InstructionError #74

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add the responsible program's account index and inner instruction index to each InstructionError #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions transaction-error/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,13 @@ solana-frozen-abi-macro = { workspace = true, optional = true }
solana-instruction = { workspace = true, default-features = false, features = [
"std",
] }
solana-pubkey = { workspace = true }
solana-sanitize = { workspace = true }

[dev-dependencies]
serde_json = { workspace = true }
test-case = { workspace = true }

[features]
frozen-abi = ["dep:solana-frozen-abi", "dep:solana-frozen-abi-macro"]
serde = ["dep:serde", "dep:serde_derive", "solana-instruction/serde"]
Expand Down
112 changes: 107 additions & 5 deletions transaction-error/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
use serde_derive::{Deserialize, Serialize};
#[cfg(feature = "frozen-abi")]
use solana_frozen_abi_macro::{AbiEnumVisitor, AbiExample};
use {core::fmt, solana_instruction::error::InstructionError, solana_sanitize::SanitizeError};
use {
core::fmt, solana_instruction::error::InstructionError, solana_pubkey::Pubkey,
solana_sanitize::SanitizeError,
};

pub type TransactionResult<T> = Result<T, TransactionError>;

Expand Down Expand Up @@ -42,9 +45,23 @@ pub enum TransactionError {
/// the `recent_blockhash` has been discarded.
BlockhashNotFound,

/// An error occurred while processing an instruction. The first element of the tuple
/// indicates the instruction index in which the error occurred.
InstructionError(u8, InstructionError),
/// An error occurred while processing an instruction.
InstructionError {
err: InstructionError,
/// The index of the inner instruction in which the error was thrown, starting from zero.
/// This value will be `None` when the error was thrown from the outer instruction's
/// program, and also for all errors stored using a version of this enum variant prior to
/// `solana-transaction-error` 3.0.0.
inner_instruction_index: Option<u8>,
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I think this would have to be a u16 because it's possible that an instruction makes more than 256 CPI's.
  2. I prefer we don't record this index because it requires fetching the CPI trace metadata to understand and if we require fetching the CPI trace, I think it's better to record the CPI stack height instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea behind this value was not to correlate it with the trace, but rather to give clients something they can use for display. For instance, explorers typically think of the first inner instruction of the second top-level instruction as being ‘Instruction 2.1.’ The indexes for such an instruction would be { outer_instruction_index: 1, inner_instruction_index: Some(0) }.

Adding this value also doesn't add any weight in blockstore, since I was able to compress it into the existing uint32 that currently stores the top-level (outer) instruction index.

Copy link
Contributor

Choose a reason for hiding this comment

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

The explorer feature you mentioned is fully reliant on the CPI trace. I'm not concerned with extra weight in the blockstore. I just want this to provide the minimal context needed to correlate the error.

Copy link
Contributor Author

@steveluscher steveluscher Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, I can't convert this to a u16.

In blockstore, we don't store structured errors, we just dump bytes into a field.

This results in errors appearing like this in long-term storage:

// TransactionError::InstructionError(42, InstructionError::Custom(0xdeadbeef))
vec![
    8, 0, 0, 0, /* Eighth enum variant - `InstructionError` */
    42, /* Outer instruction index */
    25, 0, 0, 0, 239, 190, 173, 222, /* InstructionError::Custom(0xdeadbeef) */
];

I can't start writing a u16 into the part of that serialization destined for outer_instruction_index. Old versions of the validator won't be able to read that, and new versions of the validator will stop understanding old stored data.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need a u16, since we only execute up to 64 instructions before we hit our instruction limit (which I think is a separate error). At that point it wouldn't be an instruction error...so while it's technically (now) possible to have an instruction index > 255...I don't think it's actually possible to get an index>255 here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, yeah not possible to get index > 255 right now so can skip the u16, sorry for the noise!

/// The index of the outer instruction in which the error was thrown, starting from zero. Do
/// not infer the responsible program from the instruction at this index; the error might
/// have been thrown from one of its inner instructions. See `responsible_program_address`.
outer_instruction_index: u8,
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're making a breaking change, let's change this to a u16 because I think we have a bug where we incorrectly wrap around the ix index when a tx has more than 256 instructions.

/// The address of the program that threw the error. Use this to decode the error (eg. to
/// look up a custom error code in the program's IDL). This value will be `None` for errors
/// stored using a version of this enum variant prior to `solana-transaction-error` 3.0.0.
responsible_program_address: Option<Pubkey>,
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't store the program address here. It can be looked up via tx metadata. Let's record the stack height of the failure and the tx account index of the responsible program instead. If the responsible program was resolved from an ALT, the end user should resolve that themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A central principle of this change is that the error alone should be scrutible; all questions you might have about an error should be encapsulated in the error itself.

In any case, you might obtain this error from simulateTransaction where you definitely can't fetch tx metadata, and you may not possess the message that was actually simulated from which to decode tx account indexes (eg. it might have been modified by your wallet).

Copy link
Contributor

Choose a reason for hiding this comment

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

A central principle of this change is that the error alone should be scrutible; all questions you might have about an error should be encapsulated in the error itself.

I think you're trying to maximize error context and I'm trying to minimize it. I was under the assumption that the main problem this change is trying to fix is providing a way to correlate the error with the program that returned it. The other problems you're trying to solve can be solved by dev infra tools that provide users with full instruction traces and stuff.

},

/// Loader call chain is too deep
CallChainTooDeep,
Expand Down Expand Up @@ -163,7 +180,17 @@ impl fmt::Display for TransactionError {
=> f.write_str("This transaction has already been processed"),
Self::BlockhashNotFound
=> f.write_str("Blockhash not found"),
Self::InstructionError(idx, err) => write!(f, "Error processing Instruction {idx}: {err}"),
Self::InstructionError {
err,
outer_instruction_index,
..
}
// NOTE: We intentionally do not augment the error message in the event that the error
// carries the address of the responsible program or the index of the inner
// instruction. While it would add value to the log, to do so at this point would also
// break any log parser that presumes a stable log format
// (eg. https://tinyurl.com/3uuczr68).
=> write!(f, "Error processing Instruction {outer_instruction_index}: {err}"),
Self::CallChainTooDeep
=> f.write_str("Loader call chain is too deep"),
Self::MissingSignatureForFee
Expand Down Expand Up @@ -415,3 +442,78 @@ impl TransportError {

#[cfg(not(target_os = "solana"))]
pub type TransportResult<T> = std::result::Result<T, TransportError>;

#[cfg(feature = "serde")]
#[cfg(test)]
mod tests {
use {
crate::TransactionError,
serde_json::{from_value, json, to_value},
solana_instruction::error::InstructionError,
solana_pubkey::Pubkey,
test_case::test_case,
};

#[cfg(feature = "serde")]
#[test_case(InstructionError::Custom(42), 1, Some(Pubkey::new_unique()), None; "From top-level instruction")]
#[test_case(InstructionError::Custom(42), 1, Some(Pubkey::new_unique()), Some(41); "From inner instruction")]
#[test_case(InstructionError::Custom(42), 1, None, None; "Legacy instruction without inner/program")]
fn test_serialize_instruction_error(
err: InstructionError,
outer_instruction_index: u8,
responsible_program_address: Option<Pubkey>,
inner_instruction_index: Option<u8>,
) {
let json = to_value(TransactionError::InstructionError {
err: err.clone(),
inner_instruction_index,
outer_instruction_index,
responsible_program_address,
})
.unwrap();

assert_eq!(
json,
json!({
"InstructionError": {
"err": err,
"inner_instruction_index": inner_instruction_index,
"outer_instruction_index": outer_instruction_index,
"responsible_program_address": responsible_program_address,
},
}),
)
}

#[cfg(feature = "serde")]
#[test_case(InstructionError::Custom(42), 1, Some(Pubkey::new_unique()), None; "From top-level instruction")]
#[test_case(InstructionError::Custom(42), 1, Some(Pubkey::new_unique()), Some(41); "From inner instruction")]
#[test_case(InstructionError::Custom(42), 1, None, None; "Legacy instruction without inner/program")]
#[test_case(InstructionError::Custom(42), 1, None, Some(41); "Mixed instruction with inner index but no program address")]
fn test_deserialize_instruction_error(
err: InstructionError,
outer_instruction_index: u8,
responsible_program_address: Option<Pubkey>,
inner_instruction_index: Option<u8>,
) {
let decoded_err: TransactionError = from_value(json!({
"InstructionError": {
"err": err,
"inner_instruction_index": inner_instruction_index,
"outer_instruction_index": outer_instruction_index,
"responsible_program_address": responsible_program_address,
},
}))
.unwrap();

assert_eq!(
decoded_err,
TransactionError::InstructionError {
err,
inner_instruction_index,
outer_instruction_index,
responsible_program_address,
},
)
}
}
10 changes: 5 additions & 5 deletions transaction/src/sanitized.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,11 +298,11 @@ impl SanitizedTransaction {
self.message().instructions(),
feature_set,
)
.map_err(|err| {
TransactionError::InstructionError(
index as u8,
solana_instruction::error::InstructionError::Custom(err as u32),
)
.map_err(|err| TransactionError::InstructionError {
err: solana_instruction::error::InstructionError::Custom(err as u32),
inner_instruction_index: None,
outer_instruction_index: index as u8,
responsible_program_address: Some(*program_id),
})?;
}
Ok(())
Expand Down
Loading