From 983eb1df1b0ee3383792295d4b89546681cb219f Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Wed, 27 Mar 2024 12:34:40 +0100 Subject: [PATCH 1/7] Run `cargo fmt --all` --- pallets/oracle/rpc/runtime-api/src/lib.rs | 4 ++-- testchain/node/src/service.rs | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pallets/oracle/rpc/runtime-api/src/lib.rs b/pallets/oracle/rpc/runtime-api/src/lib.rs index 55ba1f93c..df9b3c541 100644 --- a/pallets/oracle/rpc/runtime-api/src/lib.rs +++ b/pallets/oracle/rpc/runtime-api/src/lib.rs @@ -2,11 +2,11 @@ #![cfg_attr(not(feature = "std"), no_std)] use codec::{Codec, Decode, Encode}; -use frame_support::{dispatch::DispatchError}; +use frame_support::dispatch::DispatchError; use primitives::UnsignedFixedPoint; +use scale_info::TypeInfo; #[cfg(feature = "std")] use serde::{Deserialize, Deserializer, Serialize, Serializer}; -use scale_info::TypeInfo; #[derive(Eq, PartialEq, Encode, Decode, Default, TypeInfo)] #[cfg_attr(feature = "std", derive(Debug, Serialize, Deserialize))] #[cfg_attr(feature = "std", serde(rename_all = "camelCase"))] diff --git a/testchain/node/src/service.rs b/testchain/node/src/service.rs index 784aa06d8..98b94de78 100644 --- a/testchain/node/src/service.rs +++ b/testchain/node/src/service.rs @@ -4,7 +4,9 @@ use futures::StreamExt; use sc_client_api::BlockBackend; use sc_consensus_aura::{ImportQueueParams, SlotProportion, StartAuraParams}; use sc_consensus_grandpa::SharedVoterState; -use sc_executor::{NativeElseWasmExecutor, WasmExecutor, DEFAULT_HEAP_ALLOC_STRATEGY, HeapAllocStrategy}; +use sc_executor::{ + HeapAllocStrategy, NativeElseWasmExecutor, WasmExecutor, DEFAULT_HEAP_ALLOC_STRATEGY, +}; use sc_service::{ error::Error as ServiceError, Configuration, RpcHandlers, TFullBackend, TFullClient, TaskManager, @@ -115,7 +117,7 @@ pub fn new_partial_mainnet( .with_max_runtime_instances(config.max_runtime_instances) .with_runtime_cache_size(config.runtime_cache_size) .build(); - + let executor = NativeElseWasmExecutor::::new_with_wasm_executor(wasm); let (client, backend, keystore_container, task_manager) = @@ -219,7 +221,6 @@ pub fn new_partial_testnet( >, ServiceError, > { - let telemetry = config .telemetry_endpoints .clone() @@ -242,7 +243,7 @@ pub fn new_partial_testnet( .with_max_runtime_instances(config.max_runtime_instances) .with_runtime_cache_size(config.runtime_cache_size) .build(); - + let executor = NativeElseWasmExecutor::::new_with_wasm_executor(wasm); let (client, backend, keystore_container, task_manager) = @@ -461,8 +462,7 @@ pub fn new_full(mut config: Configuration) -> Result<(TaskManager, RpcHandlers), // if the node isn't actively participating in consensus then it doesn't // need a keystore, regardless of which protocol we use below. - let keystore = - if role.is_authority() { Some(keystore_container.keystore()) } else { None }; + let keystore = if role.is_authority() { Some(keystore_container.keystore()) } else { None }; let grandpa_config = sc_consensus_grandpa::Config { // FIXME #1578 make this available through chainspec From 46619fa74abafc49fe2c1029f336d3de8f688a9d Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Wed, 27 Mar 2024 16:21:53 +0100 Subject: [PATCH 2/7] Implement resubmission with fee bump transaction --- clients/wallet/src/resubmissions.rs | 61 ++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/clients/wallet/src/resubmissions.rs b/clients/wallet/src/resubmissions.rs index f4a0f55e1..1aa46b905 100644 --- a/clients/wallet/src/resubmissions.rs +++ b/clients/wallet/src/resubmissions.rs @@ -17,7 +17,7 @@ use tokio::time::sleep; use crate::horizon::responses::TransactionsResponseIter; #[cfg(test)] use mocktopus::macros::mockable; -use primitives::stellar::{types::SequenceNumber, PublicKey}; +use primitives::stellar::{types::SequenceNumber, FeeBumpTransaction, PublicKey, StroopAmount}; use reqwest::Client; pub const RESUBMISSION_INTERVAL_IN_SECS: u64 = 1800; @@ -150,6 +150,8 @@ impl StellarWallet { return self.handle_tx_bad_seq_error_with_xdr(envelope_xdr).await.map(Some), "tx_internal_error" => return self.handle_tx_internal_error(envelope_xdr).await.map(Some), + "tx_insufficient_fee" => + return self.handle_tx_insufficient_fee_error(envelope_xdr).await.map(Some), _ => { if let Ok(env) = decode_to_envelope(envelope_xdr) { self.remove_tx_envelope_from_cache(&env); @@ -191,6 +193,30 @@ impl StellarWallet { self.submit_transaction(envelope).await } + + // We encountered an error due to insufficient fee and try submitting the transaction again + // wrapped in a FeeBumpTransaction with a higher fee. The new fee has to be at least 10x the + // original fee, see [here](https://developers.stellar.org/docs/learn/encyclopedia/fees-surge-pricing-fee-strategies#fee-bumps-on-past-transactions). + async fn handle_tx_insufficient_fee_error( + &self, + envelope_xdr_as_str_opt: &Option, + ) -> Result { + let envelope = decode_to_envelope(envelope_xdr_as_str_opt)?; + + let source = self.public_key(); + // The new fee has to be at least 10x the original fee + let prev_fee = envelope.get_transaction().map(|tx| tx.fee).ok_or(DecodeError)?; + let new_fee: i64 = (prev_fee * 10) as i64; + let new_fee = StroopAmount(new_fee); + + let mut fee_bump_envelope = FeeBumpTransaction::new(source, new_fee, envelope) + .map_err(|_| DecodeError)? + .into_transaction_envelope(); + + self.sign_envelope(&mut fee_bump_envelope)?; + + self.submit_transaction(fee_bump_envelope).await + } } fn is_source_account_match(public_key: &PublicKey, tx: &TransactionResponse) -> bool { @@ -680,6 +706,39 @@ mod test { wallet.remove_cache_dir(); } + #[tokio::test] + #[serial] + async fn check_handle_tx_insufficient_fee_error_with_envelope() { + let wallet = + wallet_with_storage("resources/check_handle_tx_insufficient_fee_error_with_envelope") + .expect("should return a wallet") + .clone(); + let wallet = wallet.write().await; + + let sequence = wallet.get_sequence().await.expect("return a sequence"); + let envelope = wallet + .create_payment_envelope( + default_destination(), + StellarAsset::native(), + 13, + rand::random(), + 100, + sequence + 1, + ) + .expect("should return an envelope"); + + let envelope_xdr = envelope.to_base64_xdr(); + // Convert vec to string (because the HorizonSubmissionError always returns a string) + let envelope_xdr = + Some(String::from_utf8(envelope_xdr).expect("should create string from vec")); + + let result = wallet.handle_tx_insufficient_fee_error(&envelope_xdr).await; + + assert!(result.is_ok()); + + wallet.remove_cache_dir(); + } + #[tokio::test] #[serial] async fn check_handle_error() { From bf20c68a0a9cc4a536f3af83fd01b9380020753f Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Wed, 27 Mar 2024 16:38:43 +0100 Subject: [PATCH 3/7] Replace fee-bump transaction with normal transaction --- clients/wallet/src/horizon/responses.rs | 6 ++-- clients/wallet/src/resubmissions.rs | 43 ++++++++++++++++--------- 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/clients/wallet/src/horizon/responses.rs b/clients/wallet/src/horizon/responses.rs index 90f13e20c..86062419f 100644 --- a/clients/wallet/src/horizon/responses.rs +++ b/clients/wallet/src/horizon/responses.rs @@ -181,8 +181,8 @@ pub struct TransactionResponse { pub fee_account: Vec, #[serde(deserialize_with = "de_str_to_u64")] pub fee_charged: u64, - #[serde(deserialize_with = "de_str_to_bytes")] - pub max_fee: Vec, + #[serde(deserialize_with = "de_str_to_u64")] + pub max_fee: u64, operation_count: u32, #[serde(deserialize_with = "de_str_to_bytes")] pub envelope_xdr: Vec, @@ -219,7 +219,7 @@ impl Debug for TransactionResponse { .field("source_account_sequence", &debug_str_or_vec_u8!(&self.source_account_sequence)) .field("fee_account", &debug_str_or_vec_u8!(&self.fee_account)) .field("fee_charged", &self.fee_charged) - .field("max_fee", &debug_str_or_vec_u8!(&self.max_fee)) + .field("max_fee", &self.max_fee) .field("operation_count", &self.operation_count) .field("envelope_xdr", &debug_str_or_vec_u8!(&self.envelope_xdr)) .field("result_xdr", &debug_str_or_vec_u8!(&self.result_xdr)) diff --git a/clients/wallet/src/resubmissions.rs b/clients/wallet/src/resubmissions.rs index 1aa46b905..274d98ec4 100644 --- a/clients/wallet/src/resubmissions.rs +++ b/clients/wallet/src/resubmissions.rs @@ -17,7 +17,7 @@ use tokio::time::sleep; use crate::horizon::responses::TransactionsResponseIter; #[cfg(test)] use mocktopus::macros::mockable; -use primitives::stellar::{types::SequenceNumber, FeeBumpTransaction, PublicKey, StroopAmount}; +use primitives::stellar::{types::SequenceNumber, PublicKey}; use reqwest::Client; pub const RESUBMISSION_INTERVAL_IN_SECS: u64 = 1800; @@ -194,28 +194,31 @@ impl StellarWallet { self.submit_transaction(envelope).await } - // We encountered an error due to insufficient fee and try submitting the transaction again - // wrapped in a FeeBumpTransaction with a higher fee. The new fee has to be at least 10x the - // original fee, see [here](https://developers.stellar.org/docs/learn/encyclopedia/fees-surge-pricing-fee-strategies#fee-bumps-on-past-transactions). + // We encountered an insufficient fee error and try submitting the transaction again with a + // higher fee. We'll bump the fee by 10x the original fee. We don't use a FeeBumpTransaction + // because this operation is not supported by the stellar-relay pallet yet. async fn handle_tx_insufficient_fee_error( &self, envelope_xdr_as_str_opt: &Option, ) -> Result { - let envelope = decode_to_envelope(envelope_xdr_as_str_opt)?; + let tx_envelope = decode_to_envelope(envelope_xdr_as_str_opt)?; + let mut tx = tx_envelope.get_transaction().ok_or(DecodeError)?; - let source = self.public_key(); - // The new fee has to be at least 10x the original fee - let prev_fee = envelope.get_transaction().map(|tx| tx.fee).ok_or(DecodeError)?; - let new_fee: i64 = (prev_fee * 10) as i64; - let new_fee = StroopAmount(new_fee); + // Check if we already submitted this transaction + if !self.is_transaction_already_submitted(&tx).await { + // Remove original transaction. + // The same envelope will be saved again using a different sequence number + self.remove_tx_envelope_from_cache(&tx_envelope); - let mut fee_bump_envelope = FeeBumpTransaction::new(source, new_fee, envelope) - .map_err(|_| DecodeError)? - .into_transaction_envelope(); + // Bump the fee by 10x + tx.fee = tx.fee * 10; - self.sign_envelope(&mut fee_bump_envelope)?; + return self.bump_sequence_number_and_submit(tx).await + } + + tracing::error!("handle_tx_bad_seq_error_with_envelope(): Similar transaction already submitted. Skipping {:?}", tx); - self.submit_transaction(fee_bump_envelope).await + Err(ResubmissionError("Transaction already submitted".to_string())) } } @@ -715,12 +718,17 @@ mod test { .clone(); let wallet = wallet.write().await; + // This is the fee we will bump by 10x + let base_fee = 100; + // This is the new maximum fee we expect to be charged + let bumped_fee = base_fee * 10; + let sequence = wallet.get_sequence().await.expect("return a sequence"); let envelope = wallet .create_payment_envelope( default_destination(), StellarAsset::native(), - 13, + 10, rand::random(), 100, sequence + 1, @@ -735,6 +743,9 @@ mod test { let result = wallet.handle_tx_insufficient_fee_error(&envelope_xdr).await; assert!(result.is_ok()); + let response = result.unwrap(); + assert!(response.successful); + assert_eq!(response.max_fee, bumped_fee); wallet.remove_cache_dir(); } From 6344aaf0380ce741067b3660cf0645342b220096 Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Wed, 27 Mar 2024 16:38:49 +0100 Subject: [PATCH 4/7] Change log level --- clients/wallet/src/horizon/horizon.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/wallet/src/horizon/horizon.rs b/clients/wallet/src/horizon/horizon.rs index 25c0c3274..25a2b9319 100644 --- a/clients/wallet/src/horizon/horizon.rs +++ b/clients/wallet/src/horizon/horizon.rs @@ -267,7 +267,7 @@ impl HorizonFetcher { future::join(issue_map.read(), memos_to_issue_ids.read()).await; if issue_map.is_empty() || memos_to_issue_ids.is_empty() { - tracing::info!("fetch_horizon_and_process_new_transactions(): nothing to traverse"); + tracing::debug!("fetch_horizon_and_process_new_transactions(): nothing to traverse"); return Ok(last_cursor) } let mut txs_iter = self.fetch_transactions_iter(last_cursor).await?; From 824e6c5ecf88498dc735f995dc2cfefbcad14eb3 Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Wed, 27 Mar 2024 16:48:28 +0100 Subject: [PATCH 5/7] Change other test --- clients/wallet/src/resubmissions.rs | 48 +++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/clients/wallet/src/resubmissions.rs b/clients/wallet/src/resubmissions.rs index 274d98ec4..196a46f70 100644 --- a/clients/wallet/src/resubmissions.rs +++ b/clients/wallet/src/resubmissions.rs @@ -719,7 +719,7 @@ mod test { let wallet = wallet.write().await; // This is the fee we will bump by 10x - let base_fee = 100; + let base_fee = 99; // This is the new maximum fee we expect to be charged let bumped_fee = base_fee * 10; @@ -730,7 +730,7 @@ mod test { StellarAsset::native(), 10, rand::random(), - 100, + base_fee, sequence + 1, ) .expect("should return an envelope"); @@ -745,7 +745,7 @@ mod test { assert!(result.is_ok()); let response = result.unwrap(); assert!(response.successful); - assert_eq!(response.max_fee, bumped_fee); + assert_eq!(response.max_fee, bumped_fee as u64); wallet.remove_cache_dir(); } @@ -800,6 +800,48 @@ mod test { } } + // tx_insufficient_fee test + { + let envelope = wallet + .create_dummy_envelope_no_signature(19) + .await + .expect("returns an envelope"); + let envelope_xdr = envelope.to_base64_xdr(); + let envelope_xdr = String::from_utf8(envelope_xdr).ok(); + + // result is success + { + let error = Error::HorizonSubmissionError { + title: "title".to_string(), + status: 400, + reason: "tx_insufficient_fee".to_string(), + result_code_op: vec![], + envelope_xdr, + }; + + StellarWallet::is_transaction_already_submitted + .mock_safe(move |_, _| MockResult::Return(Box::pin(async move { false }))); + + assert!(wallet.handle_error(error).await.is_ok()); + } + + // result is error + { + let error = Error::HorizonSubmissionError { + title: "title".to_string(), + status: 400, + reason: "tx_insufficient_fee".to_string(), + result_code_op: vec![], + envelope_xdr: None, + }; + + match wallet.handle_error(error).await { + Err(Error::ResubmissionError(_)) => assert!(true), + other => panic!("expecting Error::ResubmissionError, found: {other:?}"), + }; + } + } + // tx_internal_error test { let envelope = wallet From cf015701fa861c9348d9e7c009707a0e2c5f930e Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Wed, 27 Mar 2024 17:00:36 +0100 Subject: [PATCH 6/7] Bound tx fee to a maximum --- clients/wallet/src/resubmissions.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/clients/wallet/src/resubmissions.rs b/clients/wallet/src/resubmissions.rs index 196a46f70..7950050a6 100644 --- a/clients/wallet/src/resubmissions.rs +++ b/clients/wallet/src/resubmissions.rs @@ -21,6 +21,8 @@ use primitives::stellar::{types::SequenceNumber, PublicKey}; use reqwest::Client; pub const RESUBMISSION_INTERVAL_IN_SECS: u64 = 1800; +// The maximum fee we want to charge for a transaction +const MAXIMUM_TX_FEE: u32 = 10_000_000; // 1 XLM #[cfg_attr(test, mockable)] impl StellarWallet { @@ -212,6 +214,9 @@ impl StellarWallet { // Bump the fee by 10x tx.fee = tx.fee * 10; + if tx.fee > MAXIMUM_TX_FEE { + tx.fee = MAXIMUM_TX_FEE; + } return self.bump_sequence_number_and_submit(tx).await } From 5596dbc17cc904f72de9db8f74a2b9c5ac58a441 Mon Sep 17 00:00:00 2001 From: Marcel Ebert Date: Thu, 28 Mar 2024 10:28:32 +0100 Subject: [PATCH 7/7] Update clients/wallet/src/resubmissions.rs --- clients/wallet/src/resubmissions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/wallet/src/resubmissions.rs b/clients/wallet/src/resubmissions.rs index 7950050a6..0585ef011 100644 --- a/clients/wallet/src/resubmissions.rs +++ b/clients/wallet/src/resubmissions.rs @@ -221,7 +221,7 @@ impl StellarWallet { return self.bump_sequence_number_and_submit(tx).await } - tracing::error!("handle_tx_bad_seq_error_with_envelope(): Similar transaction already submitted. Skipping {:?}", tx); + tracing::error!("handle_tx_insufficient_fee_error(): Similar transaction already submitted. Skipping {:?}", tx); Err(ResubmissionError("Transaction already submitted".to_string())) }