Merge pull request #2446 from patricklodder/1.14.4-mintxfee-tests

Implements, tests and assures wallet operator ease-of-sovereignty and making 
sure that user-defined fee logic persists through versions in the future by 
fixing and testing -paytxfee

1. There was an override of ANY user-defined value to CWallet::GetMinimumFee

  - Former logic: always override any value with either -mintxfee or 
    -mintxrelayfee, whichever is highest
    
  - Proposed logic in this pull request:
    - if the user specifies a value, only override when it is lower than 
      -mintxfee or -mintxrelayfee - this works because we set any default
      -mintxfee to be the same as -paytxfee, unless the user explicitly 
      sets a -mintxfee.
    - if no value has been specified, use the rate from -mintxfee or 
      -mintxrelayfee, whichever is highest
      
2. Test that the interaction between the wallet parameters -paytxfee and 
   -mintxfee function as intended. This has to be done using rpc tests rather 
   than unit tests because it tests the actual parameters passed to the 
   executables.
   
3. Undoing the override exposed a misconfiguration in the bumpfee.py test, 
   where fees were explicitly set higher, yet ignored in subsequent bumps.

qa/pull-tester/rpc-tests.py

@@ -155,6 +155,7 @@ testScripts = [
     # 'nulldummy.py',
     'import-rescan.py',
     'harddustlimit.py',
+    'paytxfee.py',
     # While fee bumping should work in Doge, these tests depend on free transactions, which we don't support.
     # Disable until we can do a full rewrite of the tests (possibly upstream), or revise fee schedule, or something
     'bumpfee.py',

qa/rpc-tests/bumpfee.py

@@ -210,9 +210,10 @@ def test_rebumping(rbf_node, dest_address):
     # check that re-bumping the original tx fails, but bumping the bumper succeeds
     rbf_node.settxfee(Decimal("10.00000000"))
     rbfid = create_fund_sign_send(rbf_node, {dest_address: 7.00000000})
-    bumped = rbf_node.bumpfee(rbfid, {"totalFee": 150000000})
-    assert_raises_jsonrpc(-4, "already bumped", rbf_node.bumpfee, rbfid, {"totalFee": 2000})
-    rbf_node.bumpfee(bumped["txid"], {"totalFee": 200000000})
+    bumped = rbf_node.bumpfee(rbfid, {"totalFee": 1050000000})
+    assert_raises_jsonrpc(-4, "already bumped", rbf_node.bumpfee, rbfid, {"totalFee": 11000})
+    rbf_node.bumpfee(bumped["txid"], {"totalFee": 1100000000})
+    rbf_node.settxfee(Decimal("0.00000000"))
 
 
 def test_rebumping_not_replaceable(rbf_node, dest_address):

qa/rpc-tests/paytxfee.py

@@ -0,0 +1,82 @@
+#!/usr/bin/env python3
+# Copyright (c) 2021 The Dogecoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""PayTxFee QA test.
+
+# Tests wallet behavior of -paytxfee in relation to -mintxfee
+"""
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+from decimal import Decimal
+
+class PayTxFeeTest(BitcoinTestFramework):
+
+    def __init__(self):
+        super().__init__()
+        self.setup_clean_chain = True
+        self.num_nodes = 4
+
+    def setup_nodes(self, split=False):
+        nodes = []
+
+        # node 0 has txindex to track txs
+        nodes.append(start_node(0, self.options.tmpdir,
+            ["-debug", '-txindex']))
+
+        # node 1 pays 0.1 DOGE on all txs due to implicit mintxfee = paytxfee
+        nodes.append(start_node(1, self.options.tmpdir,
+            ["-paytxfee=0.1", "-debug"]))
+
+        # node 2 will always pay 1 DOGE on all txs because of explicit mintxfee
+        nodes.append(start_node(2, self.options.tmpdir,
+            ["-mintxfee=1", "-paytxfee=0.1", "-debug"]))
+
+        # node 3 will always pay 0.1 DOGE on all txs despite explicit mintxfee of 0.01
+        nodes.append(start_node(3, self.options.tmpdir,
+            ["-mintxfee=0.01", "-paytxfee=0.1", "-debug"]))
+
+        return nodes
+
+    def run_test(self):
+
+        seed = 1000 # the amount to seed wallets with
+        amount = 995 # the amount to send back
+        targetAddress = self.nodes[0].getnewaddress()
+
+        # mine some blocks and prepare some coins
+        self.nodes[0].generate(102)
+        self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), seed)
+        self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), seed)
+        self.nodes[0].sendtoaddress(self.nodes[3].getnewaddress(), seed)
+        self.nodes[0].generate(1)
+        self.sync_all()
+
+        # create transactions
+        txid1 = self.nodes[1].sendtoaddress(targetAddress, amount)
+        txid2 = self.nodes[2].sendtoaddress(targetAddress, amount)
+        txid3 = self.nodes[3].sendtoaddress(targetAddress, amount)
+        self.sync_all()
+
+        # make sure correct fees were paid
+        tx1 = self.nodes[0].getrawtransaction(txid1, True)
+        tx2 = self.nodes[0].getrawtransaction(txid2, True)
+        tx3 = self.nodes[0].getrawtransaction(txid3, True)
+
+        assert_equal(tx1['vout'][0]['value'] + tx1['vout'][1]['value'], Decimal("999.9"))
+        assert_equal(tx2['vout'][0]['value'] + tx2['vout'][1]['value'], Decimal("999"))
+        assert_equal(tx3['vout'][0]['value'] + tx3['vout'][1]['value'], Decimal("999.9"))
+
+        # mine a block
+        self.nodes[0].generate(1);
+        self.sync_all()
+
+        # make sure all fees were mined
+        block = self.nodes[0].getblock(self.nodes[0].getbestblockhash())
+        coinbaseTx = self.nodes[0].getrawtransaction(block['tx'][0], True)
+
+        assert_equal(coinbaseTx['vout'][0]['value'], Decimal("500001.2"))
+
+if __name__ == '__main__':
+    PayTxFeeTest().main()

src/wallet/wallet.cpp

@@ -2865,19 +2865,24 @@ CAmount CWallet::GetMinimumFee(const CMutableTransaction& tx, unsigned int nTxBy
     CAmount nFeeNeeded = targetFee;
     // User didn't set: use -txconfirmtarget to estimate...
     if (nFeeNeeded == 0) {
-        int estimateFoundTarget = nConfirmTarget;
-        nFeeNeeded = pool.estimateSmartFee(nConfirmTarget, &estimateFoundTarget).GetFee(nTxBytes);
-        // ... unless we don't have enough mempool data for estimatefee, then use fallbackFee
-        if (nFeeNeeded == 0)
-            nFeeNeeded = fallbackFee.GetFee(nTxBytes);
+        //int estimateFoundTarget = nConfirmTarget;
+        //nFeeNeeded = pool.estimateSmartFee(nConfirmTarget, &estimateFoundTarget).GetFee(nTxBytes);
+        //// ... unless we don't have enough mempool data for estimatefee, then use fallbackFee
+        //if (nFeeNeeded == 0)
+        //    nFeeNeeded = fallbackFee.GetFee(nTxBytes);
+
+        // Dogecoin: Drop the smart fee estimate, use GetRequiredFee
+        nFeeNeeded = GetRequiredFee(tx, nTxBytes);
     }
     // prevent user from paying a fee below minRelayTxFee or minTxFee
-    // Dogecoin: Drop the smart fee estimate, use GetRequiredFee
-    // nFeeNeeded = std::max(nFeeNeeded, GetRequiredFee(tx, nTxBytes));
-    nFeeNeeded = GetRequiredFee(tx, nTxBytes);
+    // Dogecoin: as we're adapting minTxFee to never be higher than
+    //           payTxFee unless explicitly set, this should be fine
+    nFeeNeeded = std::max(nFeeNeeded, GetRequiredFee(tx, nTxBytes));
+
     // But always obey the maximum
     if (nFeeNeeded > maxTxFee)
         nFeeNeeded = maxTxFee;
+
     return nFeeNeeded;
 }