Skip to content

Minor cleanups for AcceptToMemoryPool #11309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maflcko merged 3 commits into from
Sep 29, 2017
Merged

Minor cleanups for AcceptToMemoryPool #11309

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd849e1
Change AcceptToMemoryPool function signature
morcos Sep 12, 2017
04f78ab
Do not reject based on mempool min fee when bypass_limits is set.
morcos Sep 12, 2017
bf64c3c
Ignore transactions added to mempool during a reorg for fee estimatio…
morcos Sep 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1788,7 +1788,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr

std::list<CTransactionRef> lRemovedTxn;

if (!AlreadyHave(inv) && AcceptToMemoryPool(mempool, state, ptx, true, &fMissingInputs, &lRemovedTxn)) {
if (!AlreadyHave(inv) &&
AcceptToMemoryPool(mempool, state, ptx, &fMissingInputs, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) {

@promag promag Sep 13, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, feeling uncomfortable with these comments 😄.

@sipa sipa Sep 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@promag Why? I think that the function calls are effectively unreadable without them (though we should change ATMP to take an options object instead of a gazillion arguments instead).

@ryanofsky ryanofsky Sep 16, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think he's just joking about the existence of "bypass limits" and "absurd fee" options.

@promag promag Sep 18, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No joking, just what @sipa said. One struct with the arguments and fill them, resembles named arguments.

mempool.check(pcoinsTip);
RelayTransaction(tx, connman);
for (unsigned int i = 0; i < tx.vout.size(); i++) {
Expand Down Expand Up @@ -1826,7 +1827,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr

if (setMisbehaving.count(fromPeer))
continue;
if (AcceptToMemoryPool(mempool, stateDummy, porphanTx, true, &fMissingInputs2, &lRemovedTxn)) {
if (AcceptToMemoryPool(mempool, stateDummy, porphanTx, &fMissingInputs2, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) {
LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString());
RelayTransaction(orphanTx, connman);
for (unsigned int i = 0; i < orphanTx.vout.size(); i++) {
Expand Down
4 changes: 2 additions & 2 deletions src/rpc/rawtransaction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,8 +937,8 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
// push to local node and sync with wallets
CValidationState state;
bool fMissingInputs;
bool fLimitFree = true;
if (!AcceptToMemoryPool(mempool, state, std::move(tx), fLimitFree, &fMissingInputs, nullptr, false, nMaxRawTxFee)) {
if (!AcceptToMemoryPool(mempool, state, std::move(tx), &fMissingInputs,
nullptr /* plTxnReplaced */, false /* bypass_limits */, nMaxRawTxFee)) {
if (state.IsInvalid()) {
throw JSONRPCError(RPC_TRANSACTION_REJECTED, strprintf("%i: %s", state.GetRejectCode(), state.GetRejectReason()));
} else {
Expand Down
3 changes: 2 additions & 1 deletion src/test/txvalidationcache_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ ToMemPool(CMutableTransaction& tx)
LOCK(cs_main);

CValidationState state;
return AcceptToMemoryPool(mempool, state, MakeTransactionRef(tx), false, nullptr, nullptr, true, 0);
return AcceptToMemoryPool(mempool, state, MakeTransactionRef(tx), nullptr /* pfMissingInputs */,
nullptr /* plTxnReplaced */, true /* bypass_limits */, 0 /* nAbsurdFee */);
}

BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
Expand Down
27 changes: 15 additions & 12 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,9 @@ void UpdateMempoolForReorg(DisconnectedBlockTransactions &disconnectpool, bool f
while (it != disconnectpool.queuedTx.get<insertion_order>().rend()) {
// ignore validation errors in resurrected transactions
CValidationState stateDummy;
if (!fAddToMempool || (*it)->IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, *it, false, nullptr, nullptr, true)) {
if (!fAddToMempool || (*it)->IsCoinBase() ||
!AcceptToMemoryPool(mempool, stateDummy, *it, nullptr /* pfMissingInputs */,
nullptr /* plTxnReplaced */, true /* bypass_limits */, 0 /* nAbsurdFee */)) {
// If the transaction doesn't make it in to the mempool, remove any
// transactions that depend on it (which would now be orphans).
mempool.removeRecursive(**it, MemPoolRemovalReason::REORG);
Expand Down Expand Up @@ -443,9 +445,9 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, CValidationSt
return CheckInputs(tx, state, view, true, flags, cacheSigStore, true, txdata);
}

static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool& pool, CValidationState& state, const CTransactionRef& ptx, bool fLimitFree,
static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool& pool, CValidationState& state, const CTransactionRef& ptx,
bool* pfMissingInputs, int64_t nAcceptTime, std::list<CTransactionRef>* plTxnReplaced,
bool fOverrideMempoolLimit, const CAmount& nAbsurdFee, std::vector<COutPoint>& coins_to_uncache)
bool bypass_limits, const CAmount& nAbsurdFee, std::vector<COutPoint>& coins_to_uncache)
{
const CTransaction& tx = *ptx;
const uint256 hash = tx.GetHash();
Expand Down Expand Up @@ -623,7 +625,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
}

// No transactions are allowed below minRelayTxFee except from disconnected blocks
if (fLimitFree && nModifiedFees < ::minRelayTxFee.GetFee(nSize)) {
if (!bypass_limits && nModifiedFees < ::minRelayTxFee.GetFee(nSize)) {
return state.DoS(0, false, REJECT_INSUFFICIENTFEE, "min relay fee not met");
}

Expand Down Expand Up @@ -865,7 +867,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
pool.addUnchecked(hash, entry, setAncestors, validForFeeEstimation);

// trim mempool and check if tx was trimmed
if (!fOverrideMempoolLimit) {
if (!bypass_limits) {
LimitMempoolSize(pool, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60);
if (!pool.exists(hash))
return state.DoS(0, false, REJECT_INSUFFICIENTFEE, "mempool full");
Expand All @@ -878,12 +880,12 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
}

/** (try to) add transaction to memory pool with a specified acceptance time **/
static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx, bool fLimitFree,
static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx,
bool* pfMissingInputs, int64_t nAcceptTime, std::list<CTransactionRef>* plTxnReplaced,
bool fOverrideMempoolLimit, const CAmount nAbsurdFee)
bool bypass_limits, const CAmount nAbsurdFee)
{
std::vector<COutPoint> coins_to_uncache;
bool res = AcceptToMemoryPoolWorker(chainparams, pool, state, tx, fLimitFree, pfMissingInputs, nAcceptTime, plTxnReplaced, fOverrideMempoolLimit, nAbsurdFee, coins_to_uncache);
bool res = AcceptToMemoryPoolWorker(chainparams, pool, state, tx, pfMissingInputs, nAcceptTime, plTxnReplaced, bypass_limits, nAbsurdFee, coins_to_uncache);
if (!res) {
for (const COutPoint& hashTx : coins_to_uncache)
pcoinsTip->Uncache(hashTx);
Expand All @@ -894,12 +896,12 @@ static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPo
return res;
}

bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx, bool fLimitFree,
bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx,
bool* pfMissingInputs, std::list<CTransactionRef>* plTxnReplaced,
bool fOverrideMempoolLimit, const CAmount nAbsurdFee)
bool bypass_limits, const CAmount nAbsurdFee)
{
const CChainParams& chainparams = Params();
return AcceptToMemoryPoolWithTime(chainparams, pool, state, tx, fLimitFree, pfMissingInputs, GetTime(), plTxnReplaced, fOverrideMempoolLimit, nAbsurdFee);
return AcceptToMemoryPoolWithTime(chainparams, pool, state, tx, pfMissingInputs, GetTime(), plTxnReplaced, bypass_limits, nAbsurdFee);
}

/** Return transaction in txOut, and if it was found inside a block, its hash is placed in hashBlock */
Expand Down Expand Up @@ -4306,7 +4308,8 @@ bool LoadMempool(void)
CValidationState state;
if (nTime + nExpiryTimeout > nNow) {
LOCK(cs_main);
AcceptToMemoryPoolWithTime(chainparams, mempool, state, tx, true, nullptr, nTime, nullptr, false, 0);
AcceptToMemoryPoolWithTime(chainparams, mempool, state, tx, nullptr /* pfMissingInputs */, nTime,
nullptr /* plTxnReplaced */, false /* bypass_limits */, 0 /* nAbsurdFee */);
if (state.IsValid()) {
++count;
} else {
Expand Down
6 changes: 3 additions & 3 deletions src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,9 @@ void PruneBlockFilesManual(int nManualPruneHeight);

/** (try to) add transaction to memory pool
* plTxnReplaced will be appended to with all transactions replaced from mempool **/
bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx, bool fLimitFree,
bool* pfMissingInputs, std::list<CTransactionRef>* plTxnReplaced = nullptr,
bool fOverrideMempoolLimit=false, const CAmount nAbsurdFee=0);
bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransactionRef &tx,
bool* pfMissingInputs, std::list<CTransactionRef>* plTxnReplaced,
bool bypass_limits, const CAmount nAbsurdFee);

/** Convert CValidationState to a human-readable message for logging */
std::string FormatStateMessage(const CValidationState &state);
Expand Down
3 changes: 2 additions & 1 deletion src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4014,5 +4014,6 @@ int CMerkleTx::GetBlocksToMaturity() const

bool CMerkleTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state)
{
return ::AcceptToMemoryPool(mempool, state, tx, true, nullptr, nullptr, false, nAbsurdFee);
return ::AcceptToMemoryPool(mempool, state, tx, nullptr /* pfMissingInputs */,
nullptr /* plTxnReplaced */, false /* bypass_limits */, nAbsurdFee);
}