AcceptToMempool: extract various policy functions #7436

pull dcousens wants to merge 4 commits into bitcoin:master from dcousens:expolicy changing 3 files +135 −101

dcousens commented at 2:58 PM on January 28, 2016: contributor

Local policy is often ~~annoying~~ difficult to change due to how large AcceptToMempool is. This PR just begins the process of breaking out various functions such that a user could easily remove/test/isolate them if need-be.

Its WIP for now, and I'd probably prefer to move the definitions to policy/*, but I figured I'd just submit this PR to gauge concept ACK/NACKs before continuing.

I'm not tied to any particular detail of this PR, just wanting to achieve easier configuration of local policy.
jonasschnelli added the label Mempool on Jan 28, 2016
morcos commented at 3:04 PM on January 28, 2016: member

@dcousens One of the things on my list is to try to encapsulate the calculation of all the now consensus critical information that must be stored in a CTxMemPoolEntry. I think isolating that first as well as other checks required for mempool consistency makes sense so its easier to not accidentally break mining while trying to make a policy change.
in src/main.cpp:None in 0f231a925c outdated
```
1154 | @@ -1233,6 +1155,111 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
1155 |      return res;
1156 |  }
1157 |  
1158 | +// TODO: move to policy/
1159 | +bool HasConflicts(CTxMemPool& pool, const CTransaction& tx, set<uint256>& setConflicts) {
```
petertodd commented at 9:34 AM on January 31, 2016:

HasConflicts() is a bit of a weird name for this, given it may return false, yet also set a bunch of conflicts in setConflicts.

Maybe call it "HasReplacableConflicts()"?

jtimon commented at 9:38 PM on February 1, 2016:

Another possibility is to have a GetConflicts method in the mempool and an AcceptConflictsAsReplaceable() function in policy. Or even better, a function AcceptTxReplacement with all the other relay/mining policy stuff that uses setConflicts (though the latter may be more complex).
in src/main.cpp:None in 0f231a925c outdated
```
1198 | +    }
1199 | +
1200 | +    return false;
1201 | +}
1202 | +
1203 | +bool IsNewTransaction(CTxMemPool& pool, CValidationState &state, const CTransaction& tx, CCoinsViewCache& view, CAmount& nValueIn, bool* pfMissingInputs, std::vector<uint256>& vHashTxnToUncache) {
```
jtimon commented at 9:42 PM on February 1, 2016:

I don't think this function should be moved to policy/policy.o. And as said the previous function, if it is to be moved to policy/policy, could take setConflicts from the mempool as const set<uint256>& instead (there's not need for policy/policy to depend on the mempool).

dcousens commented at 12:37 PM on February 2, 2016:

Will be amending these nits within the next 24 hours.
dcousens commented at 9:48 PM on February 2, 2016: contributor

@jtimon/@petertodd I moved HasConflicts to CTxMempool::GetConflicts.

Thoughts?
AcceptToMempool: extract various policy functions d6967451eb
main: s/HasConflicts/HasReplaceableConflicts e8dc634139
MOVE-ONLY: move HasReplaceableConflicts to CTxMempool::GetConflicts 726d1c2043
dcousens commented at 9:56 PM on February 2, 2016: contributor

Rebased on master
IsRateLimited fix, return true not state.DoS 1efdc4fbf0
dcousens renamed this:
~~AcceptToMempool: extract various policy functions [WIP]~~
AcceptToMempool: extract various policy functions
on Feb 2, 2016
jtimon commented at 1:39 PM on February 4, 2016: contributor

That's not what I meant: you moved the policy to the mempool too. I meant calling mempool.getconflicts to get the set of conflicts, and then, with that set, call hasrepleceableConflicts that loops through the set to determine whether the conflicts can be acceptable or not according to the local relay policy. I can draft it in code on top of this PR if my explanation is still not very clear.
dcousens commented at 11:12 PM on February 4, 2016: contributor

@jtimon OK, will amend. It wasn't the policy that was moved, but indeed there was a cross over of replacement logic.
arowser commented at 8:44 AM on May 25, 2016: contributor

Can one of the admins verify this patch?
MarcoFalke added the label Brainstorming on Jun 17, 2016
MarcoFalke commented at 4:58 PM on September 30, 2016: member

@dcousens Is this worth keeping open. (Maybe create a brainstorming issue or try to raise it as short topic in next weeks meeting?)
dcousens closed this on Oct 1, 2016
dcousens deleted the branch on Oct 1, 2016
dcousens commented at 12:50 AM on October 1, 2016: contributor

@MarcoFalke I'll come back to it later, no point keeping open. Modular policy is definitely something I'd still like to invest time into, but not right now.
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Brainstorming Mempool