concept ACK

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

Needs rebase

Rebased

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

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

Nits fixed.

utACK

Rebased

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

once-over utACK

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

@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

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.

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

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.

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.

Needs rebase.

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.