LockTime::from_height could use a clarification for the meaning of height

[TheBlueMatt]

Is the height the height at which the transaction can be included? Or the height at which the transactions can be included in the mempool?

[tnull]

Is the height the height at which the transaction can be included? Or the height at which the transactions can be included in the mempool?

I think both options you mention are the same?

The nLockTime height is the last height for which the transaction cannot be spent (i.e., is locked), or to quote Murch:

If the locktime is less than 500,000,000, the locktime is interpreted as a block height. The locktime defines the greatest block height that cannot include the transaction. The transaction may be included in any block with a greater block height than its locktime.

I agree it would be nice to clarify this in rust-bitcoin docs though.

[TheBlueMatt]

I think both options you mention are the same?

I was thinking of it as "height X means you can enter the mempool at X and get confirmed in X+1" vs "height X means you can enter the block at X and the mempool at X-1". I always confuse the two so would be nice to have it in the docs :)

[apoelstra]

Agreed, would be nice to have this in the docs! But also agree that the description in the OP is still pretty confusing. I like Matt's "is allowed in block X, meaning that the mempool will allow it at block X-1" (or X+1/X if that's the correct thing -- I also don't remember offhand).

[tcharding]

I've never heard of this so I went digging in Core to see if I could learn something, I've come up with a counter view to both @TheBlueMatt and @apoelstra so in a shameless call to authority I'm guessing I'm missing something.

In validation.cpp function CheckFinalTxAtTip we have

    // CheckFinalTxAtTip() uses active_chain_tip.Height()+1 to evaluate
    // nLockTime because when IsFinalTx() is called within
    // 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 active_chain_tip.Height().
    const int nBlockHeight = active_chain_tip.nHeight + 1;

Then we call IsFinalTx passing in nBlockHeight.

And over in node/miner.cpp function CreateNewBlock we set nHeight with

    CBlockIndex* pindexPrev = m_chainstate.m_chain.Tip();
    assert(pindexPrev != nullptr);
    nHeight = pindexPrev->nHeight + 1;

Then we call IsFinalTx using TestPackageTransactions by way of addPackageTxs (I think).

So to me it seems that both the mempool and the next block will accept a tx with locktime upto and including the value of the current chain tip.

For completeness, in IsFinalTx we check against the locktime (blockheight) arg using <

bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
{
    if (tx.nLockTime == 0)
        return true;
    if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))
        return true;

[apoelstra]

So to me it seems that both the mempool and the next block will accept a tx with locktime upto and including the value of the current chain tip.

Yes, and the next block's height will be this value +1 :).

Or are you just arguing that X/X-1 should be X+1/X in our comments? In that case you're probably right, given that you read the source and I didn't.

[TheBlueMatt]

I'm always confused about this, so I trust Tobin for reading the source and tnull because he quoted Murch who is the authority on how bitcoin works.

[tcharding]

My point is that there is no X+1 (or X-1), only X

Both the check for inclusion in the mempool and inclusion in the next block (the one being mined) both add 1 to the locktime height then use < in other words they are the same. So if height allows inclusion in the next block it will also allow entry into the mempool. (All based solely on my reading today of the quoted code.) Is it possible that Core was different at sometime in the past that led you two to both have the +1/-1 thing in mind?

[TheBlueMatt]

One person's >= X is another's > X + 1 🤷‍♂️

[sanket1729]

@tcharding, I believe adding a adding comment roughly stating the transaction is safe to broadcast when the current chain tip is at Height h should resolve the issue.

[apoelstra]

@tcharding you say "there is no +1" but then use the word "next" twice :P. "next" means plus one.

[tcharding]

Lol, so it does.