generic verify_precompiles #3055
Conversation
| @@ -2260,7 +2261,7 @@ fn verify_transaction( | |||
| let move_precompile_verification_to_svm = | |||
| feature_set.is_active(&feature_set::move_precompile_verification_to_svm::id()); | |||
| if !move_precompile_verification_to_svm { | |||
| if let Err(e) = transaction.verify_precompiles(feature_set) { | |||
| if let Err(e) = verify_precompiles(transaction, feature_set) { | |||
There was a problem hiding this comment.
implementation change, no affect on the RPC interface.
| @@ -107,6 +107,7 @@ assert_matches = { workspace = true } | |||
| ed25519-dalek = { workspace = true } | |||
| libsecp256k1 = { workspace = true } | |||
| memoffset = { workspace = true } | |||
| rand0-7 = { package = "rand", version = "0.7" } | |||
There was a problem hiding this comment.
need this to create the precompile instructions for testing.
logic was mainly copied from the individual precompile tests - if there's a better way without this old version would be happy to change & remove this
| .get_or_insert_with(|| message.instructions_iter().map(|ix| ix.data).collect()); | ||
| precompile | ||
| .verify(instruction.data, all_instruction_data, feature_set) | ||
| .map_err(|_| TransactionError::InvalidAccountIndex)?; |
There was a problem hiding this comment.
kept error type here consistent with what tx.verify_precompiles returns...but this does not seem like the correct variant to me.
There was a problem hiding this comment.
yeah I think we should fix that up, no idea why that was chosen
There was a problem hiding this comment.
Seems like the existing TransactionError::SignatureFailure is reasonable.
Do you feel that one is fine or should we add a new PrecompileFailure variant?
There was a problem hiding this comment.
TransactionError::InstructionError also seems reasonable to me
There was a problem hiding this comment.
InstructionError is probably best because invalid data offsets is not technically a signature error
runtime/src/verify_precompiles.rs
Outdated
| let precompiles = get_precompiles(); | ||
| for (program_id, instruction) in message.program_instructions_iter() { | ||
| for precompile in precompiles { | ||
| if program_id == &precompile.program_id { |
There was a problem hiding this comment.
You need to check if the precompile is enabled by a feature gate first here. Otherwise a newly added precompile would break consensus
| .get_or_insert_with(|| message.instructions_iter().map(|ix| ix.data).collect()); | ||
| precompile | ||
| .verify(instruction.data, all_instruction_data, feature_set) | ||
| .map_err(|_| TransactionError::InvalidAccountIndex)?; |
There was a problem hiding this comment.
yeah I think we should fix that up, no idea why that was chosen
Problem
verify_precompilesneeds to have a generic counterpart so that new transaction type(s) can verify themSummary of Changes
solana_runtime::verify_precompiles::verify_precompilesfnSanitizedTransaction::verify_precompileswas previously usedFixes #