Merge pull request #1062 from KomodoPlatform/dev

Several hot fixes: mem leak, approve gas limit, segwit tx history.
#1055 #1024 #941

azure-pipelines-build-stage-job.yml

@@ -47,6 +47,7 @@ jobs:
       # Explicit --test-threads=16 makes testing process slightly faster on agents that have <16 CPU cores.
       # Always run tests on mm2.1 branch and PRs
       - bash: |
+          cargo clean -p mm2-libp2p -p mm2
           cargo test --all -- --test-threads=16
         displayName: 'Test MM2'
         timeoutInMinutes: 22
@@ -65,6 +66,7 @@ jobs:
           cargo deny check advisories
         displayName: 'Check Cargo deny advisories'
         condition: eq( variables['Agent.OS'], 'Linux' )
+        enabled: false
       - bash: |
           cargo fmt -- --check
         displayName: 'Check rustfmt warnings'

docs/HEAPTRACK.md

@@ -0,0 +1,21 @@
+# Memory profiling MM2 with heaptrack
+1. Install dependencies required by heaptrack if they are not already installed on the system
+* extra-cmake-modules
+* Qt 5.2 or higher: Core, Widgets
+* KDE Frameworks 5: CoreAddons, I18n, ItemModels, ThreadWeaver, ConfigWidgets, KIO, IconThemes
+
+2. Install heaptrack on Ubuntu (18.04) or higher:
+```
+sudo apt install heaptrack heaptrack-gui
+```
+
+3. Use heaptrack to run MM2 binary and pass parameters as usual. An example for this would be:
+```
+heaptrack ./mm2 "{\"gui\":\"MM2GUI\",\"netid\":7777, \"userhome\":\"/${HOME#"/"}\", \"passphrase\":\"YOUR_PASSPHRASE_HERE\", \"rpc_password\":\"YOUR_PASSWORD_HERE\",\"i_am_seed\":true}" &
+```
+Running heaptrack like this writes a gzipped result file in the same folder the above command ran from. We can now take a look at using the next step.
+
+4. After running MM2 for sometime we can visualize the memory profiling results using the below command. Note that ```heaptrack.mm2.xxxx.gz``` is the name of the file generated through the above command with numbers instead of xxxx
+```
+heaptrack_gui heaptrack.mm2.xxxx.gz
+```

mm2src/coins/eth.rs

@@ -82,6 +82,7 @@ pub const PAYMENT_STATE_SENT: u8 = 1;
 const _PAYMENT_STATE_SPENT: u8 = 2;
 const _PAYMENT_STATE_REFUNDED: u8 = 3;
 const GAS_PRICE_PERCENT: u64 = 10;
+const DEFAULT_LOGS_BLOCK_RANGE: u64 = 1000;
 
 /// Take into account that the dynamic fee may increase by 3% during the swap.
 const GAS_PRICE_APPROXIMATION_PERCENT_ON_START_SWAP: u64 = 3;
@@ -95,9 +96,6 @@ const GAS_PRICE_APPROXIMATION_PERCENT_ON_ORDER_ISSUE: u64 = 5;
 /// - it may increase by 3% during the swap.
 const GAS_PRICE_APPROXIMATION_PERCENT_ON_TRADE_PREIMAGE: u64 = 7;
 
-const APPROVE_GAS_LIMIT: u64 = 50_000;
-const DEFAULT_LOGS_BLOCK_RANGE: u64 = 1000;
-
 lazy_static! {
     pub static ref SWAP_CONTRACT: Contract = Contract::load(SWAP_CONTRACT_ABI.as_bytes()).unwrap();
     pub static ref ERC20_CONTRACT: Contract = Contract::load(ERC20_ABI.as_bytes()).unwrap();
@@ -1390,17 +1388,17 @@ impl EthCoin {
                 let arc = self.clone();
                 Box::new(allowance_fut.and_then(move |allowed| -> EthTxFut {
                     if allowed < value {
-                        let balance_f = arc.my_balance();
-                        Box::new(balance_f.map_err(|e| ERRL!("{}", e)).and_then(move |balance| {
-                            arc.approve(swap_contract_address, balance).and_then(move |_approved| {
-                                arc.sign_and_send_transaction(
-                                    0.into(),
-                                    Action::Call(swap_contract_address),
-                                    data,
-                                    U256::from(150_000),
-                                )
-                            })
-                        }))
+                        Box::new(
+                            arc.approve(swap_contract_address, U256::max_value())
+                                .and_then(move |_approved| {
+                                    arc.sign_and_send_transaction(
+                                        0.into(),
+                                        Action::Call(swap_contract_address),
+                                        data,
+                                        U256::from(150_000),
+                                    )
+                                }),
+                        )
                     } else {
                         Box::new(arc.sign_and_send_transaction(
                             0.into(),
@@ -1591,6 +1589,34 @@ impl EthCoin {
         Box::new(fut.boxed().compat())
     }
 
+    /// Estimates how much gas is necessary to allow the contract call to complete.
+    /// `contract_addr` can be a ERC20 token address or any other contract address.
+    ///
+    /// # Important
+    ///
+    /// Don't use this method to estimate gas for a withdrawal of `ETH` coin.
+    /// For more details, see `withdraw_impl`.
+    ///
+    /// Also, note that the contract call has to be initiated by my wallet address,
+    /// because [`CallRequest::from`] is set to [`EthCoinImpl::my_address`].
+    fn estimate_gas_for_contract_call(&self, contract_addr: Address, call_data: Bytes) -> Web3RpcFut<U256> {
+        let coin = self.clone();
+        Box::new(coin.get_gas_price().and_then(move |gas_price| {
+            let eth_value = U256::zero();
+            let estimate_gas_req = CallRequest {
+                value: Some(eth_value),
+                data: Some(call_data),
+                from: Some(coin.my_address),
+                to: contract_addr,
+                gas: None,
+                // gas price must be supplied because some smart contracts base their
+                // logic on gas price, e.g. TUSD: https://github.com/KomodoPlatform/atomicDEX-API/issues/643
+                gas_price: Some(gas_price),
+            };
+            coin.estimate_gas(estimate_gas_req).map_to_mm_fut(Web3RpcError::from)
+        }))
+    }
+
     fn eth_balance(&self) -> BalanceFut<U256> {
         Box::new(
             self.web3
@@ -1646,18 +1672,27 @@ impl EthCoin {
     }
 
     fn approve(&self, spender: Address, amount: U256) -> EthTxFut {
-        match &self.coin_type {
-            EthCoinType::Eth => panic!(),
-            EthCoinType::Erc20 {
-                platform: _,
-                token_addr,
-            } => {
-                let function = try_fus!(ERC20_CONTRACT.function("approve"));
-                let data = try_fus!(function.encode_input(&[Token::Address(spender), Token::Uint(amount),]));
+        let coin = self.clone();
+        let fut = async move {
+            let token_addr = match coin.coin_type {
+                EthCoinType::Eth => return ERR!("'approve' is expected to be call for ERC20 coins only"),
+                EthCoinType::Erc20 { token_addr, .. } => token_addr,
+            };
+            let function = try_s!(ERC20_CONTRACT.function("approve"));
+            let data = try_s!(function.encode_input(&[Token::Address(spender), Token::Uint(amount)]));
 
-                self.sign_and_send_transaction(0.into(), Action::Call(*token_addr), data, U256::from(APPROVE_GAS_LIMIT))
-            },
-        }
+            let gas_limit = try_s!(
+                coin.estimate_gas_for_contract_call(token_addr, Bytes::from(data.clone()))
+                    .compat()
+                    .await
+            );
+
+            coin.sign_and_send_transaction(0.into(), Action::Call(token_addr), data, gas_limit)
+                .compat()
+                .await
+                .map_err(|e| ERRL!("{}", e))
+        };
+        Box::new(fut.boxed().compat())
     }
 
     /// Gets `PaymentSent` events from etomic swap smart contract since `from_block`
@@ -2797,16 +2832,28 @@ impl MmCoin for EthCoin {
                     // this gas_limit includes gas for `ethPayment` and `senderRefund` contract calls
                     U256::from(300_000)
                 },
-                EthCoinType::Erc20 { .. } => {
+                EthCoinType::Erc20 { token_addr, .. } => {
                     let value = match value {
                         TradePreimageValue::Exact(value) | TradePreimageValue::UpperBound(value) => {
                             wei_from_big_decimal(&value, coin.decimals)?
                         },
                     };
                     let allowed = coin.allowance(coin.swap_contract_address).compat().await?;
                     if allowed < value {
+                        // estimate gas for the `approve` contract call
+
+                        // Pass a dummy spender. Let's use `my_address`.
+                        let spender = coin.my_address;
+                        let approve_function = ERC20_CONTRACT.function("approve")?;
+                        let approve_data =
+                            approve_function.encode_input(&[Token::Address(spender), Token::Uint(value)])?;
+                        let approve_gas_limit = coin
+                            .estimate_gas_for_contract_call(token_addr, Bytes::from(approve_data))
+                            .compat()
+                            .await?;
+
                         // this gas_limit includes gas for `approve`, `erc20Payment` and `senderRefund` contract calls
-                        U256::from(300_000 + APPROVE_GAS_LIMIT)
+                        U256::from(300_000) + approve_gas_limit
                     } else {
                         // this gas_limit includes gas for `erc20Payment` and `senderRefund` contract calls
                         U256::from(300_000)

mm2src/coins/eth/eth_tests.rs

@@ -756,11 +756,18 @@ fn get_sender_trade_preimage() {
 
 #[test]
 fn get_erc20_sender_trade_preimage() {
+    const APPROVE_GAS_LIMIT: u64 = 60_000;
     static mut ALLOWANCE: u64 = 0;
+    static mut ESTIMATE_GAS_CALLED: bool = false;
+
     EthCoin::allowance
         .mock_safe(|_, _| MockResult::Return(Box::new(futures01::future::ok(unsafe { ALLOWANCE.into() }))));
 
     EthCoinImpl::get_gas_price.mock_safe(|_| MockResult::Return(Box::new(futures01::future::ok(GAS_PRICE.into()))));
+    EthCoinImpl::estimate_gas.mock_safe(|_, _| {
+        unsafe { ESTIMATE_GAS_CALLED = true };
+        MockResult::Return(Box::new(futures01::future::ok(APPROVE_GAS_LIMIT.into())))
+    });
 
     fn expected_trade_fee(gas_limit: u64, gas_price: u64) -> TradeFee {
         let amount = u256_to_big_decimal((gas_limit * gas_price).into(), 18).expect("!u256_to_big_decimal");
@@ -788,6 +795,7 @@ fn get_erc20_sender_trade_preimage() {
         .wait()
         .expect("!get_sender_trade_fee");
     log!([actual.amount.to_decimal()]);
+    unsafe { assert!(!ESTIMATE_GAS_CALLED) }
     assert_eq!(actual, expected_trade_fee(300_000, GAS_PRICE));
 
     // value is greater than allowance
@@ -797,9 +805,13 @@ fn get_erc20_sender_trade_preimage() {
         .get_sender_trade_fee(TradePreimageValue::UpperBound(value), FeeApproxStage::StartSwap)
         .wait()
         .expect("!get_sender_trade_fee");
+    unsafe {
+        assert!(ESTIMATE_GAS_CALLED);
+        ESTIMATE_GAS_CALLED = false;
+    }
     assert_eq!(
         actual,
-        expected_trade_fee(350_000, GAS_PRICE_APPROXIMATION_ON_START_SWAP)
+        expected_trade_fee(360_000, GAS_PRICE_APPROXIMATION_ON_START_SWAP)
     );
 
     // value is allowed
@@ -809,6 +821,7 @@ fn get_erc20_sender_trade_preimage() {
         .get_sender_trade_fee(TradePreimageValue::Exact(value), FeeApproxStage::OrderIssue)
         .wait()
         .expect("!get_sender_trade_fee");
+    unsafe { assert!(!ESTIMATE_GAS_CALLED) }
     assert_eq!(
         actual,
         expected_trade_fee(300_000, GAS_PRICE_APPROXIMATION_ON_ORDER_ISSUE)
@@ -821,9 +834,13 @@ fn get_erc20_sender_trade_preimage() {
         .get_sender_trade_fee(TradePreimageValue::Exact(value), FeeApproxStage::TradePreimage)
         .wait()
         .expect("!get_sender_trade_fee");
+    unsafe {
+        assert!(ESTIMATE_GAS_CALLED);
+        ESTIMATE_GAS_CALLED = false;
+    }
     assert_eq!(
         actual,
-        expected_trade_fee(350_000, GAS_PRICE_APPROXIMATION_ON_TRADE_PREIMAGE)
+        expected_trade_fee(360_000, GAS_PRICE_APPROXIMATION_ON_TRADE_PREIMAGE)
     );
 }
 

mm2src/coins/utxo.rs

@@ -1163,6 +1163,23 @@ impl<'a> UtxoConfBuilder<'a> {
     fn estimate_fee_blocks(&self) -> u32 { json::from_value(self.conf["estimate_fee_blocks"].clone()).unwrap_or(1) }
 }
 
+#[derive(Debug)]
+pub struct ElectrumBuilderArgs {
+    pub spawn_ping: bool,
+    pub negotiate_version: bool,
+    pub collect_metrics: bool,
+}
+
+impl Default for ElectrumBuilderArgs {
+    fn default() -> Self {
+        ElectrumBuilderArgs {
+            spawn_ping: true,
+            negotiate_version: true,
+            collect_metrics: true,
+        }
+    }
+}
+
 #[async_trait]
 pub trait UtxoCoinBuilder {
     type ResultCoin;
@@ -1321,21 +1338,27 @@ pub trait UtxoCoinBuilder {
                 }
             },
             Some("electrum") => {
-                let electrum = try_s!(self.electrum_client().await);
+                let electrum = try_s!(self.electrum_client(ElectrumBuilderArgs::default()).await);
                 Ok(UtxoRpcClientEnum::Electrum(electrum))
             },
             _ => ERR!("Expected enable or electrum request"),
         }
     }
 
-    async fn electrum_client(&self) -> Result<ElectrumClient, String> {
+    async fn electrum_client(&self, args: ElectrumBuilderArgs) -> Result<ElectrumClient, String> {
         let (on_connect_tx, on_connect_rx) = mpsc::unbounded();
         let ticker = self.ticker().to_owned();
         let ctx = self.ctx();
-        let event_handlers = vec![
-            CoinTransportMetrics::new(ctx.metrics.weak(), ticker.clone(), RpcClientType::Electrum).into_shared(),
-            ElectrumProtoVerifier { on_connect_tx }.into_shared(),
-        ];
+        let mut event_handlers = vec![];
+        if args.collect_metrics {
+            event_handlers.push(
+                CoinTransportMetrics::new(ctx.metrics.weak(), ticker.clone(), RpcClientType::Electrum).into_shared(),
+            );
+        }
+
+        if args.negotiate_version {
+            event_handlers.push(ElectrumProtoVerifier { on_connect_tx }.into_shared());
+        }
 
         let mut servers: Vec<ElectrumRpcRequest> = try_s!(json::from_value(self.req()["servers"].clone()));
         let mut rng = small_rng();
@@ -1360,14 +1383,18 @@ pub trait UtxoCoinBuilder {
 
         let client = Arc::new(client);
 
-        let weak_client = Arc::downgrade(&client);
-        let client_name = format!("{} GUI/MM2 {}", ctx.gui().unwrap_or("UNKNOWN"), ctx.mm_version());
-        spawn_electrum_version_loop(weak_client, on_connect_rx, client_name);
+        if args.negotiate_version {
+            let weak_client = Arc::downgrade(&client);
+            let client_name = format!("{} GUI/MM2 {}", ctx.gui().unwrap_or("UNKNOWN"), ctx.mm_version());
+            spawn_electrum_version_loop(weak_client, on_connect_rx, client_name);
 
-        try_s!(wait_for_protocol_version_checked(&client).await);
+            try_s!(wait_for_protocol_version_checked(&client).await);
+        }
 
-        let weak_client = Arc::downgrade(&client);
-        spawn_electrum_ping_loop(weak_client, servers);
+        if args.spawn_ping {
+            let weak_client = Arc::downgrade(&client);
+            spawn_electrum_ping_loop(weak_client, servers);
+        }
 
         Ok(ElectrumClient(client))
     }
@@ -1996,6 +2023,7 @@ pub fn output_script(address: &Address, script_type: ScriptType) -> Script {
         _ => match script_type {
             ScriptType::P2PKH => Builder::build_p2pkh(&address.hash),
             ScriptType::P2SH => Builder::build_p2sh(&address.hash),
+            ScriptType::P2WPKH => Builder::build_p2wpkh(&address.hash),
         },
     }
 }