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

luke-jr · 2014-10-10T03:18:44Z

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.

Diapolo · 2014-10-10T08:17:43Z

src/policy.cpp

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


Nit: Just MIT

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.

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.

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.

dcousens · 2014-10-11T04:33:12Z

src/main.cpp

+        bool fRateLimit = false;
+        if (!policy.AcceptMemPoolEntry(pool, state, entry, view, fRateLimit))
+            return false;
+        if (!fLimitFree)


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

fRateLimit = !fLimitFree ? false : false

You could just write:

if (!policy.AcceptMemPoolEntry(pool, state, entry, view, false)) return false; if (!policy.RateLimitTx(pool, state, entry, view)) return false;

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

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).

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

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

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

TheBlueMatt · 2014-10-12T18:02:42Z

src/main.cpp

@@ -55,6 +56,7 @@ unsigned int nCoinCacheSize = 5000;
 CFeeRate minRelayTxFee = CFeeRate(1000);

 CTxMemPool mempool(::minRelayTxFee);
+CNodePolicy policy;


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

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

@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 · 2014-10-12T19:24:40Z

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!

jtimon · 2014-10-12T19:25:48Z

src/init.cpp

@@ -650,6 +650,8 @@ bool AppInit2(boost::thread_group& threadGroup)
    const char* pszP2SH = "/P2SH/";
    COINBASE_FLAGS << std::vector<unsigned char>(pszP2SH, pszP2SH+strlen(pszP2SH));

+    policy.fRequireStandardTx = Params().RequireStandard();


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

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

jtimon · 2014-10-12T19:27:14Z

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

…eters on Initially populated with policy-specific code moved from AcceptToMemoryPool.

luke-jr · 2014-10-26T09:28:04Z

Rebased with requested changes.

jtimon · 2014-12-21T13:01:36Z

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 · 2014-12-29T22:00:09Z

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

jtimon · 2014-12-30T01:30:23Z

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 · 2015-07-23T18:23:19Z

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.

luke-jr force-pushed the nodepolicy branch from 2e28ef7 to f9b84ba Compare October 10, 2014 03:31

Diapolo reviewed Oct 10, 2014
View reviewed changes

luke-jr force-pushed the nodepolicy branch 2 times, most recently from ad1dcdb to 7c9d476 Compare October 11, 2014 03:41

dcousens reviewed Oct 11, 2014
View reviewed changes

luke-jr force-pushed the nodepolicy branch 3 times, most recently from ba69a22 to 752418d Compare October 12, 2014 02:30

TheBlueMatt reviewed Oct 12, 2014
View reviewed changes

jtimon reviewed Oct 12, 2014
View reviewed changes

jtimon mentioned this pull request Oct 21, 2014

Policy: Move CTxOut::IsDust() to policy.o #5114

Closed

laanwj added the Refactoring label Oct 22, 2014

luke-jr added 3 commits October 26, 2014 09:16

coins: GetTxFees method

7d6dd10

txmempool: lookupConflicts method

025995d

Introduce CNodePolicy for putting isolated node policy code and param…

3b61d68

…eters on Initially populated with policy-specific code moved from AcceptToMemoryPool.

luke-jr force-pushed the nodepolicy branch from 752418d to 3b61d68 Compare October 26, 2014 09:26

jtimon mentioned this pull request Oct 30, 2014

WIP: Policy: Separate standard and testing policies #5180

Closed

jtimon mentioned this pull request Dec 21, 2014

Reject non-final txs even in testnet/regtest #5521

Merged

This was referenced Jan 7, 2015

Policy: Create CPolicy interface and CStandardPolicy class implementing it #5595

Closed

Encapsulate policy #4943

Closed

Policy: Continue policy movements #5652

Closed

jtimon mentioned this pull request Jul 10, 2015

Policy: Prepare AcceptToMemoryPool for encapsulated alternative replacement policies #6416

Closed

jgarzik closed this Jul 23, 2015

jtimon mentioned this pull request Nov 1, 2015

Policy: Create CPolicy interface and CStandardPolicy class implementing it #6068

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

luke-jr commented Oct 10, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Oct 12, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Oct 12, 2014

Uh oh!

luke-jr commented Oct 26, 2014

Uh oh!

jtimon commented Dec 21, 2014

Uh oh!

jtimon commented Dec 29, 2014

Uh oh!

jtimon commented Dec 30, 2014

Uh oh!

jgarzik commented Jul 23, 2015

Uh oh!

Uh oh!