Unexplained 1 million block height in mempool code

[instagibbs]

Where is this magic number from?

bitcoin/src/txmempool.cpp

Line 603 in fb52d06

UpdateCoins(tx, mempoolDuplicate, 1000000);

[fanquake]

Looks like it was introduced by @TheBlueMatt in #5267.

[promag]

A block far far away?

[instagibbs]

It's not for testnet.

…

On Mon, Jan 28, 2019, 7:35 PM João Barbosa ***@***.*** wrote: A block far far away? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15080 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgC04pUw8iuSBR1Ae1XhOlVJ5P6azI-ks5vH1EOgaJpZM4Zm8HF> .

[instagibbs]

I'm having a really difficult time convincing myself this behavior could not lead to incorrect results. But this can only be hit when -checkmempool is non-0, aka -regtest by default so it's not used in production.

I think here the spendheight is the correct height since that is where this duplicate mempool is actually at.

I'll think a little more how a false assertion or validation failure could hit if unchanged.

[mzumsande]

I think that the height currently specified as a magic number is never accessed:

In check(), the ancestor tx of the mempool are "spent" in the UpdateCoins() function (adding new UTXOs to the local cache mempoolDuplicate), providing the inputs for subsequent spending of the dependent tx, and thereby allowing to check that all tx in the mempool can be mined in some order (no orphans).

While all ancestors need to be added to mempoolDuplicate, the height at which they are added is not checked in mempoolDuplicate.HaveInputs(), just their COutPoint.

I did some testing with a height of 0 instead of 1E6 and didn't see any effect.
So I don't see how the current implementation could lead to a bug, though I agree that using spendheight would make more sense and might prevent future bugs.

[instagibbs]

Lines up with my current thinking.