[r2r] UTXO RPC batch requests

[sergeyboyko0791]

Optimize requesting balances of HD addresses by using RPC batch request.

• Optimize requesting balances on init_utxo, init_qtum RPCs by using batch requests
• Optimize addresses_balances RPC
• Refactor scan_for_new_addresses RPC by spawning it as an RPC task, rename the method to init_scan_for_new_addresses
• Refactor list_mature_unspent_ordered, list_all_unspent_ordered, list_unspent_ordered from UtxoCommonOps into the following:
  • ListUtxoOps::get_mature_unspent_ordered_map - returns available mature and immature unspents in ascending order for every given addresses: Vec<Address>
  • ListUtxoOps::get_all_unspent_ordered_map - returns available unspents in ascending order for every given Vec<Address>
  • ListUtxoOps::get_unspent_ordered_list - returns available unspents in ascending order
  • ListUtxoOps::get_unspent_ordered_map - returns available unspents in ascending order for every given Vec<Address>
• Refactor UtxoCommonOps::get_verbose_transaction_from_cache_or_rpc to UtxoCommonOps::get_verbose_transactions_from_cache_or_rpc. The method requests multiple transactions concurrently
• Add UtxoRpcClientOps::list_unspent_group, UtxoRpcClientOps::get_verbose_transactions, UtxoRpcClientOps::display_balances

[sergeyboyko0791]

Do we need to use tmp files when save orders?

[sergeyboyko0791]

Same question

[shamardy]

Great Work! Just a few minor comments

[artemii235]

First review iteration.

[artemii235]

ZCoin is not assumed to work with transparent UTXOs. This trait should not be implemented for it.

[sergeyboyko0791]

Unfortunately, ZCoin::preimage_trade_fee_required_to_send_outputs is based on utxo_common::preimage_trade_fee_required_to_send_outputs, that requires ListUtxoOps to generate a preimage transaction.
https://github.com/KomodoPlatform/atomicDEX-API/blob/778461afccf42c17da0e02394ef6890a092a7a99/mm2src/coins/z_coin.rs#L1425-L1433

So I think we need to implement this method requesting z_unspents instead and then we can get rid of the ListUtxoOps trait implementation for ZCoin.
Do you think I need to do it within this PR?

[artemii235]

I can refactor it during next iteration of Zcoin implementation. Not required for now.

[artemii235]

But I already have a different idea: if only one method from a trait is required to split these implementations, then we can maybe add a trait like TradePreimageUtxoOps and implement only it for Zcoin? I didn't dig in the code regarding this, so not sure how feasible it will be. @sergeyboyko0791

[sergeyboyko0791]

Could you please explain more how TradePreimageUtxoOps can allow us to avoid implementing ListUtxoOps for ZCoin? 😅

[artemii235]

I need to check this in more details myself. It will require some time to perform this refactoring, so we can leave this as is for now.

[sergeyboyko0791]

@artemii235 @shamardy @ozkanonur @caglaryucekaya PR is ready for the review

[artemii235]

Few changes requests and questions.

[artemii235]

To avoid duplicated if unspent.outpoint.index == 0 check, we can use something like

        let mut result = BchUnspents::default();
        let mut temporary_undetermined = Vec::new();
        let to_verbose: HashSet<H256Json> = utxos
            .into_iter()
            .filter_map(|unspent| {
                if unspent.outpoint.index == 0 {
                    // Zero output is reserved for OP_RETURN of specific protocols
                    // so if we get it we can safely consider this as standard BCH UTXO.
                    // There is no need to request verbose transaction for such UTXO.
                    result.add_standard(unspent);
                    None
                } else {
                    let hash = unspent.outpoint.hash.reversed().into();
                    temporary_undetermined.push(unspent);
                    Some(hash)
                }
            })
            .collect();
        let verbose_txs = self
            .get_verbose_transactions_from_cache_or_rpc(to_verbose)
            .compat()
            .await?;

        for unspent in temporary_undetermined {
        ...
        }

What do you think?

[sergeyboyko0791]

I tried to do something like this, but couldn't do it nicely. Will change it, thanks!

[sergeyboyko0791]

Done

[artemii235]

It looks like the iterator is sending actual requests while it's responsibility of rpc_client. It can also be confused with combinators like map/filter/etc.. So the interface should be as follows:

fn batch_rpc<Rpc, T>(&self (Rpc), requests: I) -> RpcRes<Vec<T>>

[sergeyboyko0791]

Yes, this makes sense! I wanted to use the trait this way:

tx_ids
                .iter()
                .map(|txid| rpc_req!(self, "getrawtransaction", txid, verbose))
                .batch_rpc(self)

But I agree that it might be confusing.

[sergeyboyko0791]

Done

[artemii235]

RecentlySpentOutPoints support only single for_script_pubkey now. It has to be refactored for HD wallets support. Do you plan to do it on next iterations?

[sergeyboyko0791]

Yes, there is a note here. I'm still thinking about the refactoring. Can I refactor it at the next iteration with tx history?

[artemii235]

Yes 🙂

[artemii235]

If this can be reused somewhere else, it's preferred to move it outside another fn.

[sergeyboyko0791]

I think address_balance_from_unspent_map is pinned to the NativeClient::display_balances, but I think this can be reused later somewhere:

unspents.iter().fold(BigDecimal::from(0), |sum, unspent| {
                sum + big_decimal_from_sat_unsigned(unspent.value, decimals)
            })

[sergeyboyko0791]

Moved address_balance_from_unspent_map outside the NativeClient::display_balances method

[onur-ozkan]

First review iteration.

First of all great work with documenting the codebase! :) I tried to not spam the same topic in the PR comments. So please consider them as general optimizations like inlining public functions, removing the unneeded variables, etc.

About removing the variable, I think we should avoid creating variables unless the code will be very hard to read and maintain. Compilers these days are trying to solve this in order to avoid memory preasure, but it's not guaranteed so we can't rely on that specially in projects like this.

[onur-ozkan]

How about creating a constant for "TX_CACHE", I see it's used multiple times. Would be better if we have constant for this value so we don't need to make so many diffs in order to update this value.

[sergeyboyko0791]

Will it be ok for you if I do the following (but without the constant)?

    #[cfg(not(target_arch = "wasm32"))]
    fn tx_cache(&self) -> UtxoVerboseCacheShared {
        crate::utxo::tx_cache::fs_tx_cache::FsVerboseCache::new(self.ticker().to_owned(), self.tx_cache_path())
            .into_shared()
    }

    fn tx_cache_path(&self) -> PathBuf { self.ctx().dbdir().join("TX_CACHE") }

[onur-ozkan]

question

I see the functionality has been changed a little bit in this test function. Is that how it's supposed to be?

[sergeyboyko0791]

Sorry, I didn't understand your question. If I'm not mistaken, neither test_withdraw_impl_fee_details nor Qrc20Coin::get_unspent_ordered_list haven't been changed.

What do you mean? 😅

the functionality has been changed a little bit in this test function

[onur-ozkan]

What I mean is list_mature_unspent_ordered renamed into get_mature_unspent_ordered_list, but you used get_unspent_ordered_list . I wanted to know if you did it on purpose, I thought you might be confused about the function name. I guess you did it on purpose so no problem 👍

[sergeyboyko0791]

I renamed it because there is similar method that returns UnspentMap of address -> UnspentList. So map_mature_unspent_ordered doesn't fit its purpose, so I had to rename all of those methods to get_[all/mature]_unspent_ordered_[list/map] 😁

[onur-ozkan]

This can be optimized in a following way:

#[inline]
pub async fn account_balance(
    ctx: MmArc,
    req: HDAccountBalanceRequest,
) -> MmResult<HDAccountBalanceResponse, HDAccountBalanceRpcError> {
    match lp_coinfind_or_err(&ctx, &req.coin).await? {
        MmCoinEnum::UtxoCoin(utxo) => utxo.account_balance_rpc(req.params).await,
        MmCoinEnum::QtumCoin(qtum) => qtum.account_balance_rpc(req.params).await,
        _ => MmError::err(HDAccountBalanceRpcError::CoinIsActivatedNotWithHDWallet),
    }
}

[sergeyboyko0791]

Done

[onur-ozkan]

I would prefer the shorter way as in the following diff:

@@ -73,14 +73,15 @@ pub mod common_impl {
         Coin: HDWalletBalanceOps + CoinWithDerivationMethod<HDWallet = <Coin as HDWalletCoinOps>::HDWallet> + Sync,
         <Coin as HDWalletCoinOps>::Address: fmt::Display + Clone,
     {
-        let hd_wallet = coin.derivation_method().hd_wallet_or_err()?;
-
-        let account_id = params.account_index;
-        let chain = params.chain;
-        let hd_account = hd_wallet
-            .get_account(account_id)
+        let hd_account = coin
+            .derivation_method()
+            .hd_wallet_or_err()?
+            .get_account(params.account_index)
             .await
-            .or_mm_err(|| HDAccountBalanceRpcError::UnknownAccount { account_id })?;
+            .or_mm_err(|| HDAccountBalanceRpcError::UnknownAccount {
+                account_id: params.account_index,
+            })?;
+
         let total_addresses_number = hd_account.known_addresses_number(params.chain)?;

         let from_address_id = match params.paging_options {
@@ -90,13 +91,13 @@ pub mod common_impl {
         let to_address_id = std::cmp::min(from_address_id + params.limit as u32, total_addresses_number);

         let addresses = coin
-            .known_addresses_balances_with_ids(&hd_account, chain, from_address_id..to_address_id)
+            .known_addresses_balances_with_ids(&hd_account, params.chain, from_address_id..to_address_id)
             .await?;
         let page_balance = addresses.iter().fold(CoinBalance::default(), |total, addr_balance| {
             total + addr_balance.balance.clone()
         });

-        let result = HDAccountBalanceResponse {
+        Ok(HDAccountBalanceResponse {
             account_index: params.account_index,
             derivation_path: RpcDerivationPath(hd_account.account_derivation_path()),
             addresses,
@@ -106,8 +107,6 @@ pub mod common_impl {
             total: total_addresses_number,
             total_pages: calc_total_pages(total_addresses_number as usize, params.limit),
             paging_options: params.paging_options,
-        };
-
-        Ok(result)
+        })
     }
 }

to avoid from memory pressure and doing extra process executions. Do you think that this is way more complicated in order to read and maintain code?

[sergeyboyko0791]

No, I think it doesn't make the code more complicated. Will do 🙂

[sergeyboyko0791]

Refactored, but I left

let account_id = params.account_index;

variable, because it's used here

-            .or_mm_err(|| HDAccountBalanceRpcError::UnknownAccount { account_id })?;
+            .or_mm_err(|| HDAccountBalanceRpcError::UnknownAccount {
+                account_id: params.account_index,
+            })?;

[onur-ozkan]

Since the error is only used once, maybe it's better to not create variable and directly do e.to_string() here.

[sergeyboyko0791]

e is moved at this moment. There are other ways to convert the error to string on NoSuchTask and UnexpectedTaskStatus only:

        match e {
            RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()),
            RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout),
            error @ RpcTaskError::NoSuchTask(_) | error @ RpcTaskError::UnexpectedTaskStatus { .. } => {
                HDAccountBalanceRpcError::Internal(error.to_string())
            },
            RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal),
        }

or

        match e {
            RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()),
            RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout),
            RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal),
            error => HDAccountBalanceRpcError::Internal(error.to_string()),
        }

I'd prefer the first solution but it looks scary a bit 😅
What do you think?

[onur-ozkan]

e is moved at this moment. There are other ways to convert the error to string on NoSuchTask and UnexpectedTaskStatus only:

        match e {
            RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()),
            RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout),
            error @ RpcTaskError::NoSuchTask(_) | error @ RpcTaskError::UnexpectedTaskStatus { .. } => {
                HDAccountBalanceRpcError::Internal(error.to_string())
            },
            RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal),
        }

or

        match e {
            RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()),
            RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout),
            RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal),
            error => HDAccountBalanceRpcError::Internal(error.to_string()),
        }

I'd prefer the first solution but it looks scary a bit sweat_smile What do you think?

How about the following example:

@@ -94,14 +94,13 @@ impl From<AddressDerivingError> for HDAccountBalanceRpcError {

 impl From<RpcTaskError> for HDAccountBalanceRpcError {
     fn from(e: RpcTaskError) -> Self {
-        let error = e.to_string();
-        match e {
+        match &e {
             RpcTaskError::Canceled => HDAccountBalanceRpcError::Internal("Canceled".to_owned()),
-            RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(timeout),
+            RpcTaskError::Timeout(timeout) => HDAccountBalanceRpcError::Timeout(*timeout),
             RpcTaskError::NoSuchTask(_) | RpcTaskError::UnexpectedTaskStatus { .. } => {
-                HDAccountBalanceRpcError::Internal(error)
+                HDAccountBalanceRpcError::Internal(e.to_string())
             },
-            RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal),
+            RpcTaskError::Internal(internal) => HDAccountBalanceRpcError::Internal(internal.to_string()),
         }
     }
 }

[onur-ozkan]

Also, currently RpcTaskError::Internal(internal) => ... already does the partially move.

[sergeyboyko0791]

Done
I figured out the following:

let res: Result<String, String> = Err("foo".to_owned());
let own_value: String = match res {
  Ok(own_ok) => own_ok,
  // `_` is syntactic sugar that doesn't take ownership of `res`,
  // so `res` can be used in this branch as it's not even borrowed.
  Err(_) => res.expect_err(""),
};

[onur-ozkan]

Can you please specify a short doc something like:

/// Checks if `fn get_all_unspent_ordered_list` is called or not.

It could be a little bit hard to understand at the first try for newbies(like me).

[sergeyboyko0791]

Ok, sure :)

[sergeyboyko0791]

Done

[onur-ozkan]

I would put expected value right before the assertion rather than in the middle of the test flow. Just my opinion, not necessary change. :)

[sergeyboyko0791]

The expected value is used in the line below
https://github.com/KomodoPlatform/atomicDEX-API/blob/7224b85e39a2406b5b9e8cb9a8e69cd59fda514f/mm2src/coins/utxo/utxo_tests.rs#L3793-L3800

This could be done in more lines:

let addresses = vec![
        "RG278CfeNPFtNztFZQir8cgdWexVhViYVy".into(),
        "RYPz6Lr4muj4gcFzpMdv3ks1NCGn3mkDPN".into(),
        "RJeDDtDRtKUoL8BCKdH7TNCHqUKr7kQRsi".into(),
        "RQHn9VPHBqNjYwyKfJbZCiaxVrWPKGQjeF".into(),
    ];

    let addresses = expected.iter().map(|(address, _)| address.clone()).collect();
    let actual = rpc_client
        .display_balances(addresses, TEST_COIN_DECIMALS)
        .wait()
        .unwrap();

    let expected: Vec<(Address, BigDecimal)> = vec![
        ("RG278CfeNPFtNztFZQir8cgdWexVhViYVy".into(), BigDecimal::from(5.77699)),
        ("RYPz6Lr4muj4gcFzpMdv3ks1NCGn3mkDPN".into(), BigDecimal::from(0)),
        ("RJeDDtDRtKUoL8BCKdH7TNCHqUKr7kQRsi".into(), BigDecimal::from(0.77699)),
        ("RQHn9VPHBqNjYwyKfJbZCiaxVrWPKGQjeF".into(), BigDecimal::from(0.99998)),
    ];
    assert_eq!(actual, expected);

What do you think?

[onur-ozkan]

Looks okay. You can consider also the following:

fn test_native_display_balances() {
     let rpc_client = native_client_for_test();

-    let expected: Vec<(Address, BigDecimal)> = vec![
-        ("RG278CfeNPFtNztFZQir8cgdWexVhViYVy".into(), BigDecimal::from(5.77699)),
-        ("RYPz6Lr4muj4gcFzpMdv3ks1NCGn3mkDPN".into(), BigDecimal::from(0)),
-        ("RJeDDtDRtKUoL8BCKdH7TNCHqUKr7kQRsi".into(), BigDecimal::from(0.77699)),
-        ("RQHn9VPHBqNjYwyKfJbZCiaxVrWPKGQjeF".into(), BigDecimal::from(0.99998)),
+    let addresses: Vec<Address> = vec![
+        "RG278CfeNPFtNztFZQir8cgdWexVhViYVy".into(),
+        "RYPz6Lr4muj4gcFzpMdv3ks1NCGn3mkDPN".into(),
+        "RJeDDtDRtKUoL8BCKdH7TNCHqUKr7kQRsi".into(),
+        "RQHn9VPHBqNjYwyKfJbZCiaxVrWPKGQjeF".into(),
     ];

-    let addresses = expected.iter().map(|(address, _)| address.clone()).collect();
     let actual = rpc_client
-        .display_balances(addresses, TEST_COIN_DECIMALS)
+        .display_balances(addresses.clone(), TEST_COIN_DECIMALS)
         .wait()
         .unwrap();

+    let expected_balances: Vec<BigDecimal> = vec![
+        BigDecimal::from(5.77699 as f64),
+        BigDecimal::from(0 as i32),
+        BigDecimal::from(0.77699 as f64),
+        BigDecimal::from(0.99998 as f64),
+    ];
+
+    assert_eq!(addresses.len(), expected_balances.len());
+
+    let mut expected: Vec<(Address, BigDecimal)> = Vec::new();
+    addresses.iter().enumerate().for_each(|(index, address)| {
+        expected.push((*address, expected_balances[index]));
+    });
+
     assert_eq!(actual, expected);
 }

[sergeyboyko0791]

Isn't my solution more compact and easy to read? 😅

[onur-ozkan]

I just wanted to share a code flow from another aspect, all looks confirmable. The decision is up to you, I am fine with any of them :)

[sergeyboyko0791]

Done

[onur-ozkan]

Can be simplified as:

hash_unspents
    .into_iter()
    .unique_by(|unspent| (unspent.tx_hash, unspent.tx_pos))
    .collect::<Vec<_>>()

[sergeyboyko0791]

Oooh right... Thanks)

[sergeyboyko0791]

Done

[sergeyboyko0791]

@artemii235 @ozkanonur @shamardy the PR is ready for re-review. Please check if I resolved all issues

[shamardy]

LGTM! Only one non-blocker comment about inline(always) #1255 (comment)

[onur-ozkan]

Looks good to me. Great work!

[artemii235]

One question and two changes.

[artemii235]

👍