Implement SequenceLocks functions for BIP 68 #7184

morcos · 2015-12-07T21:00:09Z

SequenceLocks functions are used to evaluate sequence lock times or heights per BIP 68.

This is an alternate to #6312

~~This PR still needs to update unit tests.~~
In addition it no longer reflects sequence locked information in wallet txs. This can be added in later if desired, but I don't think its important now.

This code borrows heavily from #6312 and the work of @maaku, @btcdrak, @NicolasDorier, and @sipa

(EDIT) Important Note: This PR has changed the semantics of BIP 68 to always use MTP for comparison regardless of BIP 113. I believe this makes more sense. BIP 113 is still needed to change the semantics of nLockTime however.

NicolasDorier · 2015-12-08T03:16:12Z

wow I like that, the changes are very small to review. I'll add the tests later tonight or tomorrow..

morcos · 2015-12-08T03:25:48Z

@NicolasDorier oh good, i was mostly holding off on the tests because i wanted to see if people liked this approach.

What do you mean about IsFinalTx? I think the changes to make that aware of MTP are already merged, so it wasn't necessary for me to change it further.

instagibbs · 2015-12-08T15:07:32Z

src/main.h

@@ -341,7  341,22 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime);
 */
 bool CheckFinalTx(const CTransaction &tx, int flags = -1);


Like the symmetry, but the name here is now a bit out-dated and possibly misleading since we have two types of finality.

Agreed. I was going for minimal changes to existing consensus code, but if people like I'd be happy to rename that. Perhaps LocktimeLock() and CheckLocktimeLock()

I'd also like to take out the way CheckFinalTx is called with default -1 in the wallet code, but I could save that for another pull.

IsFinalTx is planned to be part of libconsensus, CheckFinalTx is just a convenience function using globals. I don't consider CheckFinalTx (nor CheckSequenceLocks) a "consensus function" or "consensus code".

NicolasDorier · 2015-12-08T15:56:01Z

I am trying another approach before building on yours, which would solve the removeForReorg and CNB problem without changing any code in #6312. (NicolasDorier/bitcoin@b13f475...0fb1092 now building)

I removed my comment about IsFinalTx, I was surprised not seeing the flag passed to it. But it seems client are held responsible for calculating the right blocktime. This led to code duplication, #6312 fixed that.

I like the idea of having 2 methods : CheckLockTime and CheckSequenceLockTime though, but I wonder if it will really be useful since there will be no case where we want to call one and not the other.

morcos · 2015-12-08T16:01:29Z

added unit tests

sdaftuar · 2015-12-08T16:05:12Z

@NicolasDorier One reason I like the approach in this PR is that the sequence-lock check is done separately from the nLockTime check -- locktime can be evaluated for all transactions in a block at the time a block is received (eg in ContextualCheckBlock), whereas sequence locks can only be evaluated when all inputs heights' are available (eg during ConnectBlock).

From a design perspective, it seems to me that having the consensus checks operate in the smallest logical units ought to limit the need for future refactorings (which I think is a good design goal for consensus code).

NicolasDorier · 2015-12-08T16:07:58Z

Ok, it makes sense. I'm kind of worried though about having the block of CheckLockTime responsible for calculating the blocktime each time, this is code duplication easy to get wrong. (I think that when I fixed it in #6312, there was indeed some parts which could break if the block evaluated was the genesis with MTP enabled)

What about having a CCBlockIndex::GetCutoffTime(flag) method ?

NicolasDorier · 2015-12-08T16:23:50Z

It does not solve the perf problem though, CheckSequenceLock need to be called during reorg. (Did NicolasDorier/bitcoin@9e8c7be...8670ce8 it takes time for me to compile, I don't have a linux at hand, doing it by pushing/waiting travis)

morcos · 2015-12-08T20:40:39Z

Here is another version of this that assumes BIP68 includes MTP already: morcos@ba957d5

btcdrak · 2015-12-08T21:17:59Z

@morcos Since we plan to roll out BIP113 and BIP68 together it really makes sense that BIP68 assumes MTP. I think that is a must and it simplifies things.

jtimon · 2015-12-08T21:51:50Z

NACK This wastes a lot of previous review effort by not building on top of #6312.

morcos · 2015-12-09T02:51:21Z

OK I fixed my comment about -1 and switched to the version where BIP 68 is defined to use MTP.

If it is helpful for anyone to see what this PR looks like as a change from #6312, you can see that here:
maaku/bitcoin@sequencenumbers...morcos:7184onorig6312

jtimon · 2015-12-09T06:19:01Z

If it is helpful for anyone to see what this PR looks like as a change from #6312, you can see that here: [email protected]:7184onorig6312

Thank you, it is certainly useful, at least for me.
Why not use that branch here directly (like @sdaftuar and @NicolasDorier are doing with their solutions) instead? Later maybe you can squash some of the commits if this replaces #6312 .
I don't understand why you insist in doing it in a new single commit with your name in it instead.

sdaftuar · 2015-12-10T14:47:00Z

src/primitives/transaction.h

@@ -61,13  61,39 @@ class CTxIn
    CScript scriptSig;
    uint32_t nSequence;

+    /* Setting nSequence to this value for every input in a transaction
+     * disables nLockTime. */
+    static const uint32_t SEQUENCE_FINAL = 0xffffffff;


The comments here are slightly confusing to me because of missing context. The above is a consensus rule for all transactions, while the ones below only apply as policy (for now) for tx.nVersion >= 2 transactions. I think we should expand on the comments to make this clearer, perhaps with a reference to BIP68?

(Edit: I think this also applies to #6312.)

It's not clear to me that this is is going to supersede #6312 . It seems there's at least 2 more people with alternatives. Can we maintain #6312 updated as a common base for the 3 different options?
As said, squashes can happe right before merging (you can also reset HEAD^ the squashed commit and redo it with your name if that's the reason why this currently doesn't use the common base [no other reason comes to mind]).

This comment has nothing to do with BIP 68. Setting all nSequence to max into disables nLockTime for all tx versions, today.

rustyrussell · 2016-01-07T04:45:59Z

Tested-by: Rusty Russell [email protected] (merged with #6564 for testing, with only trivial header conflict fixup).

morcos · 2016-01-14T22:19:58Z

@sdaftuar's nits and suggestions addressed.

After this receives some ACK's I can work on back ports

NicolasDorier · 2016-01-16T10:11:59Z

I think a new PR with #7187 on top of this one should be open. It makes no sense to review only this PR without #7187.

jtimon · 2016-01-16T10:20:29Z

Agree with @NicolasDorier. If it wasn't for #7187 there would be no reason to replace #6312 in the first place.

btcdrak · 2016-01-28T17:15:07Z

Test script is available here (requires merge of #6564 with #7184) https://github.com/ajtowns/op_csv-test

btcdrak · 2016-02-10T17:16:19Z

Nice catch @sipa.

@sipa is a rockstar! 🎸

sipa · 2016-02-10T18:36:17Z

test/miner_tests.cpp:82

unknown location(0): fatal error: in "miner_tests/CreateNewBlock_validity": signal: SIGABRT (application abort requested)

sipa · 2016-02-10T20:27:40Z

utACK 4315fe1afce448698732cf1bcffeb070028e4b2b after squashing

Or, utACK tree id 04ef32caaaf2b838295f39cffbdd87468f3657c9

$ git show -s --format="%T" 4315fe1afce448698732cf1bcffeb070028e4b2b
04ef32caaaf2b838295f39cffbdd87468f3657c9

A squash should retain the tree id.

SequenceLocks functions are used to evaluate sequence lock times or heights per BIP 68. The majority of this code is copied from maaku in bitcoin#6312 Further credit: btcdrak, sipa, NicolasDorier

morcos · 2016-02-10T20:42:00Z

Ok squashed, same code

git show -s --format="%T" da6ad5f684b91975cae3f37495ccbd041499e86b
04ef32caaaf2b838295f39cffbdd87468f3657c9

morcos · 2016-02-10T21:03:34Z

sigh.. small rarely occurring bug in the RPC test
@laanwj when you are ready to merge, i'll squash again, or feel free to just do it yourself

maaku · 2016-02-10T21:32:37Z

Meta-point, it would be better process to not squash these PRs...

sipa · 2016-02-10T21:42:23Z

@maaku My view is that the commit history of what is merged should optimize for reviewability by someone who has not seen the pull requests. So multiple commits that implement separate parts of a feature, make sense on their own, and result in a working codebase on their own are certainly welcome, but I don't think we should aim for having the exact history of a pull request, and certainly not when some commits are broken and need follow-up commits for fixing.

laanwj · 2016-02-11T09:28:35Z

@laanwj when you are ready to merge, i'll squash again, or feel free to just do it yourself

Okay, thanks.

Meta-point, it would be better process to not squash these PRs...

I don't agree on that. One should try to order commits so that it does not break atomicity, or create intermediate pulls that fail buld/test, as that messes with git bisect.
Also @sipa makes a good point that commit history should be optimized for reviewing, not what happened to be the chronological order of changes at the time. This may involve multiple commits if that makes reviewing easier (due to separate concerns, for example, or moves changes).

Putting the commit hash in comments is a help to compare the version that was reviewed to the version that was merged - it is not meant to reduce flexibility of the developer with regard to ordering commits.

Process discussion is completely off-topic here though, so let's leave it at that.

sdaftuar · 2016-02-11T16:25:02Z

src/main.cpp

+    index.pprev = tip;
+    // CheckSequenceLocks() uses chainActive.Height() 1 to evaluate
+    // height based locks because when SequenceLocks() is called within
+    // CBlock::AcceptBlock(), the height of the block *being*


nit: CBlock::AcceptBlock() isn't a thing, should say ConnectBlock I think.

sipa · 2016-02-11T19:19:43Z

Mental note: verify how this interacts with the wallet

it boggles the mind why these nits can't be delivered on a more timely basis

sdaftuar · 2016-02-11T20:41:41Z

ACK b043c4b

b043c4b fix sdaftuar's nits again (Alex Morcos) a51c79b Bug fix to RPC test (Alex Morcos) da6ad5f Add RPC test exercising BIP68 (mempool only) (Suhas Daftuar) c6c2f0f Implement SequenceLocks functions (Alex Morcos)

laanwj · 2016-02-12T16:07:58Z

utACK b043c4b

morcos mentioned this pull request Dec 7, 2015

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

Closed

instagibbs reviewed Dec 8, 2015
View reviewed changes

morcos force-pushed the alternate68 branch from d08dfce to 5635e5b Compare December 8, 2015 16:00

morcos mentioned this pull request Dec 8, 2015

Keep reorgs fast for SequenceLocks checks #7187

Merged

morcos force-pushed the alternate68 branch from 5635e5b to 50ce92f Compare December 9, 2015 02:48

morcos changed the title ~~[WIP] Implement SequenceLocks functions for BIP 68~~ Implement SequenceLocks functions for BIP 68 Dec 9, 2015

jonasschnelli added Feature Mempool labels Dec 9, 2015

sdaftuar reviewed Dec 10, 2015
View reviewed changes

NicolasDorier mentioned this pull request Dec 14, 2015

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

Closed

morcos force-pushed the alternate68 branch from 1f1df01 to e87d06a Compare January 14, 2016 22:18

ajtowns mentioned this pull request Jan 28, 2016

BIP-112: Mempool-only CHECKSEQUENCEVERIFY #6564

Closed

morcos and others added 2 commits February 10, 2016 15:35

Implement SequenceLocks functions

c6c2f0f

SequenceLocks functions are used to evaluate sequence lock times or heights per BIP 68. The majority of this code is copied from maaku in bitcoin#6312 Further credit: btcdrak, sipa, NicolasDorier

Add RPC test exercising BIP68 (mempool only)

da6ad5f

morcos force-pushed the alternate68 branch from 4315fe1 to da6ad5f Compare February 10, 2016 20:36

Bug fix to RPC test

a51c79b

sdaftuar reviewed Feb 11, 2016
View reviewed changes

fix sdaftuar's nits again

b043c4b

it boggles the mind why these nits can't be delivered on a more timely basis

btcdrak mentioned this pull request Feb 12, 2016

Update BIP68 implementation to match spec bitcoin/bips#330

Merged

laanwj merged commit b043c4b into bitcoin:master Feb 12, 2016

btcdrak mentioned this pull request Feb 12, 2016

BIP-112: Mempool-only CHECKSEQUENCEVERIFY #7524

Merged

jtimon mentioned this pull request Feb 14, 2016

MOVEONLY: Move consensus functions out of main #7310

Closed

btcdrak mentioned this pull request Feb 16, 2016

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

Merged

afk11 mentioned this pull request Mar 18, 2016

BIP9 versionbits softfork for BIP68, BIP112 and BIP113 #7648

Merged

NicolasDorier mentioned this pull request Mar 20, 2016

Test of BIP9 fork activation of mtp, csv, sequence_lock btcdrak/bitcoin#8

Closed

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

		@@ -341,7 341,22 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime);
		*/
		bool CheckFinalTx(const CTransaction &tx, int flags = -1);

Implement SequenceLocks functions for BIP 68 #7184

Implement SequenceLocks functions for BIP 68 #7184

Conversation

morcos commented Dec 7, 2015

NicolasDorier commented Dec 8, 2015

morcos commented Dec 8, 2015

instagibbs Dec 8, 2015

Choose a reason for hiding this comment

morcos Dec 8, 2015

Choose a reason for hiding this comment

jtimon Jan 16, 2016

Choose a reason for hiding this comment

NicolasDorier commented Dec 8, 2015

morcos commented Dec 8, 2015

sdaftuar commented Dec 8, 2015

NicolasDorier commented Dec 8, 2015

NicolasDorier commented Dec 8, 2015

morcos commented Dec 8, 2015

btcdrak commented Dec 8, 2015

jtimon commented Dec 8, 2015

morcos commented Dec 9, 2015

jtimon commented Dec 9, 2015

sdaftuar Dec 10, 2015

Choose a reason for hiding this comment

jtimon Dec 10, 2015

Choose a reason for hiding this comment

maaku Dec 10, 2015

Choose a reason for hiding this comment

rustyrussell commented Jan 7, 2016

morcos commented Jan 14, 2016

NicolasDorier commented Jan 16, 2016

jtimon commented Jan 16, 2016

btcdrak commented Jan 28, 2016

btcdrak commented Feb 10, 2016

sipa commented Feb 10, 2016

sipa commented Feb 10, 2016

morcos commented Feb 10, 2016

morcos commented Feb 10, 2016

maaku commented Feb 10, 2016

sipa commented Feb 10, 2016

laanwj commented Feb 11, 2016

sdaftuar Feb 11, 2016

Choose a reason for hiding this comment

sipa commented Feb 11, 2016

sdaftuar commented Feb 11, 2016

laanwj commented Feb 12, 2016