Merge #1201: Reduce amount selected for use in transactions

Pull request description:

  Calculate the specific amount and fee required when creating an Omni transaction.

src/omnicore/encoding.cpp

@@ -23,7 +23,7 @@
  * https://github.com/mastercoin-MSC/spec#class-b-transactions-also-known-as-the-multisig-method
  */
 bool OmniCore_Encode_ClassB(const std::string& senderAddress, const CPubKey& redeemingPubKey,
-        const std::vector<unsigned char>& vchPayload, std::vector<std::pair<CScript, int64_t> >& vecOutputs)
+        const std::vector<unsigned char>& vchPayload, std::vector<std::pair<CScript, int64_t> >& vecOutputs, CAmount *outputAmount)
 {
     unsigned int nRemainingBytes = vchPayload.size();
     unsigned int nNextByte = 0;
@@ -65,11 +65,15 @@ bool OmniCore_Encode_ClassB(const std::string& senderAddress, const CPubKey& red
 
         // Push back a bare multisig output with obfuscated data
         CScript scriptMultisigOut = GetScriptForMultisig(1, vKeys);
+        if (outputAmount)
+            *outputAmount += OmniGetDustThreshold(scriptMultisigOut);
         vecOutputs.push_back(std::make_pair(scriptMultisigOut, OmniGetDustThreshold(scriptMultisigOut)));
     }
 
     // Add the Exodus marker output
     CScript scriptExodusOutput = GetScriptForDestination(ExodusAddress());
+    if (outputAmount)
+        *outputAmount += OmniGetDustThreshold(scriptExodusOutput);
     vecOutputs.push_back(std::make_pair(scriptExodusOutput, OmniGetDustThreshold(scriptExodusOutput)));
     return true;
 }

src/omnicore/encoding.h

@@ -4,6 +4,7 @@
 class CPubKey;
 class CTxOut;
 
+#include <amount.h>
 #include <script/script.h>
 
 #include <stdint.h>
@@ -12,7 +13,8 @@ class CTxOut;
 #include <vector>
 
 /** Embeds a payload in obfuscated multisig outputs, and adds an Exodus marker output. */
-bool OmniCore_Encode_ClassB(const std::string& senderAddress, const CPubKey& redeemingPubKey, const std::vector<unsigned char>& vchPayload, std::vector<std::pair<CScript, int64_t> >& vecOutputs);
+bool OmniCore_Encode_ClassB(const std::string& senderAddress, const CPubKey& redeemingPubKey, const std::vector<unsigned char>& vchPayload,
+                            std::vector<std::pair<CScript, int64_t> >& vecOutputs, CAmount* outputAmount = nullptr);
 
 /** Embeds a payload in an OP_RETURN output, prefixed with a transaction marker. */
 bool OmniCore_Encode_ClassC(const std::vector<unsigned char>& vecPayload, std::vector<std::pair<CScript, int64_t> >& vecOutputs);

src/omnicore/wallettxbuilder.cpp

@@ -66,8 +66,8 @@ int WalletTxBuilder(
     // Next, we set the change address to the sender
     coinControl.destChange = DecodeDestination(senderAddress);
 
-    // Select the inputs
-    if (0 > mastercore::SelectCoins(*iWallet, senderAddress, coinControl, referenceAmount)) { return MP_INPUTS_INVALID; }
+    // Amount required for outputs
+    CAmount outputAmount{0};
 
     // Encode the data outputs
     switch(omniTxClass) {
@@ -77,7 +77,7 @@ int WalletTxBuilder(
             if (!AddressToPubKey(iWallet, sAddress, redeemingPubKey)) {
                 return MP_REDEMP_BAD_VALIDATION;
             }
-            if (!OmniCore_Encode_ClassB(senderAddress,redeemingPubKey,payload,vecSend)) { return MP_ENCODING_ERROR; }
+            if (!OmniCore_Encode_ClassB(senderAddress, redeemingPubKey, payload, vecSend, &outputAmount)) { return MP_ENCODING_ERROR; }
         break; }
         case OMNI_CLASS_C:
             if(!OmniCore_Encode_ClassC(payload,vecSend)) { return MP_ENCODING_ERROR; }
@@ -87,25 +87,52 @@ int WalletTxBuilder(
     // Then add a paytopubkeyhash output for the recipient (if needed) - note we do this last as we want this to be the highest vout
     if (!receiverAddress.empty()) {
         CScript scriptPubKey = GetScriptForDestination(DecodeDestination(receiverAddress));
+        outputAmount += 0 < referenceAmount ? referenceAmount : OmniGetDustThreshold(scriptPubKey);
         vecSend.push_back(std::make_pair(scriptPubKey, 0 < referenceAmount ? referenceAmount : OmniGetDustThreshold(scriptPubKey)));
     }
 
-    // Now we have what we need to pass to the wallet to create the transaction, perform some checks first
-
-    if (!coinControl.HasSelected()) return MP_ERR_INPUTSELECT_FAIL;
-
+    // Create CRecipients for outputs
     std::vector<CRecipient> vecRecipients;
     for (size_t i = 0; i < vecSend.size(); ++i) {
         const std::pair<CScript, int64_t>& vec = vecSend[i];
         CRecipient recipient = {vec.first, vec.second, false};
         vecRecipients.push_back(recipient);
     }
 
-    // Ask the wallet to create the transaction (note mining fee determined by Bitcoin Core params)
-    CAmount nFeeRet = 0;
-    int nChangePosInOut = -1;
+    CAmount nFeeRequired{std::max(minFee, iWallet->getMinimumFee(1000, coinControl, nullptr, nullptr))};
+    CTransactionRef wtxNew;
     std::string strFailReason;
-    auto wtxNew = iWallet->createTransaction(vecRecipients, coinControl, true /* sign */, nChangePosInOut, nFeeRet, strFailReason, false, minFee);
+    CAmount nFeeRet{0};
+    bool createTX{true};
+
+    while (createTX) {
+        // Select the inputs
+        auto selected = mastercore::SelectCoins(*iWallet, senderAddress, coinControl, outputAmount + nFeeRequired);
+
+        // Did not select anything at all!
+        if (!coinControl.HasSelected()) {
+            return MP_ERR_INPUTSELECT_FAIL;
+        }
+
+        // Could not select to enough to cover outputs and fee
+        if (selected < outputAmount) {
+            return MP_INPUTS_INVALID;
+        }
+
+        // Ask the wallet to create the transaction (note mining fee determined by Bitcoin Core params)
+        int nChangePosInOut = -1;
+        wtxNew = iWallet->createTransaction(vecRecipients, coinControl, true /* sign */, nChangePosInOut, nFeeRet, strFailReason, false, minFee);
+
+        // TX creation was a success or fee no longer incremeneintg
+        if (wtxNew || nFeeRet <= nFeeRequired) {
+            createTX = false;
+        } else {
+            // Set new fee required
+            nFeeRequired = nFeeRet;
+
+            PrintToLog("Increasing fee. nFeeRequired: %d selected: %d outputAmount: %d\n", nFeeRequired, selected, outputAmount);
+        }
+    }
 
     if (!wtxNew) {
         PrintToLog("%s: ERROR: wallet transaction creation failed: %s\n", __func__, strFailReason);
@@ -387,26 +414,45 @@ int CreateDExTransaction(interfaces::Wallet* pwallet, const std::string& buyerAd
     // Calculate dust for Exodus output
     CAmount dust = OmniGetDustThreshold(exodus);
 
-    // Select the inputs required to cover amount, dust and fees
-    if (0 > mastercore::SelectCoins(*pwallet, buyerAddress, coinControl, nAmount + dust)) {
-        return MP_INPUTS_INVALID;
-    }
-
-    // Make sure that we have inputs selected.
-    if (!coinControl.HasSelected()) {
-        return MP_ERR_INPUTSELECT_FAIL;
-    }
-
     // Create CRecipients for outputs
     std::vector<CRecipient> vecRecipients;
     vecRecipients.push_back({exodus, dust, false}); // Exodus
     vecRecipients.push_back({destScript, nAmount, false}); // Seller
 
-    // Ask the wallet to create the transaction (note mining fee determined by Bitcoin Core params)
-    CAmount nFeeRet = 0;
-    int nChangePosInOut = -1;
+    CAmount nFeeRequired{pwallet->getMinimumFee(1000, coinControl, nullptr, nullptr)};
+    CTransactionRef wtxNew;
     std::string strFailReason;
-    auto wtxNew = pwallet->createTransaction(vecRecipients, coinControl, true /* sign */, nChangePosInOut, nFeeRet, strFailReason, false);
+    CAmount nFeeRet{0};
+    CAmount outputAmount{nAmount + dust};
+    bool createTX{true};
+
+    while (createTX) {
+        // Select the inputs
+        auto selected = mastercore::SelectCoins(*pwallet, buyerAddress, coinControl, outputAmount + nFeeRequired);
+
+        // Did not select anything at all!
+        if (!coinControl.HasSelected()) {
+            return MP_ERR_INPUTSELECT_FAIL;
+        }
+
+        // Could not select to enough to cover outputs and fee
+        if (selected < outputAmount) {
+            return MP_INPUTS_INVALID;
+        }
+
+        int nChangePosInOut = -1;
+        wtxNew = pwallet->createTransaction(vecRecipients, coinControl, true /* sign */, nChangePosInOut, nFeeRet, strFailReason, false);
+
+        // TX creation was a success or fee no longer incremeneintg
+        if (wtxNew || nFeeRet <= nFeeRequired) {
+            createTX = false;
+        } else {
+            // Set new fee required
+            nFeeRequired = nFeeRet;
+
+            PrintToLog("Increasing fee. nFeeRequired: %d selected: %d outputAmount: %d\n", nFeeRequired, selected, outputAmount);
+        }
+    }
 
     if (!wtxNew) {
         return MP_ERR_CREATE_TX;

src/omnicore/walletutils.cpp

@@ -202,18 +202,12 @@ int64_t GetEconomicThreshold(interfaces::Wallet& iWallet, const CTxOut& txOut)
 }
 
 #ifdef ENABLE_WALLET
-int64_t SelectCoins(interfaces::Wallet& iWallet, const std::string& fromAddress, CCoinControl& coinControl, int64_t additional)
+int64_t SelectCoins(interfaces::Wallet& iWallet, const std::string& fromAddress, CCoinControl& coinControl, int64_t amountRequired)
 {
     // total output funds collected
     int64_t nTotal = 0;
     int nHeight = ::ChainActive().Height();
 
-    // select coins to cover up to 20 kB max. transaction size
-    CAmount nMax = 20 * GetEstimatedFeePerKb(iWallet);
-
-    // if referenceamount is set it is needed to be accounted for here too
-    if (0 < additional) nMax += additional;
-
     std::map<uint256, interfaces::WalletTxStatus> tx_status;
     const std::vector<interfaces::WalletTx>& transactions = iWallet.getWalletTxsDetails(tx_status);
 
@@ -271,11 +265,11 @@ int64_t SelectCoins(interfaces::Wallet& iWallet, const std::string& fromAddress,
 
                 nTotal += txOut.nValue;
 
-                if (nMax <= nTotal) break;
+                if (amountRequired <= nTotal) break;
             }
         }
 
-        if (nMax <= nTotal) break;
+        if (amountRequired <= nTotal) break;
     }
 
     return nTotal;

src/omnicore/walletutils.h

@@ -41,7 +41,7 @@ int64_t GetEconomicThreshold(interfaces::Wallet& iWallet, const CTxOut& txOut);
 
 #ifdef ENABLE_WALLET
 /** Selects spendable outputs to create a transaction. */
-int64_t SelectCoins(interfaces::Wallet& iWallet, const std::string& fromAddress, CCoinControl& coinControl, int64_t additional = 0);
+int64_t SelectCoins(interfaces::Wallet& iWallet, const std::string& fromAddress, CCoinControl& coinControl, int64_t amountRequired = 0);
 
 /** Selects all spendable outputs to create a transaction. */
 int64_t SelectAllCoins(interfaces::Wallet& iWallet, const std::string& fromAddress, CCoinControl& coinControl);

src/wallet/wallet.cpp

@@ -2841,7 +2841,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
             // Get the fee rate to use effective values in coin selection
             CFeeRate nFeeRateNeeded = GetMinimumFeeRate(*this, coin_control, &feeCalc);
 
-            nFeeRet = 0;
+            nFeeRet = min_fee;
             bool pick_new_inputs = true;
             CAmount nValueIn = 0;
 
@@ -3017,7 +3017,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
                     // change output. Only try this once.
                     if (nChangePosInOut == -1 && nSubtractFeeFromAmount == 0 && pick_new_inputs) {
                         unsigned int tx_size_with_change = nBytes + coin_selection_params.change_output_size + 2; // Add 2 as a buffer in case increasing # of outputs changes compact size
-                        CAmount fee_needed_with_change = GetMinimumFee(*this, tx_size_with_change, coin_control, nullptr);
+                        CAmount fee_needed_with_change = std::max(min_fee, GetMinimumFee(*this, tx_size_with_change, coin_control, nullptr));
                         CAmount minimum_value_for_change = GetDustThreshold(change_prototype_txout, discard_rate);
                         if (nFeeRet >= fee_needed_with_change + minimum_value_for_change) {
                             pick_new_inputs = false;

test/functional/omni_calculate_fee.py

@@ -0,0 +1,99 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017-2018 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test Omni fee calculation."""
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+
+class OmniFeeCalculation(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 2
+        self.setup_clean_chain = True
+        self.extra_args = [[], ["-omnidebug=all", "-paytxfee=0.00003"]] # 3,000 Sats per KB
+
+    def run_test(self):
+        self.log.info("test fee calculation")
+
+        node0 = self.nodes[0]
+        node1 = self.nodes[1]
+
+        # Preparing some mature Bitcoins
+        coinbase_address = node0.getnewaddress()
+        node0.generatetoaddress(130, coinbase_address)
+
+        # Obtaining a master address to work with
+        address = node1.getnewaddress()
+
+        # Send lots of small dust inputs to create a large TX
+        for _ in range(3):
+            for _ in range(50):
+                node0.sendtoaddress(address, 0.00002000)
+            node0.generatetoaddress(10, coinbase_address)
+
+        # Test small transaction with single input
+        txid = node1.omni_sendissuancefixed(address, 1, 2, 0, "", "", "TST", "", "", "1000")
+        node1.generatetoaddress(2, coinbase_address)
+
+        # Checking the transaction was valid...
+        result = node1.omni_gettransaction(txid)
+        print(result)
+        assert_equal(result['valid'], True)
+
+        # Minimum fee is 1KB and 3Sats/byte rate vins to cover 3,000 Sats will be selected.
+        assert_equal(len(node1.getrawtransaction(txid, 1)['vin']), 2)
+
+        # Large data
+        large_data = "111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111"
+
+        # Create large transaction, will fail if fee requirement not met
+        txid = node1.omni_sendissuancecrowdsale(address, 1, 2, 0, large_data, large_data, large_data, large_data, large_data, 1, "1", 32533598549, 0, 0)
+        node1.generatetoaddress(1, coinbase_address)
+
+        # Checking the transaction was valid...
+        result = node1.omni_gettransaction(txid)
+        assert_equal(result['valid'], True)
+
+        # Test DEx calls
+
+        # Create and fund new address
+        omni_address = node0.getnewaddress()
+        node0.sendtoaddress(omni_address, 20)
+        node0.generatetoaddress(1, coinbase_address)
+
+        # Participating in the Exodus crowdsale to obtain some OMNI
+        txid = node0.sendmany("", {"moneyqMan7uh8FqdCA2BV5yZ8qVrc9ikLP": 10, omni_address: 4})
+        node0.generatetoaddress(10, coinbase_address)
+
+        # Create Offer
+        txid = node0.omni_senddexsell(omni_address, 1, "1", "1", 10, "0.0001", 1)
+        node0.generatetoaddress(1, coinbase_address)
+
+        # Checking the transaction was valid...
+        result = node0.omni_gettransaction(txid)
+        assert_equal(result['valid'], True)
+
+        # Accept the DEx offer
+        txid = node1.omni_senddexaccept(address, omni_address, 1, "1")
+        node1.generatetoaddress(1, coinbase_address)
+
+        # Checking the transaction was valid...
+        result = node1.omni_gettransaction(txid)
+        print(result)
+        assert_equal(result['valid'], True)
+
+        # Give buyer bitcoin to pay with, only fee left to pay
+        node0.sendtoaddress(address, 1)
+        node0.generatetoaddress(1, coinbase_address)
+
+        # Pay for the DEx offer, tests min fee arg to WalletTxBuilder
+        txid = node1.omni_senddexpay(address, omni_address, 1, "1")
+        node1.generatetoaddress(1, coinbase_address)
+
+        # Checking the transaction was valid...
+        result = node1.omni_gettransaction(txid)
+        assert_equal(result['purchases'][0]['valid'], True)
+
+if __name__ == '__main__':
+    OmniFeeCalculation().main()

test/functional/test_runner.py

@@ -224,6 +224,7 @@
     'feature_help.py',
     'feature_shutdown.py',
     'framework_test_script.py',
+    'omni_calculate_fee.py',
     'omni_reorg.py',
     'omni_clientexpiry.py',
     'omni_metadexprices.py',