Skip to content

Performance fix for #6312 #7190

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.

Sign up for GitHub

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

Jump to bottom
Closed
NicolasDorier wants to merge 13 commits into from
Closed

Performance fix for #6312 #7190

NicolasDorier wants to merge 13 commits into from

Conversation

@NicolasDorier
Copy link
Contributor

@NicolasDorier NicolasDorier commented Dec 9, 2015

Build on #6312 commit added is 0559bb24e6e92a2d790c77707e464aa1e17919ca :

  • Remove lock time verification in CNB because the mempool is always finality coherent.
  • Improve removeForReorg by not calling CheckLockTime on every transaction in the mempool during a reorg.

maaku and others added 12 commits December 2, 2015 21:52
@maaku @NicolasDorier
Reverse structure of IsFinalTx()
3039295 
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.
@maaku @NicolasDorier
Get rid of CTxIn::IsFinal(), which is only used in one location and w…
998230b 
…hose semantics will not survive revival of sequence numbers, and introduce CTxIn::SEQUENCE_FINAL constant instead.
@maaku @NicolasDorier
Expand IsFinalTx() to the structure it will have when it becomes Lock…
bf45b9f 
…Time()

Breaks up the modifications to IsFinalTx() / LockTime() for easier review. This commit does not change the logic of IsFinalTx().
@maaku @NicolasDorier
Add rules--presently disabled--for using sequence numbers as a relati…
deedf00 
…ve lock time

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.
@maaku @NicolasDorier
Enable policy enforcing sequence numbers as a relative lock time in t…
8110e73 
…he mempool and transaction selection code, for the purpose of later supporting consensus-enforced transaction replacement

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.
@btcdrak @NicolasDorier
Add AssertLockHeld(cs_main) to LockTime() function.
33ec593
@NicolasDorier
Transform LockTime to a pure logic function, while not breaking exist…
fc4bfb4 
…ing code
@NicolasDorier
Make LockTime function easier to use by not requiring the tip, and by…
aa83819 
… making it responsible to calculate the right blockTime from flags

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
@sipa @NicolasDorier
Make LockTime use a height map of inputs
6bf228e
@btcdrak @NicolasDorier
Rename flags
bd5a760 
_SECONDS_FLAG to _TYPE_FLAG
_DISABLED_FLAG to _DISABLE_FLAG
@btcdrak
Explain the reason for unsigned cast of tx.nVersion
ae5d345
@morcos @btcdrak
Fix minor bugs in miner_tests
4a01034 
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.
@NicolasDorier NicolasDorier mentioned this pull request Dec 9, 2015
Closed
@morcos
Copy link
Contributor

morcos commented Dec 9, 2015

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.

@morcos morcos mentioned this pull request Dec 9, 2015
Merged
@NicolasDorier
Do not recalculate LockTime for a mempool entry in removeForReorg if …
2615613 
…the tip at the time of calculation is still in the main chain
@NicolasDorier
Copy link
Contributor Author

NicolasDorier commented Dec 9, 2015

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.

@morcos
Copy link
Contributor

morcos commented Dec 9, 2015

Yes that is my point.
However with the solution in #7187 it will not be necessary to relook up the inputs to the the tx and recalculate the sequence locks unless any of the inputs to the tx were in that most recent block, which seems likely to be considerably rarer.
Technically if its a 1 block reorg, the blockTime won't have changed (with MTP) but I'm not suggesting not rechecking that. 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.
With your solution I think you will still be recalculating sequence locks for a significant fraction of the mempool on a reorg which is slow and destroys your utxo cache.

@NicolasDorier
Copy link
Contributor Author

NicolasDorier commented Dec 9, 2015

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 ?

@morcos
Copy link
Contributor

morcos commented Dec 9, 2015

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.

@jtimon
Copy link
Contributor

jtimon commented Dec 11, 2015

I'm having troubles understanding what is wrong with this solution for #7176. It is very nice that it doesn't have to touch the 6-months old (and already reviewed) consensus code (#6312) further, and I think that makes it the less risky and less development-resources-wasteful option for #7176.

@NicolasDorier
Copy link
Contributor Author

NicolasDorier commented Dec 12, 2015

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.

@laanwj laanwj added the Mempool label Dec 14, 2015
@btcdrak
Copy link
Contributor

btcdrak commented Jan 20, 2016

@NicolasDorier this should be closed as the related ticket was also closed and superseded.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Mempool

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@NicolasDorier @morcos @jtimon @btcdrak @laanwj @maaku @sipa