Skip to content

Policy: Prepare AcceptToMemoryPool for encapsulated alternative replacement policies #6416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
jtimon wants to merge 4 commits into from
Closed

Policy: Prepare AcceptToMemoryPool for encapsulated alternative replacement policies #6416

jtimon wants to merge 4 commits into from

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented Jul 10, 2015

As usual with policy changes, this could be cleaner after #6068, but I'd rather not wait anymore to propose this, which has been in my mind since #5071 (because I don't want policy to depend on txmempool).
It would be good that all alternative replacement policy proposals are built on top of this, since the encapsulation makes review easier. Feedback from people that have been working on alternative replacement policies (for example, @petertodd and @luke-jr ) or people that are working on the mempool is specially welcomed.
@morcos I'm thinking that the new CTxMemPool::addUnchecked that removes conflicts before inserting the new transaction may need some call the the fee estimator, but I'm not certain yet. What do you think?

@jtimon jtimon force-pushed the policy-replacement-0.11.99 branch from 1e16a0a to 76a204e Compare July 10, 2015 12:40
@jtimon jtimon mentioned this pull request Jul 10, 2015
Closed
@jtimon jtimon force-pushed the policy-replacement-0.11.99 branch from 107b4b0 to 1a9e990 Compare July 11, 2015 12:19
@jtimon jtimon changed the title Prepare AcceptToMemoryPool for encapsulated alternative replacement policies Policy: Prepare AcceptToMemoryPool for encapsulated alternative replacement policies Jul 11, 2015
@jtimon jtimon force-pushed the policy-replacement-0.11.99 branch from 1a9e990 to 2c9a88e Compare July 12, 2015 11:00
@jtimon jtimon mentioned this pull request Jul 12, 2015
Closed
@jtimon jtimon force-pushed the policy-replacement-0.11.99 branch from 2c9a88e to 8dcae75 Compare July 16, 2015 16:16
@jtimon
Copy link
Contributor Author

jtimon commented Jul 16, 2015

Updated with code from #6421.

@jtimon jtimon mentioned this pull request Jul 16, 2015
Closed
@jtimon jtimon force-pushed the policy-replacement-0.11.99 branch from 8dcae75 to 29f2bf1 Compare July 16, 2015 17:47
@jtimon
Copy link
Contributor Author

jtimon commented Jul 17, 2015

@morcos here's #6449 on top of #6416 : https://github.com/jtimon/bitcoin/commits/morcos-6449-over-6416

Staging replacements and then remove them at the end of AcceptToMemoryPool in some way or another has been something that many people have tried, but working on alternatives to the "first seen" spend conflicts resolution policy was never a priority. Now it seems more priority than ever: apart from capping the mempool, we need some form of RBF so that people can bid up their transactions.
But it is more prioritary to cap the mempool, fine.
Fortunately, some of the first required steps for a "full mempool replacement policy" are the same as what "spend conflicts replacement policies" needs.
Will that mean that some of those common steps for zerconf-safe-RBF will be taken sooner because it's a priority for the mempool cap?
I'm afraid no, any work on this direction, as well as any other improvement to AcceptToMemoryPool() [like #6445 #6420 #6068 ] will be frozen (or mousewheel rebased) until #6421 is fully finished, discussed, tested, merged and improved by later PRs. The reasoning being that, "anything that is simple to do before should be simple to do later".
@sipa @laanwj I hope I'm wrong about my prediction, but if that's the plan I would prefer to know it as soon as possible to close this and some other PRs ( #6448 #6445 #6420 #6068) until #6421.
Even though I believe some of the work mempool-cap work can be greatly simplified and made less disruptive to the rest of the development precisely after those PRs, if I'm the only who believes so, I guess it's just better that I close my related PRs so that people can focus their "mental power" in reviewing only #6421 and PRs based on it.

@jtimon jtimon force-pushed the policy-replacement-0.11.99 branch from 29f2bf1 to fe1c480 Compare July 18, 2015 08:55
@jtimon
Copy link
Contributor Author

jtimon commented Jul 18, 2015

@morcos I would prefer to even go further with the preparations and move more things out of main, see https://github.com/jtimon/bitcoin/commits/policy-replacement-alt (which is closer to #6449 ), but I think it is unlikely that those kind of changes are welcomed for 0.11.1 (even thought it would mean that we can safely change AcceptToMemoryPool in 0.12.99 without having to worry about generating conflicts for a solution that can be backported to 0.11). What I'm trying to avoid is that other changes to AcceptToMemoryPool (of which I maintain many) get locked until 0.11.1, but maybe it's a lost cause...
What do you think @laanwj ?

@jtimon jtimon force-pushed the policy-replacement-0.11.99 branch from fe1c480 to 3501f19 Compare July 20, 2015 18:53
jtimon added 3 commits July 20, 2015 20:55
@jtimon
Optimizations: Consensus: In main::AcceptToMemoryPool, main::ConnectB…
d2f3c16 
…lock, and miner::CreateNewBlock and

In all of them, reject transactions creating new money earlier.
Consensus::CheckTxInputs gets nTxFee as output parameter and is separated from main::CheckInputs [renamed CheckInputsScripts]

- Consensus::CheckTxInputs (called by the rest):

Don't calculate nValueOut twice
Don't check nFees < 0 twice

- main::AcceptToMemoryPool:

Don't call CCoinsViewCache::HaveInputs twice
Don't calculate nValueIn 3 times
Don't calculate nValueOut 5 times

- miner::CreateNewBlock:

Don't call CCoinsViewCache::HaveInputs twice
Don't calculate nValueIn twice
Don't calculate nValueOut 3 times

- main::ConnectBlock:

Still call CCoinsViewCache::HaveInputs twice
Don't calculate nValueIn twice
Don't calculate nValueOut 3 times
@jtimon
Optimization: Unify (fLimitFree && nFees - nFeesDeleted < ::minRelayT…
4db612d 
…xFee.GetFee(nSize)) condition

The following condition won't be checked anymore unless fLimitFree == true:

GetBoolArg("-relaypriority", true) && !AllowFree(view.GetPriority(tx, chainActive.Height() + 1))
@jtimon
Cleanup: GetMinRelayFee() -> AllowBelowMinRelayFee()
abbcdb1
@jtimon jtimon force-pushed the policy-replacement-0.11.99 branch from 3501f19 to acb6575 Compare July 20, 2015 19:27
@jtimon jtimon force-pushed the policy-replacement-0.11.99 branch from acb6575 to 4c51b3f Compare July 20, 2015 21:50
@jtimon jtimon closed this Jul 26, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jtimon @laanwj