Introduce CNodePolicy for putting isolated node policy code and parameters on

luke-jr commented at 3:18 am on October 10, 2014: member

Initially populated with policy-specific code moved from AcceptToMemoryPool.

This should make no actual changes, just refactor code. If any change in behaviour is found, it is a bug in this pull request, please report it even if you think it’s a good idea.

luke-jr force-pushed on Oct 10, 2014

in src/policy.cpp: in f9b84baf92 outdated

0@@ -0,0 +1,103 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 The Bitcoin developers
3+// Distributed under the MIT/X11 software license, see the accompanying

Diapolo commented at 8:17 am on October 10, 2014:

Nit: Just MIT

luke-jr commented at 10:25 am on October 10, 2014:

Just copying it from main.cpp, where the code came from. If you want it changed here, make a convincing argument to change it there first.

TheBlueMatt commented at 6:04 pm on October 12, 2014:

All files need changing, but instead of doing it all at once, we’re just changing it as files are updated. Please use just MIT.

jtimon commented at 7:15 pm on October 12, 2014:

This has been discussed already. I though we had merged a PR changing everything to just MIT already. That would be simpler than warning people every time they copy the license from an outdated place.

in src/policy.cpp: in f9b84baf92 outdated

0@@ -0,0 +1,103 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 The Bitcoin developers
3+// Distributed under the MIT/X11 software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+// NOTE: This file is intended to be customised by the end user, and includes only local node policy logic
7+
8+#include <cmath>

Diapolo commented at 8:18 am on October 10, 2014:

Nit: These need to go below our own headers.

in src/policy.cpp: in f9b84baf92 outdated

19+{
20+    // Rather not work on nonstandard transactions (unless -testnet/-regtest)
21+    std::string reason;
22+    if (fRequireStandardTx && !IsStandardTx(tx, reason))
23+        return state.DoS(0,
24+                         error("AcceptToMemoryPool : nonstandard transaction: %s", reason),

Diapolo commented at 8:18 am on October 10, 2014:

The function name is now wrong, perhaps use __func__!?

in src/policy.cpp: in f9b84baf92 outdated

47+    // merely non-standard transaction.
48+    unsigned int nSigOps = GetLegacySigOpCount(tx);
49+    nSigOps += GetP2SHSigOpCount(tx, view);
50+    if (nSigOps > MAX_TX_SIGOPS)
51+        return state.DoS(0,
52+                         error("AcceptToMemoryPool : too many sigops %s, %d > %d",

Diapolo commented at 8:19 am on October 10, 2014:

Same here ;).

in src/policy.h: in f9b84baf92 outdated

0@@ -0,0 +1,36 @@
1+// Copyright (c) 2014 The Bitcoin developers
2+// Distributed under the MIT/X11 software license, see the accompanying

Diapolo commented at 8:20 am on October 10, 2014:

Also just MIT

in src/policy.h: in f9b84baf92 outdated

15+{
16+public:
17+    virtual bool AcceptTxPoolPreInputs(CTxMemPool& pool, CValidationState&, const CTransaction&) = 0;
18+    virtual bool AcceptTxWithInputs(CTxMemPool& pool, CValidationState&, const CTransaction&, CCoinsViewCache&) = 0;
19+    virtual bool AcceptMemPoolEntry(CTxMemPool& pool, CValidationState&, CTxMemPoolEntry&, CCoinsViewCache&, bool& fRateLimit) = 0;
20+    virtual bool RateLimitTx(CTxMemPool& pool, CValidationState&, CTxMemPoolEntry&, CCoinsViewCache&) = 0;

Diapolo commented at 8:21 am on October 10, 2014:

Why are all of these not using paramater names?

luke-jr commented at 3:36 am on October 11, 2014:

There’s nothing meaningful to expand on beyond the type…

luke-jr force-pushed on Oct 11, 2014

in src/main.cpp: in 7c9d476086 outdated

923-            dFreeCount += nSize;
924-        }
925+        bool fRateLimit = false;
926+        if (!policy.AcceptMemPoolEntry(pool, state, entry, view, fRateLimit))
927+            return false;
928+        if (!fLimitFree)

dcousens commented at 4:33 am on October 11, 2014:

Why bother with fRateLimit/fLimitFree at all here, since the logic boils down to:

fRateLimit = !fLimitFree ? false : false

You could just write:

0if (!policy.AcceptMemPoolEntry(pool, state, entry, view, false))
1    return false;
2
3if (!policy.RateLimitTx(pool, state, entry, view))
4    return false;

Is it the idea that more behaviour will be added here or?

luke-jr commented at 4:33 am on October 11, 2014:

Not all transactions should be rate limited, for different reasons, some of which are policy (contracts with retailers?), some of which are not (local user manually wants to broadcast a transaction).

luke-jr commented at 4:34 am on October 11, 2014:

Notice fRateLimit is passed to AcceptMemPoolEntry by reference, and it may (and does in some cases by default) change it.

dcousens commented at 4:39 am on October 11, 2014:

Right, the pass by reference was what I was missing. LGTM then.

luke-jr commented at 4:53 am on October 11, 2014:

I agree this feels confusing. I wonder if it ought to be moved onto CValidationState so any step of the process can set it?

in src/main.cpp: in 7c9d476086 outdated

920-                return state.DoS(0, error("AcceptToMemoryPool : free transaction rejected by rate limiter"),
921-                                 REJECT_INSUFFICIENTFEE, "insufficient priority");
922-            LogPrint("mempool", "Rate limit dFreeCount: %g => %g\n", dFreeCount, dFreeCount+nSize);
923-            dFreeCount += nSize;
924-        }
925+        bool fRateLimit = false;

dcousens commented at 4:37 am on October 11, 2014:

~~Is this meant to be true?~~

luke-jr commented at 4:38 am on October 11, 2014:

No, it’s only potentially set to true inside AcceptMemPoolEntry

luke-jr force-pushed on Oct 11, 2014

luke-jr force-pushed on Oct 12, 2014

in src/main.cpp: in 752418d790 outdated

55@@ -55,6 +56,7 @@ unsigned int nCoinCacheSize = 5000;
56 CFeeRate minRelayTxFee = CFeeRate(1000);
57 
58 CTxMemPool mempool(::minRelayTxFee);
59+CNodePolicy policy;

TheBlueMatt commented at 6:02 pm on October 12, 2014:

Why do we even need an object for policy? isnt it all static? Maybe best to just do a Params()-style Policy()?

jtimon commented at 7:18 pm on October 12, 2014:

~~A params-style global hidden behind a function you mean? I prefer a class.~~

luke-jr commented at 10:32 pm on October 12, 2014:

@TheBlueMatt It isn’t inherently static. It might be nice to one day have nodes with multiple policies, using different ones based on transaction sources and mempools…

jtimon commented at 7:24 pm on October 12, 2014: contributor

Idea ACK. @petertodd should also like this since he was positive about it when I proposed in #4298 On the class name, why not just CPolicy? Maybe better CStandardPolicy if we’re going to later add an abstract CPolicy that CStandardPolicy (and maybe CReplaceByFeePolicy) derive from. More things out of main.o, yes!

in src/init.cpp: in 752418d790 outdated

649@@ -650,6 +650,8 @@ bool AppInit2(boost::thread_group& threadGroup)
650     const char* pszP2SH = "/P2SH/";
651     COINBASE_FLAGS << std::vector<unsigned char>(pszP2SH, pszP2SH+strlen(pszP2SH));
652 
653+    policy.fRequireStandardTx = Params().RequireStandard();

jtimon commented at 7:25 pm on October 12, 2014:

I don’t think this should be called here. This should be in policy.cpp and the field in Policy is not really required.

luke-jr commented at 9:14 am on October 26, 2014:

This has to be called here. When CNodePolicy is initialised, Policy() might not have been yet.

jtimon commented at 7:27 pm on October 12, 2014: contributor

It would be easier to review if you first just moved code as functions with a move-only commit (without indentations) and then created the class and cleaned up. Like @jgarzik did in #4646

laanwj added the label Improvement on Oct 22, 2014

coins: GetTxFees method 7d6dd10f0a

txmempool: lookupConflicts method 025995d0f9

Introduce CNodePolicy for putting isolated node policy code and parameters on

Initially populated with policy-specific code moved from AcceptToMemoryPool.

3b61d6813f

luke-jr force-pushed on Oct 26, 2014

luke-jr commented at 9:28 am on October 26, 2014: member

Rebased with requested changes.

jtimon commented at 1:01 pm on December 21, 2014: contributor

Needs rebase. On top of #5521 ? As said I would avoid the boolean attribute, it doesn’t accomplish its goal of decoupling chainparams from policy.

jtimon commented at 10:00 pm on December 29, 2014: contributor

Here’s a rebased version (not on top of #5521): https://github.com/jtimon/bitcoin/commits/nodepolicy_old

jtimon commented at 1:30 am on December 30, 2014: contributor

Here’s a version rebased on top of #5114: https://github.com/jtimon/bitcoin/tree/nodepolicy_dust Here’s a version rebased on top of #5114 and #5521: https://github.com/jtimon/bitcoin/tree/nodepolicy_5521_dust And, finally, here’s a version on top of the updated #5180: https://github.com/jtimon/bitcoin/tree/nodepolicy_5180

jgarzik commented at 6:23 pm on July 23, 2015: contributor

Closing. No ACKs after a while, and it is intertwined with otherwise fast-moving policy code changes getting merged into the tree.

If this is in error, it can always be re-opened.

jgarzik closed this on Jul 23, 2015

MarcoFalke locked this on Sep 8, 2021

Introduce CNodePolicy for putting isolated node policy code and parameters on #5071