Keep reorgs fast for SequenceLocks checks #7187

morcos · 2015-12-08T19:45:07Z

Builds on #7184

Adds a commit to keep the mempool consistent with respect to sequence locked txs in the event of a reorg

jtimon · 2015-12-08T21:43:24Z

Since #6312 has gne through a lot of review already, I would prefer that this branch builds on top of that to more easily review the differences. We can always squash things before merging for a cleaner history.

morcos · 2015-12-08T21:55:09Z

~~@jtimon That was the first thing I created: [email protected]:refactorb~~
(misunderstanding of what he was asking)

But I think #7184 is a cleaner implementation of BIP 68 on its own.

jtimon · 2015-12-08T22:05:48Z

as said squashes to clean up the history can happen after review.
As mentiooned before, I still don't see how [email protected] achieves its alleged goal.

morcos · 2015-12-09T02:52:55Z

rebased

NicolasDorier · 2015-12-09T03:54:32Z

I think this PR is too complicated for fixing the perf issue, if NicolasDorier/bitcoin@67646e0...b2a27a7 is correct, it would be simpler to review. Regardless if we use #7184 or not.

jtimon · 2015-12-09T05:28:22Z

@NicolasDorier I agree. Maybe more importantly, all the extra complexity is outside of the consensus critical code. Maybe you should consider opening a PR for your solution?
In this thread we should focus on the code of this PR. We have #7176 to discuss the different solutions more generally.

jtimon · 2015-12-09T06:23:24Z

Is this supposed to fix the performance issues of CNB without touching miner.cpp, or are this just preparations without the actual performance solution?

morcos · 2015-12-09T11:19:22Z

@jtimon This fixes the performance issues by never introducing them into CNB in the first place. #7184 on its own has performance issues with reorgs.

@NicolasDorier You think just the second commit here is significantly more complicated than your final commit in #7190?

morcos · 2015-12-09T12:10:49Z

squashed a bug fix (wasn't actually skipping the work it was supposed to)

NicolasDorier · 2015-12-09T15:09:20Z

I like the fact you separate CheckLockTime (if you rename IsFinalTx) and CheckSequenceLockTime, but I really don't like the LockPoints structure. I'm still reviewing if commit 2 might not result in mempool finality corruption, I really don't find it obvious.

NicolasDorier · 2015-12-09T15:18:47Z

btw can you rename IsFinalTx to CheckLockTime as you said you wanted to do ?

morcos · 2015-12-09T16:14:02Z

@NicolasDorier My approach and yours are not that different.

You are storing the hash at which the calculation of the sequence lock time and height happened and using the fact that if that hash is still on the chain that the calculation must still be valid and the comparison to the current time and height must be valid because they can not have gone below what they were at the hash you saved.
I'm storing the lowest height hash at which the calculation of the sequence lock time and height would necessarily still be valid (which is the hash of the highest height input with a sequence lock, b/c if none of the inputs changed height then the calculation is trivially the same) and the result of the calculation. But in the case of my code, I have to redo the comparison part because the current time and height maybe be lower than they were at the time I originally did the comparison. So I will redo comparisons for every tx on every reorg, but that is extremely fast, and I save redoing the expensive calculations for many more txs.

NicolasDorier · 2015-12-09T16:54:20Z

Ok I'm getting it. Seems like it can work.
You forget to check SequenceLockTime in rpcwallet.cpp, wallet.cpp.

morcos · 2015-12-09T18:19:37Z

@NicolasDorier I left that out on purpose. See #7184. It might need to be added back in but it wasn't really clear to me how that code ever gets hit. I've tried asking about it on IRC a couple of times. But it doesn't seem possible to me to generate a wallet tx which is sequence locked except in the event of a reorg, at which point the tx won't be in your mempool anyway.

NicolasDorier · 2015-12-10T06:23:15Z

Ok, I don't really understand those implication yet.

NicolasDorier · 2015-12-10T06:24:48Z

Will you rename to IsFinalTx to CheckLockTime and CheckSequenceLocks to CheckSequenceLockTime ?

morcos · 2016-01-14T22:35:14Z

Updated for the updated #7184 and took @sdaftuar's suggestion

NicolasDorier · 2016-02-18T03:41:16Z

you may optimize even more: Only take the heights which correspond to an input who has a LockSequence for maxInputHeight calculation.

morcos · 2016-02-18T18:28:03Z

@NicolasDorier that's already done. CalculateSequenceLocks already sets heights for non sequence locked inputs to 0.

sdaftuar · 2016-02-26T18:02:44Z

Code review ACK (apart from comment nit). Testing...

sdaftuar · 2016-02-26T18:44:27Z

Tested ACK (hacked up some extra tests to bip68-sequence.py to exercise the reorg logic) 5912944f4d5eeb4078a5bb658ad09ef7eabbf53b

btcdrak · 2016-02-26T19:12:18Z

@sdaftuar Would you mind publishing/PRing those extra tests? Maybe they could be added to this PR?

morcos · 2016-02-29T21:30:36Z

comment nit addressed

sipa · 2016-03-05T05:24:29Z

Kicked Travis

sipa · 2016-03-05T22:51:46Z

src/txmempool.h

I believe it suffices to turn this into a const reference.

odinTy · 2016-03-05T23:31:09Z

If I've done something wrong it was by mistake I'm new and trying to learn. Sorry if I messed up

odinTy · 2016-03-05T23:33:54Z

Please explain all my mistakes please as this should help the future

btcdrak · 2016-03-15T11:09:50Z

utACK ddb4dab

laanwj · 2016-03-16T11:30:40Z

If I've done something wrong it was by mistake I'm new and trying to learn. Sorry if I messed up

What happened exactly? If you have specific questions about development and review process feel free to mail me on [email protected]

laanwj · 2016-03-16T15:58:14Z

ut/code review ACK ddb4dab

laanwj · 2016-03-16T16:04:25Z

src/txmempool.h

Did you consider storing a CBlockIndex* here instead of a uint256? (may be a stupid suggestion, but from what I see the only thing you do is look it back up in mapBlockIndex)

hmm... i feel like we did consider that, but i can't see now why that wouldn't work. maybe it didn't work with some previous iteration. it makes sense to switch to that if we can't see any problems with it.

right, I don't see any problems with it at least: as I see it it'd simplify the code, reduce storage requirement for LockPoints, and makes the same (valid) assumption that a block index entry never goes away

jtimon · 2016-03-16T16:12:35Z

Concept ACK, started reviewing more deeply.

morcos · 2016-03-16T16:39:50Z

@laanwj ok done. If you think this has sufficient review I can squash.

Obtain LockPoints to store in CTxMemPoolEntry and during a reorg, evaluate whether they are still valid and if not, recalculate them.

morcos · 2016-03-16T20:16:05Z

Squashed from e6cc06b which contained the switch to CBlockIndex*'s

laanwj · 2016-03-16T20:19:58Z

utACK 982670c

982670c Add LockPoints (Alex Morcos)

btcdrak · 2016-03-16T20:56:09Z

@laanwj this PR has been backported in 7543.

maflcko · 2016-04-25T13:35:10Z

Removed 'needs backport' label per previous comment

morcos closed this Dec 8, 2015

morcos reopened this Dec 8, 2015

morcos mentioned this pull request Dec 8, 2015

Performance fix propositions for CNB in case of #6312 merging #7176

Closed

morcos force-pushed the fastReorgBIP68 branch from 9da86bd to 1d1a35f Compare December 9, 2015 02:48

jonasschnelli added Feature Mempool labels Dec 9, 2015

morcos force-pushed the fastReorgBIP68 branch from 1d1a35f to 5350fdf Compare December 9, 2015 12:10

morcos mentioned this pull request Dec 9, 2015

Performance fix for #6312 #7190

Closed

btcdrak mentioned this pull request Dec 10, 2015

Implement SequenceLocks functions for BIP 68 #7184

Merged

jtimon mentioned this pull request Dec 20, 2015

BIP-68: Mempool-only sequence number constraint verification #6312

Closed

morcos force-pushed the fastReorgBIP68 branch from 5350fdf to 9d48b2e Compare January 14, 2016 22:34

morcos force-pushed the fastReorgBIP68 branch from 9d48b2e to f8b6614 Compare January 15, 2016 15:18

ajtowns mentioned this pull request Jan 28, 2016

BIP-112: Mempool-only CHECKSEQUENCEVERIFY #6564

Closed

laanwj added the Needs backport label Feb 18, 2016

sipa reviewed Mar 5, 2016
View reviewed changes

src/txmempool.h Outdated

Copy link

Member

sipa Mar 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it suffices to turn this into a const reference.

laanwj reviewed Mar 16, 2016
View reviewed changes

Add LockPoints

982670c

Obtain LockPoints to store in CTxMemPoolEntry and during a reorg, evaluate whether they are still valid and if not, recalculate them.

morcos force-pushed the fastReorgBIP68 branch from e6cc06b to 982670c Compare March 16, 2016 20:15

laanwj merged commit 982670c into bitcoin:master Mar 16, 2016

laanwj added a commit that referenced this pull request Mar 16, 2016

Merge #7187: Keep reorgs fast for SequenceLocks checks

14d6324

982670c Add LockPoints (Alex Morcos)

btcdrak mentioned this pull request Mar 16, 2016

[0.12] Backport BIP9, BIP68 and BIP112 with softfork #7543

Merged

maflcko removed the Needs backport label Apr 25, 2016

furszy mentioned this pull request Aug 7, 2020

Up to CTransactionRef point back ports to do list. PIVX-Project/PIVX#1726

Closed

21 tasks

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Keep reorgs fast for SequenceLocks checks #7187

Keep reorgs fast for SequenceLocks checks #7187

Uh oh!

Conversation

morcos commented Dec 8, 2015

Uh oh!

jtimon commented Dec 8, 2015

Uh oh!

morcos commented Dec 8, 2015

Uh oh!

jtimon commented Dec 8, 2015

Uh oh!

morcos commented Dec 9, 2015

Uh oh!

NicolasDorier commented Dec 9, 2015

Uh oh!

jtimon commented Dec 9, 2015

Uh oh!

jtimon commented Dec 9, 2015

Uh oh!

morcos commented Dec 9, 2015

Uh oh!

morcos commented Dec 9, 2015

Uh oh!

NicolasDorier commented Dec 9, 2015

Uh oh!

NicolasDorier commented Dec 9, 2015

Uh oh!

morcos commented Dec 9, 2015

Uh oh!

NicolasDorier commented Dec 9, 2015

Uh oh!

morcos commented Dec 9, 2015

Uh oh!

NicolasDorier commented Dec 10, 2015

Uh oh!

NicolasDorier commented Dec 10, 2015

Uh oh!

morcos commented Jan 14, 2016

Uh oh!

NicolasDorier commented Feb 18, 2016

Uh oh!

morcos commented Feb 18, 2016

Uh oh!

sdaftuar commented Feb 26, 2016

Uh oh!

sdaftuar commented Feb 26, 2016

Uh oh!

btcdrak commented Feb 26, 2016

Uh oh!

morcos commented Feb 29, 2016

Uh oh!

sipa commented Mar 5, 2016

Uh oh!

sipa Mar 5, 2016

Choose a reason for hiding this comment

Uh oh!

odinTy commented Mar 5, 2016

Uh oh!

odinTy commented Mar 5, 2016

Uh oh!

btcdrak commented Mar 15, 2016

Uh oh!

laanwj commented Mar 16, 2016

Uh oh!

laanwj commented Mar 16, 2016

Uh oh!

laanwj Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

morcos Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

jtimon commented Mar 16, 2016

Uh oh!

morcos commented Mar 16, 2016

Uh oh!