diff --git a/src/miner.cpp b/src/miner.cpp index 5c7f0982c4..6a665a25e0 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -100,9 +100,9 @@ void BlockAssembler::resetBlock() Optional BlockAssembler::m_last_block_num_txs{nullopt}; Optional BlockAssembler::m_last_block_weight{nullopt}; -std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, int min_tx_age, DynaFedParamEntry* proposed_entry) +std::unique_ptr BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, std::chrono::seconds min_tx_age, DynaFedParamEntry* proposed_entry) { - assert(min_tx_age >= 0); + assert(min_tx_age >= std::chrono::seconds(0)); int64_t nTimeStart = GetTimeMicros(); resetBlock(); @@ -340,7 +340,7 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::ve // Each time through the loop, we compare the best transaction in // mapModifiedTxs with the next transaction in the mempool to decide what // transaction package to work on next. -void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, int min_tx_age) +void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, std::chrono::seconds min_tx_age) { // mapModifiedTx will store sorted packages after they are modified // because some of their txs are already in the block @@ -398,7 +398,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda // Skip transactions that are under X seconds in mempool // min_tx_age value of 0 is considered "inactive", in case of mocktime - if (min_tx_age > 0 && iter->GetTime() > GetTime() - min_tx_age) { + if (min_tx_age > std::chrono::seconds(0) && iter->GetTime() > GetTime() - min_tx_age) { continue; } diff --git a/src/miner.h b/src/miner.h index ab8844069f..d5b1b734ef 100644 --- a/src/miner.h +++ b/src/miner.h @@ -159,7 +159,7 @@ public: BlockAssembler(const CChainParams& params, const Options& options); /** Construct a new block template with coinbase to scriptPubKeyIn. min_tx_age is in seconds */ - std::unique_ptr CreateNewBlock(const CScript& scriptPubKeyIn, int min_tx_age=0, DynaFedParamEntry* = nullptr); + std::unique_ptr CreateNewBlock(const CScript& scriptPubKeyIn, std::chrono::seconds min_tx_age = std::chrono::seconds(0), DynaFedParamEntry* = nullptr); static Optional m_last_block_num_txs; static Optional m_last_block_weight; @@ -175,7 +175,7 @@ private: /** Add transactions based on feerate including unconfirmed ancestors * Increments nPackagesSelected / nDescendantsUpdated with corresponding * statistics from the package selection (for logging statistics). */ - void addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, int required_wait=0) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs); + void addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, std::chrono::seconds required_wait = std::chrono::seconds(0)) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs); // helper functions for addPackageTxs() /** Remove confirmed (inBlock) entries from given set */ diff --git a/src/net.h b/src/net.h index f33147155a..44655abf80 100644 --- a/src/net.h +++ b/src/net.h @@ -761,7 +761,7 @@ public: // Used for BIP35 mempool sending bool fSendMempool GUARDED_BY(cs_tx_inventory){false}; // Last time a "MEMPOOL" request was serviced. - std::atomic timeLastMempoolReq{0}; + std::atomic m_last_mempool_req{std::chrono::seconds{0}}; int64_t nNextInvSend{0}; CCriticalSection cs_feeFilter; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 34d349e8e9..e345af604c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1541,11 +1541,11 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm if (mi != mapRelay.end()) { connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *mi->second)); push = true; - } else if (pfrom->m_tx_relay->timeLastMempoolReq) { + } else if (pfrom->m_tx_relay->m_last_mempool_req.load().count()) { auto txinfo = mempool.info(inv.hash); // To protect privacy, do not answer getdata using the mempool when // that TX couldn't have been INVed in reply to a MEMPOOL request. - if (txinfo.tx && txinfo.nTime <= pfrom->m_tx_relay->timeLastMempoolReq) { + if (txinfo.tx && txinfo.m_time <= pfrom->m_tx_relay->m_last_mempool_req.load()) { connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *txinfo.tx)); push = true; } @@ -3873,7 +3873,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) vInv.clear(); } } - pto->m_tx_relay->timeLastMempoolReq = GetTime(); + pto->m_tx_relay->m_last_mempool_req = GetTime(); } // Determine transactions to relay diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index a8bb0c191c..3d87c38f6c 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -460,7 +460,7 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool info.pushKV("weight", (int)e.GetTxWeight()); info.pushKV("fee", ValueFromAmount(e.GetFee())); info.pushKV("modifiedfee", ValueFromAmount(e.GetModifiedFee())); - info.pushKV("time", e.GetTime()); + info.pushKV("time", count_seconds(e.GetTime())); info.pushKV("height", (int)e.GetHeight()); info.pushKV("descendantcount", e.GetCountWithDescendants()); info.pushKV("descendantsize", e.GetSizeWithDescendants()); diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index fd4c00fec8..5167b2e4de 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -1048,7 +1048,7 @@ UniValue getnewblockhex(const JSONRPCRequest& request) CScript feeDestinationScript = Params().GetConsensus().mandatory_coinbase_destination; if (feeDestinationScript == CScript()) feeDestinationScript = CScript() << OP_TRUE; - std::unique_ptr pblocktemplate(BlockAssembler(Params()).CreateNewBlock(feeDestinationScript, required_wait, &proposed)); + std::unique_ptr pblocktemplate(BlockAssembler(Params()).CreateNewBlock(feeDestinationScript, std::chrono::seconds(required_wait), &proposed)); if (!pblocktemplate.get()) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Wallet keypool empty"); } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 2d121fc411..adc7937802 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -999,7 +999,8 @@ void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants, MemPool } } -int CTxMemPool::Expire(int64_t time) { +int CTxMemPool::Expire(std::chrono::seconds time) +{ AssertLockHeld(cs); indexed_transaction_set::index::type::iterator it = mapTx.get().begin(); setEntries toremove; diff --git a/src/txmempool.h b/src/txmempool.h index 84e206db02..d3ad5172db 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -107,7 +107,7 @@ public: const CAmount& GetFee() const { return nFee; } size_t GetTxSize() const; size_t GetTxWeight() const { return nTxWeight; } - int64_t GetTime() const { return nTime; } + std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; } unsigned int GetHeight() const { return entryHeight; } int64_t GetSigOpCost() const { return sigOpCost; } int64_t GetModifiedFee() const { return nFee + feeDelta; } @@ -337,7 +337,7 @@ struct TxMempoolInfo CTransactionRef tx; /** Time the transaction entered the mempool. */ - int64_t nTime; + std::chrono::seconds m_time; /** Feerate of the transaction. */ CFeeRate feeRate; @@ -663,7 +663,7 @@ public: void TrimToSize(size_t sizelimit, std::vector* pvNoSpendsRemaining = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs); /** Expire all transaction (and their dependencies) in the mempool older than time. Return the number of removed transactions. */ - int Expire(int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs); + int Expire(std::chrono::seconds time) EXCLUSIVE_LOCKS_REQUIRED(cs); /** * Calculate the ancestor and descendant count for the given transaction. diff --git a/src/util/time.h b/src/util/time.h index e4f9996777..c0470a2136 100644 --- a/src/util/time.h +++ b/src/util/time.h @@ -10,6 +10,14 @@ #include #include +/** + * Helper to count the seconds of a duration. + * + * All durations should be using std::chrono and calling this should generally be avoided in code. Though, it is still + * preferred to an inline t.count() to protect against a reliance on the exact type of t. + */ +inline int64_t count_seconds(std::chrono::seconds t) { return t.count(); } + /** * DEPRECATED * Use either GetSystemTimeInSeconds (not mockable) or GetTime (mockable) diff --git a/src/validation.cpp b/src/validation.cpp index a3694bc9ba..616dec0d3d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -327,10 +327,10 @@ bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flag // Returns the script flags which should be checked for a given block static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& chainparams); -static void LimitMempoolSize(CTxMemPool& pool, size_t limit, unsigned long age) +static void LimitMempoolSize(CTxMemPool& pool, size_t limit, std::chrono::seconds age) EXCLUSIVE_LOCKS_REQUIRED(pool.cs, ::cs_main) { - int expired = pool.Expire(GetTime() - age); + int expired = pool.Expire(GetTime() - age); if (expired != 0) { LogPrint(BCLog::MEMPOOL, "Expired %i transactions from the memory pool\n", expired); } @@ -402,7 +402,7 @@ static void UpdateMempoolForReorg(DisconnectedBlockTransactions& disconnectpool, // We also need to remove any now-immature transactions mempool.removeForReorg(&::ChainstateActive().CoinsTip(), ::ChainActive().Tip()->nHeight + 1, STANDARD_LOCKTIME_VERIFY_FLAGS); // Re-limit mempool size, in case we added any transactions - LimitMempoolSize(mempool, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60); + LimitMempoolSize(mempool, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, std::chrono::hours{gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); } // Used to avoid mempool polluting consensus critical paths if CCoinsViewMempool @@ -1082,7 +1082,7 @@ bool MemPoolAccept::Finalize(ATMPArgs& args, Workspace& ws) // trim mempool and check if tx was trimmed if (!bypass_limits) { - LimitMempoolSize(m_pool, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60); + LimitMempoolSize(m_pool, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, std::chrono::hours{gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}); if (!m_pool.exists(hash)) return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "mempool full"); } @@ -5369,8 +5369,8 @@ bool DumpMempool(const CTxMemPool& pool) file << (uint64_t)vinfo.size(); for (const auto& i : vinfo) { file << *(i.tx); - file << (int64_t)i.nTime; - file << (int64_t)i.nFeeDelta; + file << int64_t{count_seconds(i.m_time)}; + file << int64_t{i.nFeeDelta}; mapDeltas.erase(i.tx->GetHash()); } diff --git a/test/functional/mempool_persist.py b/test/functional/mempool_persist.py index 3a2d24c5e0..68bb034402 100755 --- a/test/functional/mempool_persist.py +++ b/test/functional/mempool_persist.py @@ -37,9 +37,15 @@ Test is as follows: """ from decimal import Decimal import os +import time from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal, assert_raises_rpc_error, wait_until +from test_framework.util import ( + assert_equal, + assert_greater_than_or_equal, + assert_raises_rpc_error, + wait_until, +) class MempoolPersistTest(BitcoinTestFramework): @@ -51,18 +57,13 @@ class MempoolPersistTest(BitcoinTestFramework): self.skip_if_no_wallet() def run_test(self): - chain_height = self.nodes[0].getblockcount() - assert_equal(chain_height, 200) - - self.log.debug("Mine a single block to get out of IBD") - self.nodes[0].generate(1) - self.sync_all() - self.log.debug("Send 5 transactions from node2 (to its own address)") + tx_creation_time_lower = int(time.time()) for i in range(5): last_txid = self.nodes[2].sendtoaddress(self.nodes[2].getnewaddress(), Decimal("10")) node2_balance = self.nodes[2].getbalance()['bitcoin'] self.sync_all() + tx_creation_time_higher = int(time.time()) self.log.debug("Verify that node0 and node1 have 5 transactions in their mempools") assert_equal(len(self.nodes[0].getrawmempool()), 5) @@ -75,6 +76,10 @@ class MempoolPersistTest(BitcoinTestFramework): fees = self.nodes[0].getmempoolentry(txid=last_txid)['fees'] assert_equal(fees['base'] + Decimal('0.00001000'), fees['modified']) + tx_creation_time = self.nodes[0].getmempoolentry(txid=last_txid)['time'] + assert_greater_than_or_equal(tx_creation_time, tx_creation_time_lower) + assert_greater_than_or_equal(tx_creation_time_higher, tx_creation_time) + self.log.debug("Stop-start the nodes. Verify that node0 has the transactions in its mempool and node1 does not. Verify that node2 calculates its balance correctly after loading wallet transactions.") self.stop_nodes() # Give this node a head-start, so we can be "extra-sure" that it didn't load anything later @@ -93,6 +98,9 @@ class MempoolPersistTest(BitcoinTestFramework): fees = self.nodes[0].getmempoolentry(txid=last_txid)['fees'] assert_equal(fees['base'] + Decimal('0.00001000'), fees['modified']) + self.log.debug('Verify time is loaded correctly') + assert_equal(tx_creation_time, self.nodes[0].getmempoolentry(txid=last_txid)['time']) + # Verify accounting of mempool transactions after restart is correct self.nodes[2].syncwithvalidationinterfacequeue() # Flush mempool to wallet assert_equal(node2_balance, self.nodes[2].getbalance()['bitcoin'])