validation: Pass chainparams in AcceptToMemoryPoolWorker(...) #13909

Remove unused CChainParams argument in AcceptToMemoryPoolWorker(...).

After the merge of #13527 ("policy: Remove promiscuousmempoolflags") yesterday the CChainParams argument is no longer used in AcceptToMemoryPoolWorker(...).

<!--e57a25ab6845829454e8d69fc972939a-->No more conflicts as of last run.

utACK fa61ddfbf54ca64c6807d9a49f661fff530fa5f1

utACK fa61ddf

Hmm, note that there was a silent merge conflict between 24980a3e40284d375ac8c19da25fa399ee883830 (replacing/removing Params()) and b5fea8d0ccc8117644a4ea11256bc531b60fd9a3 (adding Params()) in ATMP.

I am not sure if this is the right way to solve this merge conflict.

cc @jtimon, maybe?

@MarcoFalke Oh, a silent merge conflict - that's interesting! Could we make that less likely to happen somehow via automation?

Could it theoretically be avoided by having say the @DrahtBot test merge with different git diff algorithms and report when one of the alternative algorithms report a merge conflict and the default algorithm does not?

I doubt it, since they are in completely different lines.

See https://github.com/bitcoin/bitcoin/commit/b5fea8d0ccc8117644a4ea11256bc531b60fd9a3#diff-24efdb00bfbe56b140fb006b562cc70bR784

@MarcoFalke Got it! Thanks for clarifying!

What is the correct way to proceed here? Any advice @jtimon, @mariodian or @TheBlueMatt who touched on the silent merge conflict file? :-)

Passing them in to GetBlockScriptFlags looks like the smallest diff that still makes sense.

No activity in about a month. Closing for now. Let me know when you want to work on this again.

@MarcoFalke I'm ready. Would you mind re-opening? :-)

Does the following patch look correct?

diff --git a/src/validation.cpp b/src/validation.cpp
index d55bbfb2b..f3feea510 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -557,7 +557,7 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, CValidationSt
     return CheckInputs(tx, state, view, true, flags, cacheSigStore, true, txdata);
 }

-static bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const CTransactionRef& ptx,
+static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool& pool, CValidationState& state, const CTransactionRef& ptx,
                               bool* pfMissingInputs, int64_t nAcceptTime, std::list<CTransactionRef>* plTxnReplaced,
                               bool bypass_limits, const CAmount& nAbsurdFee, std::vector<COutPoint>& coins_to_uncache, bool test_accept)
 {
@@ -927,7 +927,7 @@ static bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state,
         // There is a similar check in CreateNewBlock() to prevent creating
         // invalid blocks (using TestBlockValidity), however allowing such
         // transactions into the mempool can be exploited as a DoS attack.
-        unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(chainActive.Tip(), Params().GetConsensus());
+        unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(chainActive.Tip(), chainparams.GetConsensus());
         if (!CheckInputsFromMempoolAndCache(tx, state, view, pool, currentBlockScriptVerifyFlags, true, txdata)) {
             return error("%s: BUG! PLEASE REPORT THIS! CheckInputs failed against latest-block but not STANDARD flags %s, %s",
                     __func__, hash.ToString(), FormatStateMessage(state));
@@ -980,7 +980,7 @@ static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPo
                         bool bypass_limits, const CAmount nAbsurdFee, bool test_accept)
 {
     std::vector<COutPoint> coins_to_uncache;
-    bool res = AcceptToMemoryPoolWorker(pool, state, tx, pfMissingInputs, nAcceptTime, plTxnReplaced, bypass_limits, nAbsurdFee, coins_to_uncache, test_accept);
+    bool res = AcceptToMemoryPoolWorker(chainparams, pool, state, tx, pfMissingInputs, nAcceptTime, plTxnReplaced, bypass_limits, nAbsurdFee, coins_to_uncache, test_accept);
     if (!res) {
         for (const COutPoint& hashTx : coins_to_uncache)
             pcoinsTip->Uncache(hashTx);

The net change from this PR would then be:

diff --git a/src/validation.cpp b/src/validation.cpp
index fceb13585..f3feea510 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -927,7 +927,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
         // There is a similar check in CreateNewBlock() to prevent creating
         // invalid blocks (using TestBlockValidity), however allowing such
         // transactions into the mempool can be exploited as a DoS attack.
-        unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(chainActive.Tip(), Params().GetConsensus());
+        unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(chainActive.Tip(), chainparams.GetConsensus());
         if (!CheckInputsFromMempoolAndCache(tx, state, view, pool, currentBlockScriptVerifyFlags, true, txdata)) {
             return error("%s: BUG! PLEASE REPORT THIS! CheckInputs failed against latest-block but not STANDARD flags %s, %s",
                     __func__, hash.ToString(), FormatStateMessage(state));

Concept NACK, there's uses of Params() inside the function that could use the parameter instead. Also, it calls to functions that could take CChainParams or Consensus::Params as parameter. This seems the reversal of some incomplete work I was doing to eliminate the use of the Params() global.

I strongly disagree with the seemingly general preference of using globals over passing parameters explicitly to the functions that need them. In no other project ever I had to defend the notion that generally globals should be avoided when possible. It is disappointing to see that not only people don't want the pseudo-global Params() to disappear but people also embrace it and undo previous changes towards making it disappear. It seems the same love exists at least for global gArgs.

@jtimon I don't have any preference for globals to be sure :-) This PR tries to get rid of an unused parameter - that's all :-)

Is the patch in #13909 (comment) in line with your suggestion?

Does the following patch look correct?

You don't need to ask for permission to fixup a change nor need to post patches in comments and ask for review. I believe in you that you can figure out from the feedback by jtimon and me what to do here.

@MarcoFalke Yes perhaps I'm too cautious sometimes :-) @MarcoFalke @jtimon Please re-review!

Oh, sorry, I missed this. A small change but 97ddc6026b4e14ee0bb4c5b04a7824ac0a7e23ab looks good to me, utACK

utACK 97ddc6026b4e14ee0bb4c5b04a7824ac0a7e23ab

Re-opened due to indication of interest :-)