spl: Bump to newest SPL crates for v2.1 release #3431
Conversation
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
#### Problem The downstream job in anza-xyz/agave#3431 is failing because of the new addition to the `UiExtension` enum. #### Summary of changes We've done this before: temporarily add a wildcard pattern so that the downstream job passes.
|
Downstream job failure will be fixed with solana-labs/solana-program-library#7446 |
#### Problem The downstream job in anza-xyz/agave#3431 is failing because of the new addition to the `UiExtension` enum. #### Summary of changes We've done this before: temporarily add a wildcard pattern so that the downstream job passes.
There was a problem hiding this comment.
Everything minus the transaction-status part is straightforward and looks good!
Regarding transaction-status, I commented on the confidential mint-burn extension. Let me know what you think because I could be missing something. If agreed, then the same comments should also apply for the confidential transfer parts with the proof offsets.
The regular confidential transfer instruction parsing logic is also a bit outdated. For example, UpdateMint checks for the wrong number of accounts and ConfigureAccount does not take into account pubkey validity proof.
I think other instructions that require zkp (withdraw, empty account, withdraw withheld tokens) also should be updated since the current parsing logic does not take into account context state accounts.
Sorry this is my fault for not keeping this part of the code base up-to-date. I would fix the rest on a follow-up PR, but since this is to be backported, we should probably fix it in this PR. Since this is a bit tedious, so I can help out and make commits to this PR if it is okay. Then we can get someone to help approve the PR. What do you think?
| })?; | ||
| let mut value = json!({ | ||
| "mint": account_keys[account_indexes[0] as usize].to_string(), | ||
| "proofAccount": account_keys[account_indexes[1] as usize].to_string(), |
There was a problem hiding this comment.
Should we explicitly check for the proof offset here and use instructionSysvar?
There was a problem hiding this comment.
That makes sense to me, done!
| // It isn't possible to figure out the accounts perfectly without | ||
| // data from other instructions in the transaction, so we do our | ||
| // best: if there are more than three accounts left, assume that | ||
| // they're context state / record proof accounts. | ||
| if account_indexes.len().saturating_sub(offset) > 3 { | ||
| map.insert( | ||
| "equalityProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset] as usize].to_string()), | ||
| ); | ||
| map.insert( | ||
| "ciphertextValidityProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset + 1] as usize].to_string()), | ||
| ); | ||
| map.insert( | ||
| "rangeProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset + 2] as usize].to_string()), | ||
| ); | ||
| offset += 3; | ||
| } |
There was a problem hiding this comment.
| // It isn't possible to figure out the accounts perfectly without | |
| // data from other instructions in the transaction, so we do our | |
| // best: if there are more than three accounts left, assume that | |
| // they're context state / record proof accounts. | |
| if account_indexes.len().saturating_sub(offset) > 3 { | |
| map.insert( | |
| "equalityProofAccount".to_string(), | |
| json!(account_keys[account_indexes[offset] as usize].to_string()), | |
| ); | |
| map.insert( | |
| "ciphertextValidityProofAccount".to_string(), | |
| json!(account_keys[account_indexes[offset + 1] as usize].to_string()), | |
| ); | |
| map.insert( | |
| "rangeProofAccount".to_string(), | |
| json!(account_keys[account_indexes[offset + 2] as usize].to_string()), | |
| ); | |
| offset += 3; | |
| } | |
| if mint_data.equality_proof_instruction_offset != 0 { | |
| let account_index = *account_indexes.get(offset).ok_or( | |
| ParseInstructionError::InstructionKeyMismatch(ParsableProgram::SplToken), | |
| )?; | |
| map.insert( | |
| "equalityProofRecordAccount".to_string(), | |
| json!(account_keys[account_index as usize].to_string()), | |
| ); | |
| offset += 1; | |
| } | |
| if mint_data.ciphertext_validity_proof_instruction_offset != 0 { | |
| let account_index = *account_indexes.get(offset).ok_or( | |
| ParseInstructionError::InstructionKeyMismatch(ParsableProgram::SplToken), | |
| )?; | |
| map.insert( | |
| "ciphertextValidityProofRecordAccount".to_string(), | |
| json!(account_keys[account_index as usize].to_string()), | |
| ); | |
| offset += 1; | |
| } | |
| if mint_data.range_proof_instruction_offset != 0 { | |
| let account_index = *account_indexes.get(offset).ok_or( | |
| ParseInstructionError::InstructionKeyMismatch(ParsableProgram::SplToken), | |
| )?; | |
| map.insert( | |
| "rangeProofRecordAccount".to_string(), | |
| json!(account_keys[account_index as usize].to_string()), | |
| ); | |
| offset += 1; | |
| } |
I might be missing something, but what do you think about this approach? If the proof offset is 0, then it is guaranteed that a record account is provided for that proof type, so we add it. If the offset is not 0, then a zk proof instruction must be included in the same transaction, but we don't parse that instruction here in the token submodule.
There was a problem hiding this comment.
This makes sense to me, except I think the code is backwards. We should add the account if the offset is zero as you wrote, right?
| // It isn't possible to figure out the accounts perfectly without | ||
| // data from other instructions in the transaction, so we do our | ||
| // best: if there are more than three accounts left, assume that | ||
| // they're context state / record proof accounts. | ||
| if account_indexes.len().saturating_sub(offset) > 3 { | ||
| map.insert( | ||
| "equalityProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset] as usize].to_string()), | ||
| ); | ||
| map.insert( | ||
| "ciphertextValidityProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset + 1] as usize].to_string()), | ||
| ); | ||
| map.insert( | ||
| "rangeProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset + 2] as usize].to_string()), | ||
| ); | ||
| offset += 3; | ||
| } |
There was a problem hiding this comment.
| // It isn't possible to figure out the accounts perfectly without | |
| // data from other instructions in the transaction, so we do our | |
| // best: if there are more than three accounts left, assume that | |
| // they're context state / record proof accounts. | |
| if account_indexes.len().saturating_sub(offset) > 3 { | |
| map.insert( | |
| "equalityProofAccount".to_string(), | |
| json!(account_keys[account_indexes[offset] as usize].to_string()), | |
| ); | |
| map.insert( | |
| "ciphertextValidityProofAccount".to_string(), | |
| json!(account_keys[account_indexes[offset + 1] as usize].to_string()), | |
| ); | |
| map.insert( | |
| "rangeProofAccount".to_string(), | |
| json!(account_keys[account_indexes[offset + 2] as usize].to_string()), | |
| ); | |
| offset += 3; | |
| } | |
| if mint_data.equality_proof_instruction_offset != 0 { | |
| let account_index = *account_indexes.get(offset).ok_or( | |
| ParseInstructionError::InstructionKeyMismatch(ParsableProgram::SplToken), | |
| )?; | |
| map.insert( | |
| "equalityProofRecordAccount".to_string(), | |
| json!(account_keys[account_index as usize].to_string()), | |
| ); | |
| offset += 1; | |
| } | |
| if mint_data.ciphertext_validity_proof_instruction_offset != 0 { | |
| let account_index = *account_indexes.get(offset).ok_or( | |
| ParseInstructionError::InstructionKeyMismatch(ParsableProgram::SplToken), | |
| )?; | |
| map.insert( | |
| "ciphertextValidityProofRecordAccount".to_string(), | |
| json!(account_keys[account_index as usize].to_string()), | |
| ); | |
| offset += 1; | |
| } | |
| if mint_data.range_proof_instruction_offset != 0 { | |
| let account_index = *account_indexes.get(offset).ok_or( | |
| ParseInstructionError::InstructionKeyMismatch(ParsableProgram::SplToken), | |
| )?; | |
| map.insert( | |
| "rangeProofRecordAccount".to_string(), | |
| json!(account_keys[account_index as usize].to_string()), | |
| ); | |
| offset += 1; | |
| } |
If I am not missing something from above. Likewise, if I am not missing something, let's try to do this?
There was a problem hiding this comment.
Yeah this works! I just think it's backwards again here.
| })?; | ||
| let mut value = json!({ | ||
| "mint": account_keys[account_indexes[0] as usize].to_string(), | ||
| "proofAccount": account_keys[account_indexes[1] as usize].to_string(), |
There was a problem hiding this comment.
That makes sense to me, done!
| // It isn't possible to figure out the accounts perfectly without | ||
| // data from other instructions in the transaction, so we do our | ||
| // best: if there are more than three accounts left, assume that | ||
| // they're context state / record proof accounts. | ||
| if account_indexes.len().saturating_sub(offset) > 3 { | ||
| map.insert( | ||
| "equalityProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset] as usize].to_string()), | ||
| ); | ||
| map.insert( | ||
| "ciphertextValidityProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset + 1] as usize].to_string()), | ||
| ); | ||
| map.insert( | ||
| "rangeProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset + 2] as usize].to_string()), | ||
| ); | ||
| offset += 3; | ||
| } |
There was a problem hiding this comment.
This makes sense to me, except I think the code is backwards. We should add the account if the offset is zero as you wrote, right?
| // It isn't possible to figure out the accounts perfectly without | ||
| // data from other instructions in the transaction, so we do our | ||
| // best: if there are more than three accounts left, assume that | ||
| // they're context state / record proof accounts. | ||
| if account_indexes.len().saturating_sub(offset) > 3 { | ||
| map.insert( | ||
| "equalityProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset] as usize].to_string()), | ||
| ); | ||
| map.insert( | ||
| "ciphertextValidityProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset + 1] as usize].to_string()), | ||
| ); | ||
| map.insert( | ||
| "rangeProofAccount".to_string(), | ||
| json!(account_keys[account_indexes[offset + 2] as usize].to_string()), | ||
| ); | ||
| offset += 3; | ||
| } |
There was a problem hiding this comment.
Yeah this works! I just think it's backwards again here.
|
Ok, this should be ready for another look based on our most recent discussion |
transaction-status/src/parse_token/extension/confidential_mint_burn.rs
Outdated
Show resolved
Hide resolved
transaction-status/src/parse_token/extension/confidential_transfer_fee.rs
Show resolved
Hide resolved
|
Can I get another approval? I had to rebase |
joncinque
added
the
automerge
#### Problem A lot of builds are currently broken due to the incompatibility between v2.0 and v2.1. All of the SPL crates have been bumped to use the Solana v2.1 crates, but it isn't being used in the monorepo. #### Summary of changes Bump the SPL crates to their newest versions, and correctly parse the new token-2022 instructions and account types. The trickiest changes are the confidential transfer / mint / burn instruction parsing, otherwise the changes are straightforward.
anza-team
removed
the
automerge
|
😱 New commits were pushed while the automerge label was present. |
joncinque
added
the
automerge
|
automerge label removed due to a CI failure |
mergify
bot
removed
the
automerge
joncinque
added
the
automerge
* spl: Bump to newest SPL crates for v2.1 release #### Problem A lot of builds are currently broken due to the incompatibility between v2.0 and v2.1. All of the SPL crates have been bumped to use the Solana v2.1 crates, but it isn't being used in the monorepo. #### Summary of changes Bump the SPL crates to their newest versions, and correctly parse the new token-2022 instructions and account types. The trickiest changes are the confidential transfer / mint / burn instruction parsing, otherwise the changes are straightforward. * Address feedback * Improve parsing logic per discussion * Parse initialize instructions correctly * Add comments for parsing * Improve account labels * Update lockfile from rebase (cherry picked from commit 43cf84f) # Conflicts: # Cargo.lock # programs/sbf/Cargo.lock # programs/sbf/Cargo.toml
#### Problem The new parsers for the confidential transfer extensions in #3431 don't have any tests, which is dangerous because they involve indexing directly into arrays, which can panic at runtime. #### Summary of changes Add tests for all the instructions involving proofs, which have the more complicated parsing logic. Specifically, it's testing to make sure that nothing panics. These tests actually uncovered a bug that triggered a panic for instructions with just barely enough accounts and also the instructions sysvar.
#### Problem The new parsers for the confidential transfer extensions in #3431 don't have any tests, which is dangerous because they involve indexing directly into arrays, which can panic at runtime. #### Summary of changes Add tests for all the instructions involving proofs, which have the more complicated parsing logic. Specifically, it's testing to make sure that nothing panics. These tests actually uncovered a bug that triggered a panic for instructions with just barely enough accounts and also the instructions sysvar. (cherry picked from commit 8730dbb) # Conflicts: # Cargo.toml # transaction-status/src/parse_token/extension/confidential_mint_burn.rs # transaction-status/src/parse_token/extension/confidential_transfer.rs
Problem
A lot of builds are currently broken due to the incompatibility between v2.0 and v2.1. All of the SPL crates have been bumped to use the Solana v2.1 crates, but it isn't being used in the monorepo.
Summary of changes
Bump the SPL crates to their newest versions, and correctly parse the new token-2022 instructions and account types.
The trickiest changes are the confidential transfer / mint / burn instruction parsing, otherwise the changes are straightforward.
Note: this needs to be backported to v2.1 so that users can use consistent crates. Currently, things are broken because of the major version bump of curve25519-dalek between v2.0 and v2.1. Once we break the circular dependency between the monorepo and SPL, this will be much easier.