AirgapTransactionError type from blockstore

[steveluscher]

Problem

At present, we leak the serialization format of TransactionError straight into blockstore. Any change to the serialization format of TransactionError (this being an example) could change the bytes that are stored in blockstore. A change in that serialization could cause old validator clients to fail to load transaction errors stored by newer clients, and vice versa.

Summary of Changes

In this PR, we create an ‘airgap’ – StoredTransaction between TransactionError and the type that is serialized for storage in blockstore.

This provides 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 bytes that are backward compatible with old validator clients, and you can create deserialization logic that handles bytes stored by older validator clients.

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

This is a prerequisite for #6083

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 82.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 82.8%. Comparing base (bfebc57) to head (308ede0).
Report is 21 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #6434     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         847      847             
  Lines      379486   379535     +49     
=========================================
+ Hits       314509   314547     +38     
- Misses      64977    64988     +11     

🚀 New features to boost your workflow:

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

[joncinque]

Makes sense to me! Confirming that the current generated::TransactionError is just a Vec<u8> currently, so the new type allows us to easily customize serde without any impact on sdk users.

Please get approval from at least one of the other reviewers before merging.

[joncinque]

nit: this might just be me, but I prefer using from instead of all the Intos

Suggested change

	let stored_error = Into::<StoredTransactionError>::into(value).0;
	let stored_error = StoredTransactionError::from(value).0;

[joncinque]

nit: is the Self needed here? I think the compiler should be able to figure it out

Suggested change

	bincode::deserialize::<Self>(&bytes).expect("transaction error to deserialize from bytes")
	bincode::deserialize(&bytes).expect("transaction error to deserialize from bytes")

[steveluscher]

How does this look, @steviez?

[steviez]

How does this look, @steviez?

I'll try to take a look in the next day or two; sorry for the delay and appreciate your patience

[steviez]

This is a prerequisite for #6083

I see that #6083 has been closed and you have created #6546 as an index of your thoughts.

Given that you appear to be abandoning the overall goal for the time being, do you think there is still value in landing this PR right now ?

[steveluscher]

I do, @steviez. It will make @jstarry's life easier (and subsequent PRs easier to review) if we land this work now.

[steviez]

Finally got around to looking at this. The general idea of the change makes sense.

I'm having to brush off some cobwebs to remember how all this stuff is hooked up so please bear with me on these questions 😅

Is this line below problematic ? Namely, we're invoking serialize() directly, but should we be using one of the From implementations ?

agave/storage-proto/src/convert.rs

Lines 426 to 431 in ffc8065

	let err = match status {
	Ok(()) => None,
	Err(err) => Some(generated::TransactionError {
	err: bincode::serialize(&err).expect("transaction error to serialize to bytes"),
	}),
	};

And is StoredTransactionError wired up to anything yet ? Should it be connected to this type ?

agave/storage-proto/src/lib.rs

Lines 177 to 178 in ffc8065

	#[derive(Serialize, Deserialize)]
	pub struct StoredTransactionStatusMeta {

[steveluscher]

Namely, we're invoking serialize() directly, but should we be using one of the From implementations ?

Yeesh. It does look as though I missed that. All this code used to be in one PR. PR splitting gone wrong. I'll finish this up next week.

[steveluscher]

#6704.