Merge #2378: [Policy] Child-pays-for-parent Implementation + CoinAge …

…priority removal

f5b2e54 Fix comparisons for integer expressions of different signedness (random-zebra)
784d8b4 [Doc] Add Mempool priority changes to release notes (random-zebra)
6163d4b Remove -blockminsize option (Suhas Daftuar)
b304fb9 Allow setting minrelaytxfee to 0 (Alex Morcos)
006136e Make boost::multi_index comparators const (Suhas Daftuar)
5858fb5 [Tests] Remove priority check for sapling txes (random-zebra)
e267d02 [Cleanup] Remove coin age priority completely (random-zebra)
4298938 [RPC] Remove priorityDelta from prioritisetransaction (random-zebra)
cd1535a [rpc] Remove priority information from mempool RPC calls (Alex Morcos)
3b4d207 [Refactor][RPC] entryToJSON/EntryDescriptionString out of mempoolToJSON (random-zebra)
142415e Fix edge case with stale fee estimates (Alex Morcos)
9f47b0f Add clarifying comments to fee estimation (Alex Morcos)
57b7922 Add extra logging to processBlock in fee estimation. (Alex Morcos)
59486ef Add IsCurrentForFeeEstimatation (Alex Morcos)
d23ebfd Pass pointers to existing CTxMemPoolEntries to fee estimation (Alex Morcos)
be5982e Always update fee estimates on new blocks. (Alex Morcos)
aa97809 rename bool to validFeeEstimate (Alex Morcos)
82aba64 Remove member variable hadNoDependencies from CTxMemPoolEntry (random-zebra)
c3758e1 Don't track transactions at all during IBD. (Alex Morcos)
9ca59ae [rpc] rawtx: Prepare fLimitFree to make it an option (MarcoFalke)
f682e75 [wallet] Set fLimitFree = true (MarcoFalke)
0221d5a [QA] Fix random failure in wallet_abandonconflict.py (random-zebra)
33c9aa1 [Trivial] Pass hash by const ref to CBlockPolicyEstimator::removeTx (random-zebra)
7be33e0 Remove extraneous LogPrint from fee estimation (Alex Morcos)
7b6e8a3 [test] Remove priority from tests (Alex Morcos)
c2ecf27 No longer allow "free" transactions (Alex Morcos)
5c577c5 [Cleanup] Remove feature_nulldummy.py unused functional test (random-zebra)
3097847 [cleanup] Remove estimatePriority and estimateSmartPriority (random-zebra)
f283e8a [debug] Change -printpriority option (Alex Morcos)
dcddcaf [Cleanup] Remove unused (always false) fAllowFree arg in GetMinRelayFee (random-zebra)
03d2ee9 [mining] Remove -blockprioritysize. (Alex Morcos)
cfaeb97 [BUG] Don't add priority txes (random-zebra)
01882da Add unit tests for ancestor feerate mining (Suhas Daftuar)
49b1a9b [Policy] Limit total SHIELD size and check spork 20 in addPackageTxs (random-zebra)
cc473ed Use ancestor-feerate based transaction selection for mining (random-zebra)
dfaf4e7 [Wallet] Remove sendfree (random-zebra)

Pull request description:

  This PR implements CPFP transaction selection mechanism, which is the simplest "fee bumping technique" (we might want to consider implementing RBF too later on).
  The algorithm in `CreateNewBlock` now selects transactions based on their feerate, inclusive of unconfirmed ancestor transactions.  This means that a low-fee transaction can become more likely to be selected if a high-fee transaction that spends its outputs is relayed: the fee burned in the "child" transaction, actually pays for the parent tx too (hence the name).

  This PR also removes completely the (already deprecated) notion of coinage-based priority and "free transactions area".

  Changes backported / adapted from:

  - bitcoin#7600 [Suhas Daftuar]
  - bitcoin#8287 [MarcoFalke]
  - bitcoin#8295 [Suhas Daftuar]
  - bitcoin#9138 [Alex Morcos]
  - bitcoin#9602 [Alex Morcos]
  - bitcoin#11847 [Suhas Daftuar]

ACKs for top commit:
  Fuzzbawls:
    utACK f5b2e54
  furszy:
    re-ACK f5b2e54 after rebase and last commit.

Tree-SHA512: aef64628008699c81735660fcbe51789b7dc9d2a670d0c695399a2821f01f5d72e46d72b5f57d7b28c0e0028b60b4d6294ee101b8038ea46d237c7b8729a79da

contrib/devtools/check-doc.py

@@ -22,7 +22,7 @@
 REGEX_ARG = re.compile(r'(?:map(?:Multi)?Args(?:\.count\(|\[)|Get(?:Bool)?Arg\()\"(\-[^\"]+?)\"')
 REGEX_DOC = re.compile(r'HelpMessageOpt\(\"(\-[^\"=]+?)(?:=|\")')
 # list unsupported, deprecated and duplicate args as they need no documentation
-SET_DOC_OPTIONAL = set(['-rpcssl', '-benchmark', '-h', '-help', '-socks', '-tor', '-debugnet', '-whitelistalwaysrelay', '-prematurewitness', '-walletprematurewitness', '-promiscuousmempoolflags', '-blockminsize', '-sendfreetransactions', '-checklevel', '-liquidityprovider', '-anonymizepivxamount', '-dbcrashratio'])
+SET_DOC_OPTIONAL = set(['-rpcssl', '-benchmark', '-h', '-help', '-socks', '-tor', '-debugnet', '-whitelistalwaysrelay', '-prematurewitness', '-walletprematurewitness', '-promiscuousmempoolflags', '-blockminsize', '-checklevel', '-liquidityprovider', '-anonymizepivxamount', '-dbcrashratio'])
 
 def main():
     used = check_output(CMD_GREP_ARGS, shell=True, universal_newlines=True)

doc/release-notes.md

@@ -100,6 +100,26 @@ It is now possible to only redo validation, without rebuilding the block index,
 This new option is useful when the blocks on disk are assumed to be fine, but the chainstate is still corrupted. It is also useful for benchmarks.
 
 
+Mining/Staking transaction selection ("Child Pays For Parent")
+--------------------------------------------------------------
+
+The block-generation transaction selection algorithm now selects transactions based on their feerate inclusive of unconfirmed ancestor transactions.  This means that a low-fee transaction can become more likely to be selected if a high-fee transaction that spends its outputs is relayed.
+With this change, the `-blockminsize` command line option has been removed.
+
+
+Removal of Priority Estimation - Coin Age Priority
+--------------------------------------------------
+
+In previous versions of PIVX Core, a portion of each block could be reserved for transactions based on the age and value of UTXOs they spent. This concept (Coin Age Priority) is a policy choice by miners/stakers, and there are no consensus rules around the inclusion of Coin Age Priority transactions in blocks. 
+PIVX Core v6.0.0 removes all remaining support for Coin Age Priority (See [PR #2378](https://github.com/PIVX-Project/PIVX/pull/2378)). This has the following implications:
+
+- The concept of *free transactions* has been removed. High Coin Age Priority transactions would previously be allowed to be relayed even if they didn't attach a miner fee. This is no longer possible since there is no concept of Coin Age Priority. The `-limitfreerelay` and `-relaypriority` options which controlled relay of free transactions have therefore been removed.
+- The `-blockprioritysize` option has been removed.
+- The `prioritisetransaction` RPC no longer takes a `priority_delta` argument. The RPC is still used to change the apparent fee-rate of the transaction by using the `fee_delta` argument.
+- `-minrelaytxfee` can now be set to 0. If `minrelaytxfee` is set, then fees smaller than `minrelaytxfee` (per kB) are rejected from relaying, mining and transaction creation. This defaults to 10000 satoshi/kB.
+- The `-printpriority` option has been updated to only output the fee rate and hash of transactions included in a block by the mining code.
+
+
 GUI changes
 -----------
 

src/blockassembler.h

@@ -1,5 +1,5 @@
 // Copyright (c) 2009-2010 Satoshi Nakamoto
-// Copyright (c) 2009-2015 The Bitcoin Core developers
+// Copyright (c) 2009-2021 The Bitcoin Core developers
 // Copyright (c) 2021 The PIVX Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or https://www.opensource.org/licenses/mit-license.php.
@@ -12,6 +12,8 @@
 
 #include <stdint.h>
 #include <memory>
+#include "boost/multi_index_container.hpp"
+#include "boost/multi_index/ordered_index.hpp"
 
 class CBlockIndex;
 class CChainParams;
@@ -29,6 +31,105 @@ struct CBlockTemplate
     std::vector<int64_t> vTxSigOps;
 };
 
+// Container for tracking updates to ancestor feerate as we include (parent)
+// transactions in a block
+struct CTxMemPoolModifiedEntry {
+    CTxMemPoolModifiedEntry(CTxMemPool::txiter entry)
+    {
+        iter = entry;
+        nSizeWithAncestors = entry->GetSizeWithAncestors();
+        nModFeesWithAncestors = entry->GetModFeesWithAncestors();
+        nSigOpCountWithAncestors = entry->GetSigOpCountWithAncestors();
+    }
+
+    CTxMemPool::txiter iter;
+    uint64_t nSizeWithAncestors;
+    CAmount nModFeesWithAncestors;
+    unsigned int nSigOpCountWithAncestors;
+};
+
+/** Comparator for CTxMemPool::txiter objects.
+ *  It simply compares the internal memory address of the CTxMemPoolEntry object
+ *  pointed to. This means it has no meaning, and is only useful for using them
+ *  as key in other indexes.
+ */
+struct CompareCTxMemPoolIter {
+    bool operator()(const CTxMemPool::txiter& a, const CTxMemPool::txiter& b) const
+    {
+        return &(*a) < &(*b);
+    }
+};
+
+struct modifiedentry_iter {
+    typedef CTxMemPool::txiter result_type;
+    result_type operator() (const CTxMemPoolModifiedEntry &entry) const
+    {
+        return entry.iter;
+    }
+};
+
+// This matches the calculation in CompareTxMemPoolEntryByAncestorFee,
+// except operating on CTxMemPoolModifiedEntry.
+// TODO: refactor to avoid duplication of this logic.
+struct CompareModifiedEntry {
+    bool operator()(const CTxMemPoolModifiedEntry &a, const CTxMemPoolModifiedEntry &b) const
+    {
+        double f1 = (double)a.nModFeesWithAncestors * b.nSizeWithAncestors;
+        double f2 = (double)b.nModFeesWithAncestors * a.nSizeWithAncestors;
+        if (f1 == f2) {
+            return CTxMemPool::CompareIteratorByHash()(a.iter, b.iter);
+        }
+        return f1 > f2;
+    }
+};
+
+// A comparator that sorts transactions based on number of ancestors.
+// This is sufficient to sort an ancestor package in an order that is valid
+// to appear in a block.
+struct CompareTxIterByAncestorCount {
+    bool operator()(const CTxMemPool::txiter &a, const CTxMemPool::txiter &b) const
+    {
+        if (a->GetCountWithAncestors() != b->GetCountWithAncestors())
+            return a->GetCountWithAncestors() < b->GetCountWithAncestors();
+        return CTxMemPool::CompareIteratorByHash()(a, b);
+    }
+};
+
+typedef boost::multi_index_container<
+    CTxMemPoolModifiedEntry,
+    boost::multi_index::indexed_by<
+        boost::multi_index::ordered_unique<
+            modifiedentry_iter,
+            CompareCTxMemPoolIter
+        >,
+        // sorted by modified ancestor fee rate
+        boost::multi_index::ordered_non_unique<
+            // Reuse same tag from CTxMemPool's similar index
+            boost::multi_index::tag<ancestor_score>,
+            boost::multi_index::identity<CTxMemPoolModifiedEntry>,
+            CompareModifiedEntry
+        >
+    >
+> indexed_modified_transaction_set;
+
+typedef indexed_modified_transaction_set::nth_index<0>::type::iterator modtxiter;
+typedef indexed_modified_transaction_set::index<ancestor_score>::type::iterator modtxscoreiter;
+
+struct update_for_parent_inclusion
+{
+    update_for_parent_inclusion(CTxMemPool::txiter it) : iter(it) {}
+
+    void operator() (CTxMemPoolModifiedEntry &e)
+    {
+        e.nModFeesWithAncestors -= iter->GetFee();
+        e.nSizeWithAncestors -= iter->GetTxSize();
+        e.nSigOpCountWithAncestors -= iter->GetSigOpCount();
+    }
+
+    CTxMemPool::txiter iter;
+};
+
+
 /** Generate a new block */
 class BlockAssembler
 {
@@ -38,8 +139,8 @@ class BlockAssembler
     // A convenience pointer that always refers to the CBlock in pblocktemplate
     CBlock* pblock{nullptr};
 
-    // Configuration parameters for the block size
-    unsigned int nBlockMaxSize{0}, nBlockMinSize{0};
+    // Configuration parameters for the block max size
+    unsigned int nBlockMaxSize{0};
 
     // Information on the current status of the block
     uint64_t nBlockSize{0};
@@ -52,10 +153,6 @@ class BlockAssembler
     int nHeight{0};
     const CChainParams& chainparams;
 
-    // Variables used for addScoreTxs and addPriorityTxs
-    int lastFewTxs{0};
-    bool blockFinished{false};
-
     // Keep track of block space used for shield txes
     unsigned int nSizeShielded{0};
 
@@ -79,18 +176,27 @@ class BlockAssembler
     void AddToBlock(CTxMemPool::txiter iter);
 
     // Methods for how to add transactions to a block.
-    /** Add transactions based on modified feerate */
-    void addScoreTxs();
-    /** Add transactions based on tx "priority" */
-    void addPriorityTxs();
+    /** Add transactions based on feerate including unconfirmed ancestors */
+    void addPackageTxs();
     /** Add the tip updated incremental merkle tree to the header */
     void appendSaplingTreeRoot();
 
-    // helper function for addScoreTxs and addPriorityTxs
-    /** Test if tx will still "fit" in the block */
-    bool TestForBlock(CTxMemPool::txiter iter);
-    /** Test if tx still has unconfirmed parents not yet in block */
-    bool isStillDependent(CTxMemPool::txiter iter);
+    // helper functions for addPackageTxs()
+    /** Remove confirmed (inBlock) entries from given set */
+    void onlyUnconfirmed(CTxMemPool::setEntries& testSet);
+    /** Test if a new package would "fit" in the block */
+    bool TestPackage(uint64_t packageSize, unsigned int packageSigOps);
+    /** Test if a set of transactions are all final */
+    bool TestPackageFinality(const CTxMemPool::setEntries& package);
+    /** Return true if given transaction from mapTx has already been evaluated,
+      * or if the transaction's cached data in mapTx is incorrect. */
+    bool SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set &mapModifiedTx, CTxMemPool::setEntries &failedTx);
+    /** Sort the package in an order that is valid to appear in a block */
+    void SortForBlock(const CTxMemPool::setEntries& package, CTxMemPool::txiter entry, std::vector<CTxMemPool::txiter>& sortedEntries);
+    /** Add descendants of given transactions to mapModifiedTx with ancestor
+      * state updated assuming given transactions are inBlock. */
+    void UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set &mapModifiedTx);
+
 };
 
 /** Modify the nonce/extranonce in a block */

src/coins.cpp

@@ -364,33 +364,6 @@ bool CCoinsViewCache::HaveInputs(const CTransaction& tx) const
     return true;
 }
 
-double CCoinsViewCache::GetPriority(const CTransaction& tx, int nHeight, CAmount &inChainInputValue) const
-{
-    inChainInputValue = 0;
-    if (tx.IsCoinBase() || tx.IsCoinStake())
-        return 0.0;
-
-
-    // Shielded transfers do not reveal any information about the value or age of a note, so we
-    // cannot apply the priority algorithm used for transparent utxos.  Instead, we just
-    // use the maximum priority for all (partially or fully) shielded transactions.
-    // (Note that coinbase/coinstakes transactions cannot contain Sapling shielded Spends or Outputs.)
-    if (tx.IsShieldedTx()) {
-        return INF_PRIORITY;
-    }
-
-    double dResult = 0.0;
-    for (const CTxIn& txin : tx.vin) {
-        const Coin& coin = AccessCoin(txin.prevout);
-        if (coin.IsSpent()) continue;
-        if (coin.nHeight <= (unsigned)nHeight) {
-            dResult += coin.out.nValue * (nHeight - coin.nHeight);
-            inChainInputValue += coin.out.nValue;
-        }
-    }
-    return tx.ComputePriority(dResult);
-}
-
 int CCoinsViewCache::GetCoinDepthAtHeight(const COutPoint& output, int nHeight) const
 {
     const Coin& coin = AccessCoin(output);

src/coins.h

@@ -389,13 +389,6 @@ class CCoinsViewCache : public CCoinsViewBacked
     //! Check whether all prevouts of the transaction are present in the UTXO set represented by this view
     bool HaveInputs(const CTransaction& tx) const;
 
-    /**
-     * Return priority of tx at height nHeight. Also calculate the sum of the values of the inputs
-     * that are already in the chain.  These are the inputs that will age and increase priority as
-     * new blocks are added to the chain.
-     */
-    double GetPriority(const CTransaction& tx, int nHeight, CAmount &inChainInputValue) const;
-
     /*
      * Return the depth of a coin at height nHeight, or -1 if not found
      */

src/init.cpp

@@ -559,16 +559,14 @@ std::string HelpMessage(HelpMessageMode mode)
     strUsage += HelpMessageOpt("-logtimemicros", strprintf("Add microsecond precision to debug timestamps (default: %u)", DEFAULT_LOGTIMEMICROS));
     if (showDebug) {
         strUsage += HelpMessageOpt("-mocktime=<n>", "Replace actual time with <n> seconds since epoch (default: 0)");
-        strUsage += HelpMessageOpt("-limitfreerelay=<n>", strprintf(_("Continuously rate-limit free transactions to <n>*1000 bytes per minute (default:%u)"), DEFAULT_LIMITFREERELAY));
-        strUsage += HelpMessageOpt("-relaypriority", strprintf(_("Require high priority for relaying free or low-fee transactions (default:%u)"), DEFAULT_RELAYPRIORITY));
         strUsage += HelpMessageOpt("-maxsigcachesize=<n>", strprintf(_("Limit size of signature cache to <n> MiB (default: %u)"), DEFAULT_MAX_SIG_CACHE_SIZE));
     }
     strUsage += HelpMessageOpt("-maxtipage=<n>", strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)", DEFAULT_MAX_TIP_AGE));
     strUsage += HelpMessageOpt("-minrelaytxfee=<amt>", strprintf(_("Fees (in %s/Kb) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)"), CURRENCY_UNIT, FormatMoney(::minRelayTxFee.GetFeePerK())));
     strUsage += HelpMessageOpt("-printtoconsole", strprintf(_("Send trace/debug info to console instead of debug.log file (default: %u)"), 0));
     if (showDebug) {
         strUsage += HelpMessageOpt("-dustrelayfee=<amt>", strprintf("Fee rate (in %s/kB) used to define dust, the value of an output such that it will cost more than its value in fees at this fee rate to spend it. (default: %s)", CURRENCY_UNIT, FormatMoney(DUST_RELAY_TX_FEE)));
-        strUsage += HelpMessageOpt("-printpriority", strprintf(_("Log transaction priority and fee per kB when mining blocks (default: %u)"), DEFAULT_PRINTPRIORITY));
+        strUsage += HelpMessageOpt("-printpriority", strprintf("Log transaction fee per kB when mining blocks (default: %u)", DEFAULT_PRINTPRIORITY));
     }
     strUsage += HelpMessageOpt("-shrinkdebugfile", _("Shrink debug.log file on client startup (default: 1 when no -debug)"));
     AppendParamsHelpMessages(strUsage, showDebug);
@@ -597,9 +595,9 @@ std::string HelpMessage(HelpMessageMode mode)
     }
 
     strUsage += HelpMessageGroup(_("Block creation options:"));
-    strUsage += HelpMessageOpt("-blockminsize=<n>", strprintf(_("Set minimum block size in bytes (default: %u)"), DEFAULT_BLOCK_MIN_SIZE));
     strUsage += HelpMessageOpt("-blockmaxsize=<n>", strprintf(_("Set maximum block size in bytes (default: %d)"), DEFAULT_BLOCK_MAX_SIZE));
-    strUsage += HelpMessageOpt("-blockprioritysize=<n>", strprintf(_("Set maximum size of high-priority/low-fee transactions in bytes (default: %d)"), DEFAULT_BLOCK_PRIORITY_SIZE));
+    if (showDebug)
+        strUsage += HelpMessageOpt("-blockversion=<n>", "Override block version to test forking scenarios");
 
     strUsage += HelpMessageGroup(_("RPC server options:"));
     strUsage += HelpMessageOpt("-server", _("Accept command line and JSON-RPC commands"));
@@ -1145,18 +1143,19 @@ bool AppInitParameterInteraction()
     if (nConnectTimeout <= 0)
         nConnectTimeout = DEFAULT_CONNECT_TIMEOUT;
 
-    // Fee-per-kilobyte amount considered the same as "free"
+    // Fee-per-kilobyte amount required for mempool acceptance and relay
     // If you are mining, be careful setting this:
     // if you set it to zero then
     // a transaction spammer can cheaply fill blocks using
-    // 1-satoshi-fee transactions. It should be set above the real
+    // 0-fee transactions. It should be set above the real
     // cost to you of processing a transaction.
     if (gArgs.IsArgSet("-minrelaytxfee")) {
         CAmount n = 0;
-        if (ParseMoney(gArgs.GetArg("-minrelaytxfee", ""), n) && n > 0)
-            ::minRelayTxFee = CFeeRate(n);
-        else
+        if (!ParseMoney(gArgs.GetArg("-minrelaytxfee", ""), n)) {
             return UIError(AmountErrMsg("minrelaytxfee", gArgs.GetArg("-minrelaytxfee", "")));
+        }
+        // High fee check is done afterward in CWallet::ParameterInteraction()
+        ::minRelayTxFee = CFeeRate(n);
     }
 
     const CChainParams& chainparams = Params();

src/net_processing.cpp

@@ -1547,7 +1547,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
                             LogPrint(BCLog::MEMPOOL, "   invalid orphan tx %s\n", orphanHash.ToString());
                         }
                         // Has inputs but not accepted to mempool
-                        // Probably non-standard or insufficient fee/priority
+                        // Probably non-standard or insufficient fee
                         LogPrint(BCLog::MEMPOOL, "   removed orphan tx %s\n", orphanHash.ToString());
                         vEraseQueue.push_back(orphanHash);
                         assert(recentRejects);