CreateNewBlock: Child-pays-for-parent / Add transaction fee later #1647

Closed
wants to merge 2 commits into
from

Projects

None yet
@luke-jr
Member
luke-jr commented Aug 1, 2012

Status: Passes unit tests

Consider parent transactions in the "cost" of child transactions until confirmed, and confirm them together

This is the part of #1240 that @gavinandresen left out of #1590 since he felt it belonged in a separate commit/pullreq.

@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/27b24244c4d1f97690d5908e3412ac5306920281 for binaries and test log.

@luke-jr luke-jr referenced this pull request Aug 13, 2012
Closed

next 2012-08-13 #1671

@mikehearn

Gavin, any thoughts on this?

@gavinandresen
Member

@mikehearn : needs unit tests, in my humble opinion. It is a very good candidate for "needs 100% code coverage from tests", because transaction selection is such a key piece of the Bitcoin infrastructure.

Also needs thorough code review, with an eye towards "Could I construct a series of transactions that made the selection algorithm take O(N^2) time ?"

@luke-jr
Member
luke-jr commented Nov 1, 2012

@gavinandresen What kind of unit tests would you like for this? I already dealt with the O(N^2) problem a while back on Eligius, though of course more reviews are always welcome.

@gmaxwell
Member
gmaxwell commented Nov 1, 2012

My suggestion would be start with a bunch of "random" mempools and them making sure that the selections it makes are valid (no mistaken dependencies) and actually get the most fees possible (e.g. by externally precomputing the correct answers). I'd also include cases designed to trigger complexity attacks (mempool with a 2 groups of 100 long chains or whatever).

After that I'd run lcov with the test and look and make sure every conceivably reachable branch (e.g. all except the invisible boost added heap allocation failure tests) is hit by the tests. If not, add tests that trigger them.

After that the code should be intentionally broken (e.g. by randomly removing some statements and/or turning some if() to if(!())) and make sure the tests fail. I volunteer to do this kind of testing (plus some basic smoke tests) once regular unit tests for this are written.

@gavinandresen
Member

Thanks @gmaxwell , those are exactly the types of tests I think this needs!

@luke-jr
Member
luke-jr commented Nov 13, 2012

Update: It seems since rebasing this, new performance problems have cropped up. :(

@luke-jr
Member
luke-jr commented Nov 24, 2012

Update: False alarm, I debugged the performance problem and it was a result of a poorly thought-out merge of this with #1648 (which elevated priority of transactions that benefit the miner directly). This seems to still perform well without that change (or with it done once).

@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/56c7ea61d70d4a6699f1a53b9d03eb803a9c58c7 for binaries and test log.

@luke-jr
Member
luke-jr commented Jan 12, 2013

Fixed a bug triggered by today's onslaught of !IsFinal transactions...

@gavinandresen
Member

Rebase needed.

Although I think this needs a re-think/rewrite:

When we have a memory-limited mempool (needed for anti-DoS), we'll run into a chicken-and-egg problem: parent transaction may be evicted from mempool, child will get stuck (and eventually evicted) from the orphan pool. Seems to me what is needed is a new protocol message that is "here is a bundle of dependent transactions, with children that pay for their parents."

@luke-jr
Member
luke-jr commented Oct 25, 2013

Rebased. I agree it isn't perfect, but this is 1) better than nothing, and 2) well-tested. If anyone wants to put the effort into a rewrite, I'd be glad to defer and give it testing.. but I think everyone's busy enough already.

@ABISprotocol

Thank you for your work on this, it would be good to see it in a 0.9 release, am curious if tests are completed.

@luke-jr
Member
luke-jr commented Jan 23, 2014

I'm not sure if I'm going to have time to do tests before 0.9 - hopefully someone else can help out with that. :(

@ABISprotocol

If the tests for this need any funding support from some bounty fund and if there is not support or priority, it's possible the tests may be able to be bountyfied (ok, not a word) as part of something I'm working on, it's probably 60 days out though before any funds would be available.

@ABISprotocol

#3753 also just had merge conflicts on testing (failed merge).

@luke-jr
Member
luke-jr commented Jun 26, 2014

Rebased, but the changes were significant enough that IMO this needs re-review and re-testing :(

@BitcoinPullTester

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/p1647_200f8abb943e5acd3bf599b4bfa8e9beccd53d1f/ for test log.

This pull does not merge cleanly onto current master
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@dgenr8 dgenr8 added a commit to dgenr8/bitcoin that referenced this pull request Aug 13, 2014
@dgenr8 dgenr8 Rebase #1647 ff9870a
@dgenr8
dgenr8 commented Aug 15, 2014

How much mining actually occurs using miner.cpp? Is it basically example code at this point? Since CPFP makes economic sense for miners, wouldn't they have implemented it already? I admit to cluelessness about the mining landscape.

@luke-jr
Member
luke-jr commented Aug 15, 2014

@dgenr8 It's supposed to be example code, but unfortunately a lot of miners still end up using it as-is today. :(

@laanwj
Member
laanwj commented Aug 15, 2014

Well "the internal miner" is example code. It's slow and useless.

However miner.cppalso contains code for transaction selection from the mempool, which is used for getblocktemplate, which is used by many miners.

@laanwj laanwj added this to the 0.11.0 milestone Dec 29, 2014
@laanwj
Member
laanwj commented Dec 29, 2014

Labeling this for 0.11. This is the oldest pull request open, let's finally get it in by then.

@voisine
voisine commented Mar 6, 2015

Would love to see this get in. Have a situation right now where a new user received 0.01btc tx with no fee and then spent their entire balance, 17btc, which is stuck. It would be a real improvement for user experience if wallet software could just include a fee to cover unconfirmed input tx in addition to itself instead of trying to explain to users why they can't spend those funds yet.

@gavinandresen
Member

Needs tests, as suggested by gmaxwell. A regression test making sure that this would actually fix the situation described by @voisine would be excellent (although non-trivial-- do you assume that the 0.01btc tx with no fee got relayed and is still in memory pools, or not?)

And needs a good review from somebody who is good at spotting potential DoS weaknesses (like Sergio).

@ABISprotocol

Yes, thanks to those who have worked on this so far. This being the oldest open pull request, and having merits for inclusion in 0.11, it needs and deserves both a thoughtful review and merge.

@jgarzik
Member
jgarzik commented Mar 21, 2015

ACK - light test at branch's current tip

@sipa sipa commented on the diff Mar 21, 2015
src/miner.cpp
for (map<uint256, CTxMemPoolEntry>::iterator mi = mempool.mapTx.begin();
mi != mempool.mapTx.end(); ++mi)
{
const CTransaction& tx = mi->second.GetTx();
+
+ const uint256& hash = tx.GetHash();
+ CTxInfo& txinfo = mapInfoById[hash];
+ txinfo.hash = hash;
+ txinfo.pmapInfoById = &mapInfoById;
@sipa
sipa Mar 21, 2015 Bitcoin member

This cyclic dependency is pretty ugly. Can't you just pass the mapInfoById to the methods in txindo that need it?

@luke-jr
luke-jr Mar 21, 2015 Bitcoin member

That's basically everything... :/

@sipa
sipa Mar 23, 2015 Bitcoin member

It's easier to not put the logic in the data structure, I think. The fact that you need a pointer from the low-level data structure to the container is just a symptom of that.

@sipa
Member
sipa commented Mar 21, 2015

Has anyone done benchmarks on the impact on block creation time?

@morcos morcos commented on the diff Apr 6, 2015
src/miner.cpp
{
- if (!porphan->setDependsOn.empty())
+ fResort = true;
@morcos
morcos Apr 6, 2015 collaborator

Aren't this !empty check and flag to resort redundant given the foreach loop below?

@luke-jr
luke-jr Apr 6, 2015 Bitcoin member

Probably better to just set fResort here and not in the foreach... (done)

@morcos morcos and 1 other commented on an outdated diff Apr 6, 2015
src/miner.cpp
{
- BOOST_FOREACH(COrphan* porphan, mapDependers[hash])
+ pblock->vtx.push_back(*ptxinfo->ptx);
+ pblocktemplate->vTxFees.push_back(ptxinfo->nTxFee);
+ pblocktemplate->vTxSigOps.push_back(ptxinfo->nTxSigOps);
@morcos
morcos Apr 6, 2015 collaborator

I think that ptxinfo->nTxSigOps is just that transaction's GetP2SHSigOpCount and doesn't include its GetLegacySigOpCount.

@luke-jr
luke-jr Apr 6, 2015 Bitcoin member

Good catch, fixed.

@luke-jr luke-jr CreateNewBlock: Consider parent transactions in the "cost" of child t…
…ransactions until confirmed, and confirm them together
102a33a
@morcos
Collaborator
morcos commented Apr 8, 2015

I've done a code review on this and it looks good to me. I've also done some performance testing in manufactured RPC tests as well as under more typical load. It can cause CreateNewBlock to take multiples as long if there are chains of transactions in the mempool, so I do think it would be useful to test deliberately extreme chains as mentioned above. Under typical mempool activity though it seems to be about a 40% hit, making CreateNewBlock take on average 200ms instead of 140ms. Before and after this change the maximum time on a typical mempool is about 1 second.

EDIT: see later comment, has some issues.

@luke-jr
Member
luke-jr commented Apr 9, 2015

@morcos If you want to push those RPC tests as a branch somewhere, I can include them when I rebase this.

@morcos
Collaborator
morcos commented Apr 20, 2015

@luke-jr Feel free to take a look at this, https://github.com/morcos/bitcoin/commits/CPFP. I'd rebased and then added 2 commits to do some timing. Its not really something to include as it doesn't test anything, but its just what I was using to stress the code. I'm not quite sure why it takes so long, but I didn't look into. See the commit text for the most recent commit.

@jtimon jtimon commented on the diff Apr 25, 2015
src/miner.cpp
+ if (mempool.mapTx.count(txin.prevout.hash))
+ {
+ // Input is still unconfirmed
+ const uint256& hashPrev = txin.prevout.hash;
+ nValueIn = mempool.mapTx[hashPrev].GetTx().vout[txin.prevout.n].nValue;
+ txinfo.addDependsOn(hashPrev);
+ mapInfoById[hashPrev].setDependents.insert(hash);
+ nConf = 0;
+ }
+ else
+ {
+ // We don't know where the input is
+ // In this case, it's impossible to include this transaction in a block, so mark it invalid and move on
+ txinfo.fInvalid = true;
+ LogPrintf("priority %s invalid input %s\n", txinfo.hash.ToString().substr(0,10).c_str(), txin.prevout.hash.ToString().substr(0,10).c_str());
+ goto nexttxn;
@jtimon
jtimon Apr 25, 2015 collaborator

What about replacing this part

                    txinfo.fInvalid = true;
                    goto nexttxn;
                 }
                  nTotalIn += nValueIn;
             }
             nTxFee = nTotalIn - tx.GetValueOut(); 
             mempool.ApplyDeltas(hash, dPriority, nTotalIn);
             vecPriority.push_back(&txinfo); 
nexttxn:    (void)1;

with the following?

                    txinfo.fInvalid = true;
                 }
                 if (!txinfo.fInvalid)
                    nTotalIn += nValueIn;
             }
             if (!txinfo.fInvalid) {
                 nTxFee = nTotalIn - tx.GetValueOut(); 
                 mempool.ApplyDeltas(hash, dPriority, nTotalIn);
                 vecPriority.push_back(&txinfo);
             }

https://xkcd.com/292/

@jtimon
jtimon Apr 25, 2015 collaborator

Or just have a separate function:

if (NewFunction(...)) {
    nTotalIn += nValueIn;
    nTxFee = nTotalIn - tx.GetValueOut(); 
    mempool.ApplyDeltas(hash, dPriority, nTotalIn);
    vecPriority.push_back(&txinfo);
}
@laanwj laanwj removed this from the 0.11.0 milestone May 18, 2015
@morcos
Collaborator
morcos commented Jul 15, 2015

I think I missed a few things on my first review of this. It seems like if B and C are both children of A, and D depends on both B and C, then the effectiveSize() of D will double count the size of A?

Also, it doesn't look like the fees of A, B, or C are counted, only the fees of the final child (D in this case).

Or am I missing something now...

@jgarzik
Member
jgarzik commented Sep 15, 2015

I leave this pull request open in the hope that it's current status of "98% there" will get the final way there. @luke-jr has faithfully updated it and addressed feedback over time, and consensus is that it's merge worthy.

It seems like it just needs a reviewer or two to expend a day or two of focused analysis and testing.

@sipa
Member
sipa commented Sep 15, 2015
@morcos
Collaborator
morcos commented Sep 15, 2015

Its actually a fair amount of code that will still need to be written to do CPFP even after #6654, but I agree that it'll provide the right framework for implementing it. I think the approach of starting from the dependent tracking framework and adding ancestor tracking is likely to get us to functional CPFP quicker than starting from this pull.

@luke-jr
Member
luke-jr commented Sep 16, 2015

I agree the long-term implementation is likely to be different, but given the extreme maturity of this code (especially as opposed to the immaturity of #6654 and similar proposals), I do think it would be best to merge it now/soon, with the understanding that it may be replaced by an improved implementation later. Don't let the perfect be the enemy of the good etc.

@morcos
Collaborator
morcos commented Sep 16, 2015

@luke-jr did you see my comment a few comments up? I think there are a couple of things that don't work properly with the code now.

@jtimon
Collaborator
jtimon commented Sep 16, 2015

I think it makes sense to try another version of #6654 on top of this if it's not too much work and take a look at that branch before merging #6654

@luke-jr
Member
luke-jr commented Sep 16, 2015

I think I missed a few things on my first review of this. It seems like if B and C are both children of A, and D depends on both B and C, then the effectiveSize() of D will double count the size of A?

Yes, probably. But it's still an improvement over not considering the dependency at all.

Also, it doesn't look like the fees of A, B, or C are counted, only the fees of the final child (D in this case).

IIRC this is intentional.

@laanwj
Member
laanwj commented Sep 25, 2015

Closing this for now. Should be reopened (or implemented otherwise) after the mempool refactor work.

@laanwj laanwj closed this Sep 25, 2015
@dcousens

Maybe @laanwj tag it as blocked and keep it open?
I'm still not sure how this 'roadmap' is being tracked?

@luke-jr
Member
luke-jr commented Feb 14, 2016

After 2 or 3 failed attempts to rewrite/port CPFP to 0.12, I am abandoning CPFP for the time being. Nobody seems to use it, and it's too complex.

Pretty sure the older versions also have a bug that they don't update grandchildren priority/feerate when their grandparents get mined. (Not a big deal.)

@schildbach

@luke-jr Bitcoin Wallet users use it quite a bit. Current problem is only a few miners have implemented it.

@luke-jr
Member
luke-jr commented Feb 14, 2016

Hmm, good to know. It's not going to make Knots 0.12.0 though - maybe I'll reconsider it later.

@rebroad
rebroad commented Mar 1, 2016

Sorry if I'm being thick, I can see how this patch allows the recipient of a transaction to effectively pay the fee, but how often is this actually needed?

@sipa
Member
sipa commented Mar 1, 2016
@schildbach

@rebroad Especially in times like this week, with the network being very unreliable, CPFP is used a lot. Based on my logs, I'd say several thousand times a day.

@voisine
voisine commented Apr 7, 2016

Just wanted to add that we use CPFP in breadwallet as well, when spending unconfirmed inputs that don't come from the wallet's own change outputs. It's needed to get transactions unstuck if a user tries to spend their entire wallet balance including recent unconfirmed low/no-fee inputs.

@schildbach

@voisine I assume in this case, the CPFP transaction is the same as the (presumably big) transaction that empties the wallet? Or are you somehow splitting it up into txns that act as CPFP and a transaction that empties the wallet?

@voisine
voisine commented Apr 7, 2016

this is if the user tries to spend more funds in a single tx than they have confirmed utxos that can satisfy, typically when emptying the wallet shortly after a receive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment