validation: Pass chainparams in AcceptToMemoryPoolWorker(...) by practicalswift · Pull Request #13909 · bitcoin/bitcoin

practicalswift · 2018-08-08T07:32:42Z

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

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

DrahtBot · 2018-08-08T07:54:33Z

No more conflicts as of last run.

domob1812 · 2018-08-08T11:14:37Z

utACK fa61ddfbf54ca64c6807d9a49f661fff530fa5f1

Empact · 2018-08-08T18:43:11Z

utACK fa61ddf

maflcko · 2018-08-08T19:23:21Z

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

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

cc @jtimon, maybe?

practicalswift · 2018-08-08T23:12:39Z

@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?

maflcko · 2018-08-08T23:18:04Z

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

See b5fea8d#diff-24efdb00bfbe56b140fb006b562cc70bR784

practicalswift · 2018-08-08T23:24:03Z

@MarcoFalke Got it! Thanks for clarifying!

practicalswift · 2018-08-27T16:14:29Z

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

maflcko · 2018-08-27T17:28:44Z

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

maflcko · 2018-09-20T20:16:14Z

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

practicalswift · 2018-09-21T09:37:52Z

@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));

jtimon · 2018-09-24T12:18:33Z

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.

practicalswift · 2018-09-24T12:36:41Z

@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?

maflcko · 2018-09-24T12:57:49Z

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.

practicalswift · 2018-09-24T13:02:45Z

@MarcoFalke Yes perhaps I'm too cautious sometimes :-)

@MarcoFalke @jtimon Please re-review!

jtimon · 2018-10-19T16:41:08Z

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

maflcko · 2018-10-20T14:42:07Z

utACK 97ddc60

practicalswift · 2018-10-20T19:09:29Z

Re-opened due to indication of interest :-)

…r(...) 97ddc60 validation: Pass chainparams in AcceptToMemoryPoolWorker(...) (practicalswift) Pull request description: Remove unused `CChainParams` argument in `AcceptToMemoryPoolWorker(...)`. After the merge of #13527 ("policy: Remove promiscuousmempoolflags") yesterday the `CChainParams` argument is no longer used in `AcceptToMemoryPoolWorker(...)`. Tree-SHA512: f1bab4498b64f0ab5230b8172f860df8fa8a302e4ee7385be4ba9c65a37cbc3ef640df78348c477169b9414e5c6a160a0b6471a11f4bb27921500ec208ef5340

practicalswift changed the title ~~validation: Remove unused CChainParams argument to AcceptToMemoryPoolWorker(...)~~ validation: Remove unused CChainParams argument in AcceptToMemoryPoolWorker(...) Aug 8, 2018

fanquake added the Refactoring label Aug 8, 2018

This was referenced Aug 8, 2018

validation: Document where we are intentionally ignoring bool return values from validation related functions #13864

Closed

WIP: Transaction Pool Layer #13804

Closed

practicalswift mentioned this pull request Aug 31, 2018

wallet: Remove unused local variable old_label #14117

Closed

maflcko closed this Sep 20, 2018

maflcko reopened this Sep 21, 2018

practicalswift changed the title ~~validation: Remove unused CChainParams argument in AcceptToMemoryPoolWorker(...)~~ validation: Remove unused CChainParams parameter in AcceptToMemoryPoolWorker(...) Sep 24, 2018

practicalswift force-pushed the remove-chainparams-argument-to-AcceptToMemoryPoolWorker branch from fa61ddf to 3f1f3bc Compare September 24, 2018 13:00

maflcko changed the title ~~validation: Remove unused CChainParams parameter in AcceptToMemoryPoolWorker(...)~~ validation: Pass chainparams in AcceptToMemoryPoolWorker(...) Sep 24, 2018

practicalswift changed the title ~~validation: Pass chainparams in AcceptToMemoryPoolWorker(...)~~ validation: Resolve silent merge conflict. Pass chainparams in AcceptToMemoryPoolWorker(...). Sep 24, 2018

practicalswift changed the title ~~validation: Resolve silent merge conflict. Pass chainparams in AcceptToMemoryPoolWorker(...).~~ validation: Pass chainparams in AcceptToMemoryPoolWorker(...) Sep 24, 2018

validation: Pass chainparams in AcceptToMemoryPoolWorker(...)

97ddc60

practicalswift force-pushed the remove-chainparams-argument-to-AcceptToMemoryPoolWorker branch from 3f1f3bc to 97ddc60 Compare September 24, 2018 14:03

practicalswift closed this Oct 19, 2018

practicalswift reopened this Oct 20, 2018

maflcko merged commit 97ddc60 into bitcoin:master Oct 21, 2018

practicalswift deleted the remove-chainparams-argument-to-AcceptToMemoryPoolWorker branch April 10, 2021 19:36

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

practicalswift commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domob1812 commented Aug 8, 2018

Uh oh!

Empact commented Aug 8, 2018

Uh oh!

maflcko commented Aug 8, 2018

Uh oh!

practicalswift commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 8, 2018

Uh oh!

practicalswift commented Aug 8, 2018

Uh oh!

practicalswift commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 27, 2018

Uh oh!

maflcko commented Sep 20, 2018

Uh oh!

practicalswift commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Sep 24, 2018

Uh oh!

practicalswift commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 24, 2018

Uh oh!

practicalswift commented Sep 24, 2018

Uh oh!

jtimon commented Oct 19, 2018

Uh oh!

maflcko commented Oct 20, 2018

Uh oh!

practicalswift commented Oct 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

practicalswift commented Aug 8, 2018 •

edited

Loading

DrahtBot commented Aug 8, 2018 •

edited

Loading

practicalswift commented Aug 8, 2018 •

edited

Loading

practicalswift commented Aug 27, 2018 •

edited

Loading

practicalswift commented Sep 21, 2018 •

edited

Loading

practicalswift commented Sep 24, 2018 •

edited

Loading