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.

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

[Mempool] Improve removal of invalid transactions after reorgs #6915

Merged
merged 8 commits into from Dec 1, 2015

Conversation

Projects
None yet
9 participants
@sdaftuar
Member

sdaftuar commented Oct 30, 2015

@TheBlueMatt This is a rebased version of #6656 (which in turn built off #6595).

As previously reported in #6595, there's currently a bug where we don't remove transactions that become invalid after a reorg due to the transaction's locktime. The solution proposed in #6595 doesn't exhibit great behavior because it doesn't make sense to evict locktime-invalid transactions in the middle of a reorg before the new tip is updated (since typically the block height is the same or greater at the end); @TheBlueMatt addressed this in #6656, which is rebased and included here.

I have also added a commit that vastly improves the efficiency of CTxMemPool::removeForReorg(), by tracking in each mempool entry whether or not the transaction spends a coinbase. Then in removeForReorg(), we only access coins for those transactions that spend a coinbase, instead of looking up all inputs for all transactions in the mempool (which is slow and blows up the UTXO cache).

I compared this code with and without this last commit on some historical data, and observed that this drastically improves runtime of removeForReorg() and reduces memory usage in pcoinsTip. Running this code on historical data on a few days in July, I observed 4 reorgs, and the runtime of removeForReorg() went from ranging between 167ms - 719ms down to between 5.5ms-16ms; meanwhile pcoinsTip memory usage (after the call to removeForReorg()) was reduced by between 144MB - 413MB.

When BIP68 support is merged we'll probably want to update this approach, since IsFinalTx() will become a more expensive function that needs to look at inputs.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Oct 30, 2015

Contributor

concept ACK

Contributor

dcousens commented Oct 30, 2015

concept ACK

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 5, 2015

Contributor

Didnt look too closely, but the last commit seems reasonable.

Contributor

TheBlueMatt commented Nov 5, 2015

Didnt look too closely, but the last commit seems reasonable.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 10, 2015

Contributor

Needs rebase

Contributor

petertodd commented Nov 10, 2015

Needs rebase

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 10, 2015

Member

Rebased

Member

sdaftuar commented Nov 10, 2015

Rebased

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 11, 2015

Member

utACK.

For BIP68 transactions maybe it makes sense to replace the spendscoinbase flag with a minimum height/time at which the transaction becomes valid (which also generalized the wanted behaviour for coinbase-spending transactions).

Member

sipa commented Nov 11, 2015

utACK.

For BIP68 transactions maybe it makes sense to replace the spendscoinbase flag with a minimum height/time at which the transaction becomes valid (which also generalized the wanted behaviour for coinbase-spending transactions).

@morcos morcos referenced this pull request Nov 12, 2015

Merged

Rewrite CreateNewBlock #6898

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 12, 2015

Member

@sipa, @sdaftuar and I discussed adding the BIP68 time and locktime at length.
We were waiting for the code to finalize, and had gotten hung up on the locktime potentially being quite recent and therefore causing a great many transactions to have all 3 heights recalculated on a reorg. But now I'm realizing it makes a lot of sense to only include BIP68 and coinbase height, and then separately scan for locktime since its so easy!

In any case, I think this should be merged and that should be a future improvement.
I will review shortly.

Member

morcos commented Nov 12, 2015

@sipa, @sdaftuar and I discussed adding the BIP68 time and locktime at length.
We were waiting for the code to finalize, and had gotten hung up on the locktime potentially being quite recent and therefore causing a great many transactions to have all 3 heights recalculated on a reorg. But now I'm realizing it makes a lot of sense to only include BIP68 and coinbase height, and then separately scan for locktime since its so easy!

In any case, I think this should be merged and that should be a future improvement.
I will review shortly.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 12, 2015

Member

Nits fixed.

Member

sdaftuar commented Nov 12, 2015

Nits fixed.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 12, 2015

Member

utACK

Member

morcos commented Nov 12, 2015

utACK

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 17, 2015

Member

Rebased

Member

sdaftuar commented Nov 17, 2015

Rebased

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 18, 2015

Contributor

Easiest to read with ?w=0 (omits white space changes).

Contributor

dcousens commented Nov 18, 2015

Easiest to read with ?w=0 (omits white space changes).

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 18, 2015

Contributor

once-over utACK

Contributor

dcousens commented Nov 18, 2015

once-over utACK

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 22, 2015

Member

I think this needs an update for MTP, as it appears to be using adjusted time. @TheBlueMatt

Member

gmaxwell commented Nov 22, 2015

I think this needs an update for MTP, as it appears to be using adjusted time. @TheBlueMatt

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 23, 2015

Member

@gmaxwell Oops -- thanks for catching that. I thought it might make sense to pull removeForReorg out of the mempool and into main, so that we can keep policy code more organized and hopefully duplicate less logic.

If this approach seems okay, I'll try to go back and clean up the commit history here.

ping @TheBlueMatt

Member

sdaftuar commented Nov 23, 2015

@gmaxwell Oops -- thanks for catching that. I thought it might make sense to pull removeForReorg out of the mempool and into main, so that we can keep policy code more organized and hopefully duplicate less logic.

If this approach seems okay, I'll try to go back and clean up the commit history here.

ping @TheBlueMatt

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 23, 2015

Contributor

I'm not a huge fan. This isnt actually policy code, its code who's behavior is derived from consensus rules, so I dont think it belongs in main. I'd much rather pass the MTP spendable-height to CTxMemPool::...() than to do this all in main.

Contributor

TheBlueMatt commented Nov 23, 2015

I'm not a huge fan. This isnt actually policy code, its code who's behavior is derived from consensus rules, so I dont think it belongs in main. I'd much rather pass the MTP spendable-height to CTxMemPool::...() than to do this all in main.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 23, 2015

Member

Ok I tried the smaller change of just passing through the MTP into removeForReorg, is this what you had in mind?

Member

sdaftuar commented Nov 23, 2015

Ok I tried the smaller change of just passing through the MTP into removeForReorg, is this what you had in mind?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 24, 2015

Member

18:22 < BlueMatt> anyway, I'd prefer to not duplicate the code....can we pull that out
into a function?

@TheBlueMatt: Yeah I also didn't want to duplicate that code, but it seemed like the right way to do that would be to reuse the CheckFinalTx function, which does very little, here's what it looks like:

bool CheckFinalTx(const CTransaction &tx, int flags)
{
    AssertLockHeld(cs_main);

    // By convention a negative value for flags indicates that the
    // current network-enforced consensus rules should be used. In
    // a future soft-fork scenario that would mean checking which
    // rules would be enforced for the next block and setting the
    // appropriate flags. At the present time no soft-forks are
    // scheduled, so no flags are set.
    flags = std::max(flags, 0);

    // CheckFinalTx() uses chainActive.Height()+1 to evaluate
    // nLockTime because when IsFinalTx() is called within
    // CBlock::AcceptBlock(), the height of the block *being*
    // evaluated is what is used. Thus if we want to know if a
    // transaction can be part of the *next* block, we need to call
    // IsFinalTx() with one more than chainActive.Height().
    const int nBlockHeight = chainActive.Height() + 1;

    // Timestamps on the other hand don't get any special treatment,
    // because we can't know what timestamp the next block will have,
    // and there aren't timestamp applications where it matters.
    // However this changes once median past time-locks are enforced:
    const int64_t nBlockTime = (flags & LOCKTIME_MEDIAN_TIME_PAST)
                             ? chainActive.Tip()->GetMedianTimePast()
                             : GetAdjustedTime();

    return IsFinalTx(tx, nBlockHeight, nBlockTime);
}

We're basically duplicating all the code here (except maybe that std::max(flags, 0); I don't really understand what that is) already, so pulling out the one line to avoid duplicating that doesn't seem worth it... I'd rather find a way to reuse the whole function (which is what I did in my first attempt).

My only other idea here would be to change CTxMemPool::removeForReorg to take a "flags" argument only, and then invoke CheckFinalTx instead of IsFinalTx.

Member

sdaftuar commented Nov 24, 2015

18:22 < BlueMatt> anyway, I'd prefer to not duplicate the code....can we pull that out
into a function?

@TheBlueMatt: Yeah I also didn't want to duplicate that code, but it seemed like the right way to do that would be to reuse the CheckFinalTx function, which does very little, here's what it looks like:

bool CheckFinalTx(const CTransaction &tx, int flags)
{
    AssertLockHeld(cs_main);

    // By convention a negative value for flags indicates that the
    // current network-enforced consensus rules should be used. In
    // a future soft-fork scenario that would mean checking which
    // rules would be enforced for the next block and setting the
    // appropriate flags. At the present time no soft-forks are
    // scheduled, so no flags are set.
    flags = std::max(flags, 0);

    // CheckFinalTx() uses chainActive.Height()+1 to evaluate
    // nLockTime because when IsFinalTx() is called within
    // CBlock::AcceptBlock(), the height of the block *being*
    // evaluated is what is used. Thus if we want to know if a
    // transaction can be part of the *next* block, we need to call
    // IsFinalTx() with one more than chainActive.Height().
    const int nBlockHeight = chainActive.Height() + 1;

    // Timestamps on the other hand don't get any special treatment,
    // because we can't know what timestamp the next block will have,
    // and there aren't timestamp applications where it matters.
    // However this changes once median past time-locks are enforced:
    const int64_t nBlockTime = (flags & LOCKTIME_MEDIAN_TIME_PAST)
                             ? chainActive.Tip()->GetMedianTimePast()
                             : GetAdjustedTime();

    return IsFinalTx(tx, nBlockHeight, nBlockTime);
}

We're basically duplicating all the code here (except maybe that std::max(flags, 0); I don't really understand what that is) already, so pulling out the one line to avoid duplicating that doesn't seem worth it... I'd rather find a way to reuse the whole function (which is what I did in my first attempt).

My only other idea here would be to change CTxMemPool::removeForReorg to take a "flags" argument only, and then invoke CheckFinalTx instead of IsFinalTx.

@NicolasDorier

This comment has been minimized.

Show comment
Hide comment
@NicolasDorier

NicolasDorier Nov 26, 2015

Member

in #6312 IsFinalTx (renamed LockTime) is responsible for calculating the nBlockTime from flags.
LockTime function can be called by removeForReorg with the passed flag, this would prevent code duplication.

6312 is ready to merge once it get more ACK, this could be rebased on it, would make things simpler.

Member

NicolasDorier commented Nov 26, 2015

in #6312 IsFinalTx (renamed LockTime) is responsible for calculating the nBlockTime from flags.
LockTime function can be called by removeForReorg with the passed flag, this would prevent code duplication.

6312 is ready to merge once it get more ACK, this could be rebased on it, would make things simpler.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 28, 2015

Member

Needs rebase.

Member

sipa commented Nov 28, 2015

Needs rebase.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 30, 2015

Member

Rebased, and I changed the last commit to call CheckFinalTx. I don't love this, but it avoids the code duplication of having all the callers separately calculate what time to pass in.

@NicolasDorier We'll want to reimplement this function after #6312, since LockTime will be a more expensive function to call. I'm happy to tackle that when the time comes, but I don't want to rebase this on #6312 since this is tagged for 0.12 and fixes a bug. For now, I think this formulation should be easy to merge into #6312, since I'm now just passing the flags variable through to CheckFinalTx.

Member

sdaftuar commented Nov 30, 2015

Rebased, and I changed the last commit to call CheckFinalTx. I don't love this, but it avoids the code duplication of having all the callers separately calculate what time to pass in.

@NicolasDorier We'll want to reimplement this function after #6312, since LockTime will be a more expensive function to call. I'm happy to tackle that when the time comes, but I don't want to rebase this on #6312 since this is tagged for 0.12 and fixes a bug. For now, I think this formulation should be easy to merge into #6312, since I'm now just passing the flags variable through to CheckFinalTx.

@laanwj laanwj merged commit 2d8860e into bitcoin:master Dec 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 1, 2015

Merge pull request #6915
2d8860e Fix removeForReorg to use MedianTimePast (Suhas Daftuar)
b7fa4aa Don't call removeForReorg if DisconnectTip fails (Suhas Daftuar)
7e49f5f Track coinbase spends in CTxMemPoolEntry (Suhas Daftuar)
bb8ea1f removeForReorg calls once-per-disconnect-> once-per-reorg (Matt Corallo)
474b84a Make indentation in ActivateBestChainStep readable (Matt Corallo)
b0a064c Fix comment in removeForReorg (Matt Corallo)
9b060e5 Fix removal of time-locked transactions during reorg (Matt Corallo)
0c9959a Add failing test checking timelocked-txn removal during reorg (Matt Corallo)

str4d added a commit to str4d/zcash that referenced this pull request Feb 3, 2018

@str4d str4d referenced this pull request Feb 3, 2018

Merged

Overwinter SignatureHash #2903

str4d added a commit to str4d/zcash that referenced this pull request Feb 8, 2018

zkbot added a commit to zcash/zcash that referenced this pull request Feb 8, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements zcash/zips#129.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#6915
  - Only the rework of `mempool.check()` calls that the next PR depends on.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of  #2254. Closes #1408 and #2584.

str4d added a commit to str4d/zcash that referenced this pull request Feb 8, 2018

zkbot added a commit to zcash/zcash that referenced this pull request Feb 8, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements zcash/zips#129.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#6915
  - Only the rework of `mempool.check()` calls that the next PR depends on.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of  #2254. Closes #1408 and #2584.

str4d added a commit to str4d/zcash that referenced this pull request Feb 8, 2018

str4d added a commit to str4d/zcash that referenced this pull request Feb 14, 2018

str4d added a commit to str4d/zcash that referenced this pull request Feb 16, 2018

zkbot added a commit to zcash/zcash that referenced this pull request Feb 19, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements zcash/zips#129.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#6915
  - Only the rework of `mempool.check()` calls that the next PR depends on.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of #2074 and #2254. Closes #1408 and #2584.

zkbot added a commit to zcash/zcash that referenced this pull request Feb 20, 2018

Auto merge of #2940 - str4d:nu-activation-mempool-expiry, r=str4d
Mempool improvements, branch ID awareness

Whenever the local chain tip is updated, transactions in the mempool which commit to an
unmineable branch ID (for example, just before a network upgrade activates, where the
next block will have a different branch ID) will be removed.

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6654
  - Only the mempool index change.
- bitcoin/bitcoin#6776
- bitcoin/bitcoin#7020
- bitcoin/bitcoin#6915

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request Feb 20, 2018

Auto merge of #2940 - str4d:nu-activation-mempool-expiry, r=str4d
Mempool improvements, branch ID awareness

Whenever the local chain tip is updated, transactions in the mempool which commit to an
unmineable branch ID (for example, just before a network upgrade activates, where the
next block will have a different branch ID) will be removed.

Includes commits cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6654
  - Only the mempool index change.
- bitcoin/bitcoin#6776
- bitcoin/bitcoin#7020
- bitcoin/bitcoin#6915

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