Build on #6312 commit added is 0559bb24e6e92a2d790c77707e464aa1e17919ca :
Instead of checking if the transaction is in a category of cases to return true and failing otherwise, we return false if the tx is a failure case and otherwise return true. This removes an early-exit opportunity which will no longer be possible once sequence numbers are re-enabled, but has no change in behaviour.
Breaks up the modifications to IsFinalTx() / LockTime() for easier review. This commit does not change the logic of IsFinalTx().
In summary, the sequence number of each input in a transaction can be used to store a relative locktime: a delta value which is added to the height or time of the block which included the output being spent to generate a per-input lock time from the sequence number. If enforced, this would make consensus-enforced transaction replacement possible as replacing an input with a lower sequence number / relative lock time would allow the new transaction to make it on the chain before previous versions of the transaction become valid.
This commit does not implement the transaction replacement logic. Nor does this commit make the relative lock time rules as consensus or policy rules. It merely adds new, but disabled functionality and unit tests of that functionality.
Transactions that fail relative lock time checks will be rejected from the mempool and not included in generated blocks, making it easy to test the feature. However blocks including transactions containing "invalid" relative lock times will still be accepted; this is *not* the soft-fork required to actually enable relative lock times for production use.
LockTime() : the nCoinTime of a confirmed coin does not depend on MTP
flag, so the nCoinTime of an unconfirmed also should not be.
Addressing sipa's nits
_SECONDS_FLAG to _TYPE_FLAG
_DISABLED_FLAG to _DISABLE_FLAG
There was an extraneous tx being added to the mempool which spent the same inputs. This only didn't cause a problem because locktimes stopped it from being selected for the template.
In addition, since the merger of MedianTimePast, locked txs that were time based were not being correctly tested in the unit test.
Any tx that has been in your mempool for longer than 10 blocks will have to be recalculated from scratch during a reorg. In addition any transaction which entered your mempool more recently than the reorg depth will have to recalculated, so you will always have to recalculate at least the txs that came in since the last blcok.
I think if we are going to try to make reorgs fast, we should do it properly.
We could potentially scan the whole header chain instead of only 10, but I don’t think this is a good idea performance wise. I think this is better to only scan 10 and just recalculate locktime if it is more, rather than wasting time scanning to the genesis. I can change that though if you think it is indeed faster to scan to genesis. MempoolEntry older than 10 blocks are rather rare I would expect.
In addition any transaction which entered your mempool more recently than the reorg depth will have to recalculated, so you will always have to recalculate at least the txs that came in since the last blcok.
I don’t understand your point. Imagine tip = Block A, the tx1 comes in mempool. Then a reorg happens that replace BlockA with BlockB then tx1 will have, as expected, locktime invalidated. This must be done, and can’t be optimized, because the blockTime in LockTime(), MTP or not, will have changed.
If I understand your point : yes, during a reorg, all transaction which came since the last block will be invalidated, and they should be.
Technically if its a 1 block reorg, the blockTime won’t have changed (with MTP)
The blockTime will have changed. The old tip (BlockA) was part of the median. When calculating LockTime of a mempool entry, we are assuming the tx will get in next block just after tip, so BlockA or BlockB are part of MTP calculation.
I’m just saying you don’t have to recalculate the time/height at which the sequence locks are valid which is the expensive part.
I need to. After a reorg, the “birthdate” of the inputs may have changed, and so, the minimum time/height (what you call LockPoint) to satisfy as well. Am I missing something ?
The blockTime will have changed. The old tip (BlockA) was part of the median.
Yes sorry, you are right about that, anyway we both agreed it should be checked.
I need to. After a reorg, the “birthdate” of the inputs may have changed, and so, the minimum time/height (what you call LockPoint) to satisfy as well. Am I missing something ?
Yes you need to in your code. I’m saying my approach saves that.
I agree, with jtimon. What is wrong with this code is the I recheck more than I need to. In this code, if there is a 1 block reorg (block A replaced by block B), then all mempool transaction which came after BlockA will need to be rechecked. Morcos solution does not have this problem.
I myself prefer not touching the code that was heavily reviewed though.
Labels
Mempool