From c6df53379b27aefc9fb95f1cf61bb115762ae93d Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Tue, 6 Aug 2024 15:47:51 +0200 Subject: [PATCH 01/12] feat: solana resigning --- state-chain/chains/src/lib.rs | 12 +++- state-chain/chains/src/mocks.rs | 8 ++- state-chain/chains/src/sol/api.rs | 11 +++- state-chain/pallets/cf-broadcast/src/lib.rs | 19 ++++--- state-chain/runtime/src/chainflip.rs | 61 +++++++++++++++------ 5 files changed, 81 insertions(+), 30 deletions(-) diff --git a/state-chain/chains/src/lib.rs b/state-chain/chains/src/lib.rs index 54b197422dc..01da9d48611 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -4,6 +4,7 @@ #![feature(split_array)] use core::{fmt::Display, iter::Step}; +use std::marker::PhantomData; use crate::{ benchmarking_value::{BenchmarkValue, BenchmarkValueExtended}, @@ -23,7 +24,8 @@ use frame_support::{ traits::{AtLeast32BitUnsigned, CheckedAdd, CheckedSub}, BoundedVec, DispatchError, }, - Blake2_256, CloneNoBound, DebugNoBound, EqNoBound, Parameter, PartialEqNoBound, StorageHasher, + Blake2_256, CloneNoBound, DebugNoBound, EqNoBound, Never, Parameter, PartialEqNoBound, + StorageHasher, }; use instances::{ChainCryptoInstanceAlias, ChainInstanceAlias}; use scale_info::TypeInfo; @@ -390,7 +392,7 @@ where call: &Call, payload: &<::ChainCrypto as ChainCrypto>::Payload, maybe_current_onchain_key: Option<<::ChainCrypto as ChainCrypto>::AggKey>, - ) -> bool; + ) -> RequiresSignatureRefresh; /// Calculate the Units of gas that is allowed to make this call. fn calculate_gas_limit(_call: &Call) -> Option { @@ -703,3 +705,9 @@ pub struct ChannelRefundParameters { pub refund_address: ForeignChainAddress, pub min_price: Price, } + +pub enum RequiresSignatureRefresh> { + True(Option), + False, + _Phantom(PhantomData, Never), +} diff --git a/state-chain/chains/src/mocks.rs b/state-chain/chains/src/mocks.rs index d0361b171bd..15d0566a614 100644 --- a/state-chain/chains/src/mocks.rs +++ b/state-chain/chains/src/mocks.rs @@ -428,7 +428,11 @@ impl, Call: ApiCall> _call: &Call, _payload: &<::ChainCrypto as ChainCrypto>::Payload, _maybe_current_on_chain_key: Option<<::ChainCrypto as ChainCrypto>::AggKey>, - ) -> bool { - REQUIRES_REFRESH.with(|is_valid| *is_valid.borrow()) + ) -> RequiresSignatureRefresh { + if REQUIRES_REFRESH.with(|is_valid| *is_valid.borrow()) { + RequiresSignatureRefresh::True(None) + } else { + RequiresSignatureRefresh::False + } } } diff --git a/state-chain/chains/src/sol/api.rs b/state-chain/chains/src/sol/api.rs index 1762a50ea9b..5742f5774b7 100644 --- a/state-chain/chains/src/sol/api.rs +++ b/state-chain/chains/src/sol/api.rs @@ -131,6 +131,7 @@ pub enum SolanaTransactionType { pub struct SolanaApi { pub call_type: SolanaTransactionType, pub transaction: SolTransaction, + pub signer: Option, #[doc(hidden)] #[codec(skip)] _phantom: PhantomData, @@ -158,6 +159,7 @@ impl SolanaApi { Ok(Self { call_type: SolanaTransactionType::BatchFetch, transaction, + signer: None, _phantom: Default::default(), }) } @@ -210,6 +212,7 @@ impl SolanaApi { Self { call_type: SolanaTransactionType::Transfer, transaction, + signer: None, _phantom: Default::default(), }, vec![egress_id], @@ -255,6 +258,7 @@ impl SolanaApi { Ok(Self { call_type: SolanaTransactionType::RotateAggKey, transaction, + signer: None, _phantom: Default::default(), }) } @@ -371,6 +375,7 @@ impl SolanaApi { Ok(Self { call_type: SolanaTransactionType::CcmTransfer, transaction, + signer: None, _phantom: Default::default(), }) } @@ -384,9 +389,10 @@ impl ApiCall for SolanaApi { fn signed( mut self, threshold_signature: &::ThresholdSignature, - _signer: ::AggKey, + signer: ::AggKey, ) -> Self { self.transaction.signatures = vec![*threshold_signature]; + self.signer = Some(signer); self } @@ -407,7 +413,7 @@ impl ApiCall for SolanaApi { } fn signer(&self) -> Option<::AggKey> { - todo!() + self.signer } } @@ -510,6 +516,7 @@ impl SetGovKeyWithAggKey for Solan Ok(Self { call_type: SolanaTransactionType::SetGovKeyWithAggKey, transaction, + signer: None, _phantom: Default::default(), }) } diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 31dfe6f984b..e9da88579c1 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -15,7 +15,7 @@ use cf_primitives::{BroadcastId, ThresholdSignatureRequestId}; use cf_chains::{ address::IntoForeignChainAddress, ApiCall, Chain, ChainCrypto, FeeRefundCalculator, - RetryPolicy, TransactionBuilder, TransactionMetadata as _, + RequiresSignatureRefresh, RetryPolicy, TransactionBuilder, TransactionMetadata as _, }; use cf_traits::{ impl_pallet_safe_mode, offence_reporting::OffenceReporter, BroadcastNomination, Broadcaster, @@ -773,13 +773,18 @@ impl, I: 'static> Pallet { if let Some(broadcast_data) = AwaitingBroadcast::::get(broadcast_id) { // If the broadcast is not pending, we should not retry. - if let Some(api_call) = PendingApiCalls::::get(broadcast_id) { - if T::TransactionBuilder::requires_signature_refresh( - &api_call, - &broadcast_data.threshold_signature_payload, - CurrentOnChainKey::::get(), - ) { + if let Some(mut api_call) = PendingApiCalls::::get(broadcast_id) { + if let RequiresSignatureRefresh::True(maybe_modified_apicall) = + T::TransactionBuilder::requires_signature_refresh( + &api_call, + &broadcast_data.threshold_signature_payload, + CurrentOnChainKey::::get(), + ) { Self::deposit_event(Event::::ThresholdSignatureInvalid { broadcast_id }); + if let Some(modified_apicall) = maybe_modified_apicall { + PendingApiCalls::::insert(broadcast_id, modified_apicall.clone()); + api_call = modified_apicall; + } Self::threshold_sign(api_call, broadcast_id, true); log::info!( "Signature is invalid -> rescheduled threshold signature for broadcast id {}.", diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index 26bf7dc6833..dba8f965e6a 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -54,8 +54,8 @@ use cf_chains::{ }, AnyChain, ApiCall, Arbitrum, CcmChannelMetadata, CcmDepositMetadata, Chain, ChainCrypto, ChainEnvironment, ChainState, ChannelRefundParameters, DepositChannel, ForeignChain, - ReplayProtectionProvider, SetCommKeyWithAggKey, SetGovKeyWithAggKey, Solana, - TransactionBuilder, + ReplayProtectionProvider, RequiresSignatureRefresh, SetCommKeyWithAggKey, SetGovKeyWithAggKey, + Solana, TransactionBuilder, }; use cf_primitives::{chains::assets, AccountRole, Asset, BasisPoints, Beneficiaries, ChannelId}; use cf_traits::{ @@ -234,8 +234,8 @@ macro_rules! impl_transaction_builder_for_evm_chain { call: &$chain_api<$env>, _payload: &::Payload, maybe_current_on_chain_key: Option<::AggKey> - ) -> bool { - maybe_current_on_chain_key.map_or(false, |current_on_chain_key| call.signer().is_some_and(|signer|current_on_chain_key != signer )) + ) -> RequiresSignatureRefresh> { + maybe_current_on_chain_key.map_or(RequiresSignatureRefresh::False, |current_on_chain_key| if call.signer().is_some_and(|signer|current_on_chain_key != signer ) {RequiresSignatureRefresh::True(None)} else {RequiresSignatureRefresh::False}) } /// Calculate the gas limit for a this evm chain's call, using the current gas price. @@ -283,10 +283,14 @@ impl TransactionBuilder> for DotTransactio call: &PolkadotApi, payload: &<::ChainCrypto as ChainCrypto>::Payload, _maybe_current_onchain_key: Option<::AggKey>, - ) -> bool { + ) -> RequiresSignatureRefresh> { // Current key and signature are irrelevant. The only thing that can invalidate a polkadot // transaction is if the payload changes due to a runtime version update. - &call.threshold_signature_payload() != payload + if &call.threshold_signature_payload() != payload { + RequiresSignatureRefresh::True(None) + } else { + RequiresSignatureRefresh::False + } } } @@ -308,14 +312,14 @@ impl TransactionBuilder> for BtcTransactionB _call: &BitcoinApi, _payload: &<::ChainCrypto as ChainCrypto>::Payload, _maybe_current_onchain_key: Option<::AggKey>, - ) -> bool { + ) -> RequiresSignatureRefresh> { // The payload for a Bitcoin transaction will never change and so it doesnt need to be // checked here. We also dont need to check for the signature here because even if we are in // the next epoch and the key has changed, the old signature for the btc tx is still valid // since its based on those old input UTXOs. In fact, we never have to resign btc txs and // the btc tx is always valid as long as the input UTXOs are valid. Therefore, we don't have // to check anything here and just rebroadcast. - false + RequiresSignatureRefresh::False } } @@ -341,16 +345,39 @@ impl TransactionBuilder> for SolanaTransaction } fn requires_signature_refresh( - _call: &SolanaApi, + call: &SolanaApi, _payload: &<::ChainCrypto as ChainCrypto>::Payload, - _maybe_current_onchain_key: Option<::AggKey>, - ) -> bool { - // We use Durable Nonce mechanism to avoid the 150 blocks expiry period and so - // transactions won't expire. Then, the only reason to resign would be if the - // payload has been updated or the aggKey has been updated (key rotation). - // The payload won't be refreshed, as explained above, and the the broadcast - // barrier prevents a transaction from requiring to be resigned by a new aggKey. - false + maybe_current_onchain_key: Option<::AggKey>, + ) -> RequiresSignatureRefresh> { + // The only reason to resign would be if the aggKey has been updated on chain (key rotation) + // and this apicall is signed with the old key and is still pending. In this case, we need + // to modify the apicall, by replacing the aggkey with the new key, in the key_accounts in + // the tx's message to create a new valid threshold signing payload. + maybe_current_onchain_key.map_or(RequiresSignatureRefresh::False, |current_on_chain_key| { + match call.signer().and_then(|signer| { + if signer != current_on_chain_key { + Some(signer) + } else { + None + } + }) { + Some(signer) => { + let mut modified_call = (*call).clone(); + let current_aggkey = SolanaThresholdSigner::keys( + SolanaThresholdSigner::current_key_epoch().unwrap(), + ) + .unwrap(); + for (i, key) in call.transaction.message.account_keys.iter().enumerate() { + if *key == signer.into() { + modified_call.transaction.message.account_keys[i] = + current_aggkey.into() + } + } + RequiresSignatureRefresh::True(Some(call.clone())) + }, + None => RequiresSignatureRefresh::False, + } + }) } } From 81525214edb6a4f88eb15f77346b58db9e9cb9fb Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Tue, 6 Aug 2024 19:23:59 +0200 Subject: [PATCH 02/12] fix: clippy, comment --- state-chain/chains/src/lib.rs | 2 +- state-chain/runtime/src/chainflip.rs | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/state-chain/chains/src/lib.rs b/state-chain/chains/src/lib.rs index 01da9d48611..899c968c89e 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -4,7 +4,7 @@ #![feature(split_array)] use core::{fmt::Display, iter::Step}; -use std::marker::PhantomData; +use sp_std::marker::PhantomData; use crate::{ benchmarking_value::{BenchmarkValue, BenchmarkValueExtended}, diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index dba8f965e6a..1b2c243b38a 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -363,6 +363,9 @@ impl TransactionBuilder> for SolanaTransaction }) { Some(signer) => { let mut modified_call = (*call).clone(); + // the unwraps should be safe because we are in the code where on chain key + // already exists (see above) and so thecurrent_key_epoch should also exist and + // so should the corresponding key, let current_aggkey = SolanaThresholdSigner::keys( SolanaThresholdSigner::current_key_epoch().unwrap(), ) From d070c6d214c0e5ed5d55f844b1e4ccb8dd6df872 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Thu, 8 Aug 2024 15:42:08 +0200 Subject: [PATCH 03/12] fix: return mdofied_call --- state-chain/runtime/src/chainflip.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index 1b2c243b38a..73c55afdada 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -376,7 +376,7 @@ impl TransactionBuilder> for SolanaTransaction current_aggkey.into() } } - RequiresSignatureRefresh::True(Some(call.clone())) + RequiresSignatureRefresh::True(Some(modified_call.clone())) }, None => RequiresSignatureRefresh::False, } From 1f8327d5c800d9167b06ece3d3b4af609492ceb7 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Fri, 9 Aug 2024 11:55:09 +0200 Subject: [PATCH 04/12] chore: address comments --- state-chain/runtime/src/chainflip.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index 73c55afdada..edbc4e59a86 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -235,7 +235,8 @@ macro_rules! impl_transaction_builder_for_evm_chain { _payload: &::Payload, maybe_current_on_chain_key: Option<::AggKey> ) -> RequiresSignatureRefresh> { - maybe_current_on_chain_key.map_or(RequiresSignatureRefresh::False, |current_on_chain_key| if call.signer().is_some_and(|signer|current_on_chain_key != signer ) {RequiresSignatureRefresh::True(None)} else {RequiresSignatureRefresh::False}) + maybe_current_on_chain_key.map_or(RequiresSignatureRefresh::False, |current_on_chain_key| + if call.signer().is_some_and(|signer|current_on_chain_key != signer ) {RequiresSignatureRefresh::True(None)} else {RequiresSignatureRefresh::False}) } /// Calculate the gas limit for a this evm chain's call, using the current gas price. @@ -363,13 +364,9 @@ impl TransactionBuilder> for SolanaTransaction }) { Some(signer) => { let mut modified_call = (*call).clone(); - // the unwraps should be safe because we are in the code where on chain key - // already exists (see above) and so thecurrent_key_epoch should also exist and - // so should the corresponding key, - let current_aggkey = SolanaThresholdSigner::keys( - SolanaThresholdSigner::current_key_epoch().unwrap(), - ) - .unwrap(); + // the unwrap should be safe because we are in the code where on chain key + // already exists (see above) and so the active_epoch_key() should also exist. + let current_aggkey = SolanaThresholdSigner::active_epoch_key().unwrap().key; for (i, key) in call.transaction.message.account_keys.iter().enumerate() { if *key == signer.into() { modified_call.transaction.message.account_keys[i] = From a327a5251f9f5bf1ecf55d05a4e3b05a4ad3a70c Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Fri, 9 Aug 2024 14:19:28 +0200 Subject: [PATCH 05/12] chore: format --- state-chain/runtime/src/chainflip.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index edbc4e59a86..75eb8d52469 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -235,8 +235,13 @@ macro_rules! impl_transaction_builder_for_evm_chain { _payload: &::Payload, maybe_current_on_chain_key: Option<::AggKey> ) -> RequiresSignatureRefresh> { - maybe_current_on_chain_key.map_or(RequiresSignatureRefresh::False, |current_on_chain_key| - if call.signer().is_some_and(|signer|current_on_chain_key != signer ) {RequiresSignatureRefresh::True(None)} else {RequiresSignatureRefresh::False}) + maybe_current_on_chain_key.map_or(RequiresSignatureRefresh::False, + |current_on_chain_key| if call.signer().is_some_and(|signer|current_on_chain_key != signer ) { + RequiresSignatureRefresh::True(None) + } else { + RequiresSignatureRefresh::False + } + ) } /// Calculate the gas limit for a this evm chain's call, using the current gas price. From c3ceed340bb7844a6f6d8fd50c6c26d2f817a162 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Fri, 9 Aug 2024 15:04:26 +0200 Subject: [PATCH 06/12] chore: address comments --- state-chain/runtime/src/chainflip.rs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index 75eb8d52469..bceb954ee91 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -360,27 +360,20 @@ impl TransactionBuilder> for SolanaTransaction // to modify the apicall, by replacing the aggkey with the new key, in the key_accounts in // the tx's message to create a new valid threshold signing payload. maybe_current_onchain_key.map_or(RequiresSignatureRefresh::False, |current_on_chain_key| { - match call.signer().and_then(|signer| { - if signer != current_on_chain_key { - Some(signer) - } else { - None - } - }) { - Some(signer) => { + match call.signer() { + Some(signer) if signer != current_on_chain_key => { let mut modified_call = (*call).clone(); // the unwrap should be safe because we are in the code where on chain key // already exists (see above) and so the active_epoch_key() should also exist. let current_aggkey = SolanaThresholdSigner::active_epoch_key().unwrap().key; - for (i, key) in call.transaction.message.account_keys.iter().enumerate() { + for key in modified_call.transaction.message.account_keys.iter_mut() { if *key == signer.into() { - modified_call.transaction.message.account_keys[i] = - current_aggkey.into() + *key = current_aggkey.into() } } RequiresSignatureRefresh::True(Some(modified_call.clone())) }, - None => RequiresSignatureRefresh::False, + _ => RequiresSignatureRefresh::False, } }) } From ed724473d51bdc3c04c0b325f0eaefc95a206edd Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Tue, 13 Aug 2024 15:02:49 +0200 Subject: [PATCH 07/12] chore: no refresh replay protection for sol --- state-chain/chains/src/sol/api.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/state-chain/chains/src/sol/api.rs b/state-chain/chains/src/sol/api.rs index 5742f5774b7..866d932a5aa 100644 --- a/state-chain/chains/src/sol/api.rs +++ b/state-chain/chains/src/sol/api.rs @@ -409,7 +409,7 @@ impl ApiCall for SolanaApi { } fn refresh_replay_protection(&mut self) { - todo!() + // No replay protection refresh for Bitcoin. } fn signer(&self) -> Option<::AggKey> { From 1a44ce730b864802e036b2abd6b1b88714bed973 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Tue, 13 Aug 2024 15:09:03 +0200 Subject: [PATCH 08/12] fix: comment --- state-chain/chains/src/sol/api.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/state-chain/chains/src/sol/api.rs b/state-chain/chains/src/sol/api.rs index 866d932a5aa..caf37d48d38 100644 --- a/state-chain/chains/src/sol/api.rs +++ b/state-chain/chains/src/sol/api.rs @@ -409,7 +409,7 @@ impl ApiCall for SolanaApi { } fn refresh_replay_protection(&mut self) { - // No replay protection refresh for Bitcoin. + // No replay protection refresh for Solana. } fn signer(&self) -> Option<::AggKey> { From 94c06979b9ca108e35f35dc05bd3fb5cbc4ffff3 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Wed, 14 Aug 2024 10:23:14 +0200 Subject: [PATCH 09/12] feat: solana resigning test --- .../cf-integration-tests/src/solana.rs | 76 ++++++++++++++++++- state-chain/chains/src/sol/api.rs | 2 +- 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/state-chain/cf-integration-tests/src/solana.rs b/state-chain/cf-integration-tests/src/solana.rs index bec2f7ab2f3..74a4680a42e 100644 --- a/state-chain/cf-integration-tests/src/solana.rs +++ b/state-chain/cf-integration-tests/src/solana.rs @@ -1,18 +1,21 @@ #![cfg(test)] +use std::marker::PhantomData; + use super::*; use cf_chains::{ address::{AddressConverter, AddressDerivationApi, EncodedAddress}, assets::any::Asset, ccm_checker::CcmValidityError, sol::{ - api::{SolanaEnvironment, SolanaTransactionBuildingError}, - sol_tx_core::sol_test_values, + api::{SolanaApi, SolanaEnvironment, SolanaTransactionBuildingError}, + sol_tx_core::{sol_test_values, CompiledInstruction, MessageHeader}, SolAddress, SolApiEnvironment, SolCcmAccounts, SolCcmAddress, SolHash, SolPubkey, SolTrackedData, SolanaCrypto, }, CcmChannelMetadata, CcmDepositMetadata, Chain, ChainState, ExecutexSwapAndCallError, - ForeignChainAddress, SetAggKeyWithAggKey, SetAggKeyWithAggKeyError, Solana, SwapOrigin, + ForeignChainAddress, RequiresSignatureRefresh, SetAggKeyWithAggKey, SetAggKeyWithAggKeyError, + Solana, SwapOrigin, TransactionBuilder, }; use cf_primitives::{AccountRole, AuthorityCount, ForeignChain, SwapId}; use cf_test_utilities::{assert_events_match, assert_has_matching_event}; @@ -21,7 +24,10 @@ use frame_support::traits::{OnFinalize, UnfilteredDispatchable}; use pallet_cf_ingress_egress::DepositWitness; use pallet_cf_validator::RotationPhase; use state_chain_runtime::{ - chainflip::{address_derivation::AddressDerivation, ChainAddressConverter, SolEnvironment}, + chainflip::{ + address_derivation::AddressDerivation, ChainAddressConverter, SolEnvironment, + SolanaTransactionBuilder, + }, Runtime, RuntimeCall, RuntimeEvent, SolanaIngressEgress, SolanaInstance, SolanaThresholdSigner, Swapping, }; @@ -522,3 +528,65 @@ fn failed_ccm_does_not_consume_durable_nonce() { ) }); } + +#[test] +fn solana_resigning() { + const EPOCH_BLOCKS: u32 = 100; + const MAX_AUTHORITIES: AuthorityCount = 10; + super::genesis::with_test_defaults() + .blocks_per_epoch(EPOCH_BLOCKS) + .max_authorities(MAX_AUTHORITIES) + .with_additional_accounts(&[ + (DORIS, AccountRole::LiquidityProvider, 5 * FLIPPERINOS_PER_FLIP), + (ZION, AccountRole::Broker, 5 * FLIPPERINOS_PER_FLIP), + (ALICE, AccountRole::Broker, 5 * FLIPPERINOS_PER_FLIP), + (BOB, AccountRole::Broker, 5 * FLIPPERINOS_PER_FLIP), + ]) + .build() + .execute_with(|| { + let (mut testnet, _, _) = network::fund_authorities_and_join_auction(MAX_AUTHORITIES); + assert_ok!(RuntimeCall::SolanaVault( + pallet_cf_vaults::Call::::initialize_chain {} + ) + .dispatch_bypass_filter(pallet_cf_governance::RawOrigin::GovernanceApproval.into())); + testnet.move_to_the_next_epoch(); + + setup_sol_environments(); + register_refund_addresses(&DORIS); + setup_pool_and_accounts(vec![Asset::Sol, Asset::SolUsdc], OrderType::LimitOrder); + + const CURRENT_SIGNER: [u8; 32] = [3u8; 32]; + let apicall = SolanaApi { + call_type: cf_chains::sol::api::SolanaTransactionType::Transfer, + transaction: cf_chains::sol::SolTransaction { + signatures: vec![[1u8; 64].into()], + message: cf_chains::sol::SolMessage { + header: MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 1, + num_readonly_unsigned_accounts: 1, + }, + account_keys: vec![CURRENT_SIGNER.into()], + recent_blockhash: [4u8; 32].into(), + instructions: vec![CompiledInstruction { + program_id_index: 1, + accounts: vec![0], + data: vec![], + }], + }, + }, + signer: Some(CURRENT_SIGNER.into()), + _phantom: PhantomData::, + }; + + let modified_call = SolanaTransactionBuilder::requires_signature_refresh( + &apicall, + &Default::default(), + Some([5u8; 32].into()), + ); + + assert!(matches!(modified_call, RequiresSignatureRefresh::True(call) if + call.clone().unwrap().transaction.message.account_keys[0] == ::current_agg_key().unwrap().into())); + }); +} diff --git a/state-chain/chains/src/sol/api.rs b/state-chain/chains/src/sol/api.rs index caf37d48d38..c86d3109504 100644 --- a/state-chain/chains/src/sol/api.rs +++ b/state-chain/chains/src/sol/api.rs @@ -134,7 +134,7 @@ pub struct SolanaApi { pub signer: Option, #[doc(hidden)] #[codec(skip)] - _phantom: PhantomData, + pub _phantom: PhantomData, } impl SolanaApi { From 42393c2f0460622a75ba285d685238a4cc84e388 Mon Sep 17 00:00:00 2001 From: albert Date: Wed, 14 Aug 2024 13:39:19 +0200 Subject: [PATCH 10/12] chore: use real transfer and check against crafted transaction --- .../cf-integration-tests/src/solana.rs | 66 ++++++++++++------- 1 file changed, 42 insertions(+), 24 deletions(-) diff --git a/state-chain/cf-integration-tests/src/solana.rs b/state-chain/cf-integration-tests/src/solana.rs index 74a4680a42e..dd099ffeba4 100644 --- a/state-chain/cf-integration-tests/src/solana.rs +++ b/state-chain/cf-integration-tests/src/solana.rs @@ -9,7 +9,8 @@ use cf_chains::{ ccm_checker::CcmValidityError, sol::{ api::{SolanaApi, SolanaEnvironment, SolanaTransactionBuildingError}, - sol_tx_core::{sol_test_values, CompiledInstruction, MessageHeader}, + sol_tx_core::sol_test_values, + transaction_builder::SolanaTransactionBuilder, SolAddress, SolApiEnvironment, SolCcmAccounts, SolCcmAddress, SolHash, SolPubkey, SolTrackedData, SolanaCrypto, }, @@ -19,6 +20,7 @@ use cf_chains::{ }; use cf_primitives::{AccountRole, AuthorityCount, ForeignChain, SwapId}; use cf_test_utilities::{assert_events_match, assert_has_matching_event}; +use cf_utilities::bs58_array; use codec::Encode; use frame_support::traits::{OnFinalize, UnfilteredDispatchable}; use pallet_cf_ingress_egress::DepositWitness; @@ -26,7 +28,7 @@ use pallet_cf_validator::RotationPhase; use state_chain_runtime::{ chainflip::{ address_derivation::AddressDerivation, ChainAddressConverter, SolEnvironment, - SolanaTransactionBuilder, + SolanaTransactionBuilder as RuntimeSolanaTransactionBuilder, }, Runtime, RuntimeCall, RuntimeEvent, SolanaIngressEgress, SolanaInstance, SolanaThresholdSigner, Swapping, @@ -531,6 +533,8 @@ fn failed_ccm_does_not_consume_durable_nonce() { #[test] fn solana_resigning() { + use crate::solana::sol_test_values::TEST_DURABLE_NONCE; + const EPOCH_BLOCKS: u32 = 100; const MAX_AUTHORITIES: AuthorityCount = 10; super::genesis::with_test_defaults() @@ -556,37 +560,51 @@ fn solana_resigning() { setup_pool_and_accounts(vec![Asset::Sol, Asset::SolUsdc], OrderType::LimitOrder); const CURRENT_SIGNER: [u8; 32] = [3u8; 32]; + + let mut transaction = SolanaTransactionBuilder::transfer_native( + 10000000, + SolAddress(bs58_array("EwVmJgZwHBzmdVUzdujfbxFdaG25PMzbPLx8F7PvhWgs")), + CURRENT_SIGNER.into(), + (SolAddress(bs58_array("2cNMwUCF51djw2xAiiU54wz1WrU8uG4Q8Kp8nfEuwghw")), TEST_DURABLE_NONCE), + 100, + ).unwrap(); + transaction.signatures = vec![[1u8; 64].into()]; + + let original_account_keys = transaction.message.account_keys.clone(); + let apicall = SolanaApi { call_type: cf_chains::sol::api::SolanaTransactionType::Transfer, - transaction: cf_chains::sol::SolTransaction { - signatures: vec![[1u8; 64].into()], - message: cf_chains::sol::SolMessage { - header: MessageHeader { - num_required_signatures: 1, - num_readonly_signed_accounts: 1, - num_readonly_unsigned_accounts: 1, - }, - account_keys: vec![CURRENT_SIGNER.into()], - recent_blockhash: [4u8; 32].into(), - instructions: vec![CompiledInstruction { - program_id_index: 1, - accounts: vec![0], - data: vec![], - }], - }, - }, + transaction, signer: Some(CURRENT_SIGNER.into()), _phantom: PhantomData::, }; - let modified_call = SolanaTransactionBuilder::requires_signature_refresh( + let modified_call = RuntimeSolanaTransactionBuilder::requires_signature_refresh( &apicall, &Default::default(), Some([5u8; 32].into()), ); - - assert!(matches!(modified_call, RequiresSignatureRefresh::True(call) if - call.clone().unwrap().transaction.message.account_keys[0] == ::current_agg_key().unwrap().into())); + if let RequiresSignatureRefresh::True(call) = modified_call { + let agg_key = ::current_agg_key().unwrap(); + let transaction = call.clone().unwrap().transaction; + for (modified_key, original_key) in transaction.message.account_keys.iter().zip(original_account_keys.iter()) { + if *original_key != SolPubkey::from(CURRENT_SIGNER) { + assert_eq!(modified_key, original_key); + assert_ne!(*modified_key, SolPubkey::from(agg_key)); + } else { + assert_eq!(*modified_key, SolPubkey::from(agg_key)); + } + } + let serialized_tx = transaction + .clone() + .finalize_and_serialize().unwrap(); + + // Compare against a manually crafted transaction that works with the current test values and + // agg_key. Not comparing the first byte (number of signatures) nor the signature itself + let expected_serialized_tx = hex_literal::hex!("010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000306f68d61e8d834034cf583f486f2a08ef53ce4134ed41c4d88f4720c39518745b617eb2b10d3377bda2bc7bea65bec6b8372f4fc3463ec2cd6f9fde4b2c633d192cf1dd130e0341d60a0771ac40ea7900106a423354d2ecd6e609bd5e2ed833dec00000000000000000000000000000000000000000000000000000000000000000306466fe5211732ffecadba72c39be7bc8ce5bbc5f7126b2c439b3a4000000006a7d517192c568ee08a845f73d29788cf035c3145b21ab344d8062ea9400000c27e9074fac5e8d36cf04f94a0606fdd8ddbb420e99a489c7915ce5699e4890004030301050004040000000400090364000000000000000400050284030000030200020c020000008096980000000000").to_vec(); + assert_eq!(&serialized_tx[1+64..], &expected_serialized_tx[1+64..]); + } else { + panic!("RequiresSignatureRefresh is False"); + } }); } From 75d2ebc154a69fcc4e89cbd1bdd4675ccf79c1d0 Mon Sep 17 00:00:00 2001 From: Ramiz Siddiqui Date: Thu, 15 Aug 2024 11:23:07 +0200 Subject: [PATCH 11/12] chore: address comments --- state-chain/runtime/src/chainflip.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index bceb954ee91..fb7892b94f5 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -363,15 +363,18 @@ impl TransactionBuilder> for SolanaTransaction match call.signer() { Some(signer) if signer != current_on_chain_key => { let mut modified_call = (*call).clone(); - // the unwrap should be safe because we are in the code where on chain key - // already exists (see above) and so the active_epoch_key() should also exist. - let current_aggkey = SolanaThresholdSigner::active_epoch_key().unwrap().key; - for key in modified_call.transaction.message.account_keys.iter_mut() { - if *key == signer.into() { - *key = current_aggkey.into() - } - } - RequiresSignatureRefresh::True(Some(modified_call.clone())) + SolanaThresholdSigner::active_epoch_key().map_or( + RequiresSignatureRefresh::False, + |active_epoch_key| { + let current_aggkey = active_epoch_key.key; + for key in modified_call.transaction.message.account_keys.iter_mut() { + if *key == signer.into() { + *key = current_aggkey.into() + } + } + RequiresSignatureRefresh::True(Some(modified_call.clone())) + }, + ) }, _ => RequiresSignatureRefresh::False, } From a4f1554713a9c10ebafd2b864a6a6edceb052309 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 15 Aug 2024 13:54:16 +0200 Subject: [PATCH 12/12] fix: remove signer and signature on refresh --- state-chain/runtime/src/chainflip.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index 532c78d81ea..95c559c5e06 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -372,6 +372,10 @@ impl TransactionBuilder> for SolanaTransaction *key = current_aggkey.into() } } + for sig in modified_call.transaction.signatures.iter_mut() { + *sig = Default::default() + } + modified_call.signer = None; RequiresSignatureRefresh::True(Some(modified_call.clone())) }, )