Implement SequenceLocks functions for BIP 68

morcos commented at 9:00 pm on December 7, 2015: member

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 commented at 3:16 am on December 8, 2015: contributor

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

morcos commented at 3:25 am on December 8, 2015: member

@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.

in src/main.h: in d08dfced59 outdated

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

instagibbs commented at 3:07 pm on December 8, 2015:

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

morcos commented at 3:25 pm on December 8, 2015:

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.

jtimon commented at 9:44 am on January 16, 2016:

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 commented at 3:56 pm on December 8, 2015: contributor

I am trying another approach before building on yours, which would solve the removeForReorg and CNB problem without changing any code in #6312. (https://github.com/NicolasDorier/bitcoin/compare/b13f47535cc2...0fb1092ed53f 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 force-pushed on Dec 8, 2015

morcos commented at 4:01 pm on December 8, 2015: member

added unit tests

sdaftuar commented at 4:05 pm on December 8, 2015: member

@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 commented at 4:07 pm on December 8, 2015: contributor

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 commented at 4:23 pm on December 8, 2015: contributor

It does not solve the perf problem though, CheckSequenceLock need to be called during reorg. (Did https://github.com/NicolasDorier/bitcoin/compare/9e8c7be9bc94...8670ce84fd01 it takes time for me to compile, I don’t have a linux at hand, doing it by pushing/waiting travis)

morcos commented at 8:40 pm on December 8, 2015: member

Here is another version of this that assumes BIP68 includes MTP already: https://github.com/morcos/bitcoin/commit/ba957d5407ecc3d1f320893f47cfc7181a0a0878

btcdrak commented at 9:17 pm on December 8, 2015: contributor

@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 commented at 9:51 pm on December 8, 2015: contributor

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

morcos force-pushed on Dec 9, 2015

morcos commented at 2:51 am on December 9, 2015: member

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: https://github.com/maaku/bitcoin/compare/sequencenumbers...morcos:7184onorig6312

morcos renamed this:
~~[WIP] Implement SequenceLocks functions for BIP 68~~
Implement SequenceLocks functions for BIP 68
on Dec 9, 2015

jtimon commented at 6:19 am on December 9, 2015: contributor

If it is helpful for anyone to see what this PR looks like as a change from #6312, you can see that here: maaku@sequencenumbers…morcos: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.

jonasschnelli added the label Feature on Dec 9, 2015

jonasschnelli added the label Mempool on Dec 9, 2015

in src/primitives/transaction.h: in 50ce92f985 outdated

60@@ -61,13 +61,39 @@ class CTxIn
61     CScript scriptSig;
62     uint32_t nSequence;
63 
64+    /* Setting nSequence to this value for every input in a transaction
65+     * disables nLockTime. */
66+    static const uint32_t SEQUENCE_FINAL = 0xffffffff;

sdaftuar commented at 2:47 pm on December 10, 2015:

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.)

jtimon commented at 7:41 pm on December 10, 2015:

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]).

maaku commented at 8:57 pm on December 10, 2015:

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

in src/main.cpp: in 50ce92f985 outdated

764+    return std::make_pair(nMinHeight, nMinTime);
765+}
766+
767+static bool EvaluateSequenceLocks(const CBlockIndex& block, std::pair<int, int64_t> lockPair)
768+{
769+    int64_t nBlockTime =  block.GetAncestor(std::max(block.nHeight-1, 0))->GetMedianTimePast();

sdaftuar commented at 4:25 pm on December 10, 2015:

I think using block.nHeight-1 here forces an unnecessary restriction on the caller (ie that block.nHeight is being set to 1 more than the current tip height) – if we change this to be

0int64_t nBlockTime = block.pprev ? block.pprev->GetMedianTimePast() : 0;

Then we would be able to change the CheckSequenceLocks code in the future to allow us to test whether a transaction is able to be confirmed in the next N blocks for N > 1 (right now we can only do N=1).

This would allow for mempool policy code that could permit transactions that may not be mineable now, but will be mineable in the near future.

NicolasDorier commented at 5:05 pm on December 10, 2015:

The semantic used by caller is the following : Check SequenceLock assuming the transaction is included in “block”

This is needed. Because block might be an existing old block but also a block on top of tip. (for mempool transactions) Changing the semantic of this method would make things less clear.

I introduced this change on https://github.com/maaku/bitcoin/commit/aa83819cac79edc3ab9c140cbbd2cd1417e04a8a . Previously, the LockTime method needed both : the tip and the block where it was tested against. I simplified by making it requires only the block it is tested against.

There is no concept of “tip” to have in the definition of sequence lock.

sdaftuar commented at 5:31 pm on December 10, 2015:

@NicolasDorier My point is that we’re requiring nHeight-1 to be a valid blockindex entry, when really all we need to do is rely on pprev being a valid block index (modulo checks for the genesis block).

If we use pprev for the time calculation rather than nHeight, that would give us the freedom to tweak the value of nHeight that we pass in, so that we could test for transactions with height-based-relative-locks being confirmable in N blocks rather than just in the next block.

NicolasDorier commented at 3:10 am on December 11, 2015:

woops, sorry I misunderstood your point. (I thought you wanted to pass the previous block directly) Correct.

rustyrussell commented at 4:45 am on January 7, 2016: contributor

Tested-by: Rusty Russell rusty@rustcorp.com.au (merged with #6564 for testing, with only trivial header conflict fixup).

morcos force-pushed on Jan 14, 2016

morcos commented at 10:19 pm on January 14, 2016: member

@sdaftuar’s nits and suggestions addressed.

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

NicolasDorier commented at 10:11 am on January 16, 2016: contributor

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 commented at 10:20 am on January 16, 2016: contributor

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

btcdrak commented at 5:15 pm on January 28, 2016: contributor

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

in src/main.cpp: in e87d06a20e outdated

770+    return std::make_pair(nMinHeight, nMinTime);
771+}
772+
773+static bool EvaluateSequenceLocks(const CBlockIndex& block, std::pair<int, int64_t> lockPair)
774+{
775+    int64_t nBlockTime = block.pprev ? block.pprev->GetMedianTimePast() : 0;

petertodd commented at 4:23 am on January 31, 2016:

Would you mind adding a comment explaining when block.pprev might be null; if I understand it correctly the only case where that is true is the genesis block, in which case it’d be good to add an assertion that the block height matches that case.

morcos commented at 9:22 pm on February 1, 2016:

I’m not sure why we think its ever necessary to check sequence locks on a genesis block. So what would you think about assert(block.pprev) and then removing the ternary operator?

btcdrak commented at 5:23 am on February 2, 2016:

@morcos No, this is the reason for the ternary #6312 (review)

morcos commented at 3:14 pm on February 2, 2016:

@btcdrak That was the old code. The new code calls SequenceLocks from ConnectBlock which doesn’t run on the genesis block and doesn’t call SequenceLocks on coinbase txs. If both of those things change, it would get caught by the proposed assert.

jtimon commented at 11:18 am on February 3, 2016:

Ack on assert(block.pprev): we should never validate the genesis block anyway (it’s not only correct, it is the first consensus rule!).

in src/main.cpp: in e87d06a20e outdated

805+    prevheights.resize(tx.vin.size());
806+    for (size_t txinIndex = 0; txinIndex < tx.vin.size(); txinIndex++) {
807+        const CTxIn& txin = tx.vin[txinIndex];
808+        CCoins coins;
809+        if (!viewMemPool.GetCoins(txin.prevout.hash, coins)) {
810+            return  error("%s: Missing input", __func__);

petertodd commented at 4:29 am on January 31, 2016:

nit: extra space after return

petertodd commented at 9:08 am on January 31, 2016: contributor

ACK https://github.com/morcos/bitcoin/commit/e87d06a20e7d1dd9f3000c419a4dfe805e3dbec4, modulo nits fixed in https://github.com/morcos/bitcoin/pull/7

That said, while I’m happy to see this merged as a mempool-only thing, before we actually soft-fork this in I’d like to see more unit tests; it’s notable and worrying that I could change https://github.com/morcos/bitcoin/blob/e87d06a20e7d1dd9f3000c419a4dfe805e3dbec4/src/main.cpp#L760 to remove the -1 and only one test failed. It might be good to extend the format of the transaction_(in)valid.json tests to let you specify txin heights/blocktimes, and then create some dummy block and coin indexes to use it. Another option might be to make a data-driven block acceptance unittest.

NicolasDorier commented at 9:44 am on January 31, 2016: contributor

@petertodd can you review #7187 as well ? Those two PR goes together.

CodeShark commented at 11:24 am on January 31, 2016: contributor

@morcos I just gave this PR a once over - very clean implementation! I’ll run a few more tests.

morcos commented at 9:25 pm on February 1, 2016: member

@petertodd I addressed your nits, but did it a bit differently than your suggestions. Let me know if you’re ok with this.

In particular though, it occurs to me that the subtracting 1 to retain nLockTime semantics is an artifact of how this code used to be combined with nLockTime checks. These semantics are purely internal. I think it might be cleaner to not subtract 1 and change the comparison to be a strict >? It’s not clear to me what will be more confusing for later coders, because we will be saving these effective lock times in LockPoints as in #7187, so maybe keeping the nLockTime semantics makes sense?

maaku commented at 9:33 pm on February 1, 2016: contributor

Keeping the existing semantics makes a lot more sense, as silly as they are, especially if there is any future refactorings (unknown at this time) that generalize nLockTime and nSequence behavior.

On Mon, Feb 1, 2016 at 1:26 PM, Alex Morcos notifications@github.com wrote:

@petertodd https://github.com/petertodd I addressed your nits, but did it a bit differently than your suggestions. Let me know if you’re ok with this.

In particular though, it occurs to me that the subtracting 1 to retain nLockTime semantics is an artifact of how this code used to be combined with nLockTime checks. These semantics are purely internal. I think it might be cleaner to not subtract 1 and change the comparison to be a strict

? It’s not clear to me what will be more confusing for later coders, because we will be saving these effective lock times in LockPoints as in #7187 #7187, so maybe keeping the nLockTime semantics makes sense?

— Reply to this email directly or view it on GitHub #7184 (comment).

josephpoon commented at 4:24 am on February 3, 2016: none

Tested ACK using btcdrak’s BIP68+OP_CSV combined branch https://github.com/bitcoin/bitcoin/compare/master...btcdrak:sequenceandcsv on regtest.

This pull request is very useful for Lightning Network channels without pre-set expiries. Thanks~~~!

CodeShark commented at 7:38 am on February 4, 2016: contributor

Tested ACK on regtest

petertodd commented at 7:23 pm on February 4, 2016: contributor

utACK https://github.com/morcos/bitcoin/commit/ca786956afa9bf344255959447b0386dd142ebd1

Roasbeef commented at 4:50 am on February 6, 2016: none

LGTM! Err, I mean, Tested ACK :)

Testing methodology:

Pulled down @btcdrak’s combined BIP68+OP_CSV branch: https://github.com/bitcoin/bitcoin/compare/master...btcdrak:sequenceandcsv.
Started a local Bitcoin Core node with the -regtest flag activated.
I then created an integration test in a new branch within Lnd created specifically to test the combined BIP 68+112 branch. This particular test case directly exercises a possible scenario within LN, wherein one of the sides immediately broadcasts the current (and only) commitment transaction. As a result of this uncooperative closure, the party that broadcasted the transaction must wait a CSV delay before they can re-claim their funds.
The test case exercises the following negative paths when verifying a transaction attempting to spend a CSV output, additionally ensuring the txn spending the CSV output is mature enough:
- invalid tx version
- mismatched sequence numbers
- sequence disable flag set
- CSV delay pre-mature

The test case detailed above can be found here.

A major thanks to all those involved from BIP drafting, to BIP review, to coding the reference implementation, to code review, and finally, testing. As Joseph said above, relative time-locks are extremely useful within the Lightning Network 😎.

btcdrak commented at 6:10 am on February 6, 2016: contributor

@NicolasDorier I think you are confused. The combined branch is this PR.

NicolasDorier commented at 6:40 am on February 6, 2016: contributor

Ok my bad, I tought your branch was #6312 (removed my comment)

CodeShark commented at 8:35 pm on February 6, 2016: contributor

Tested ACK using btcdrak’s BIP68+OP_CSV combined branch master…btcdrak:sequenceandcsv on regtest.

in src/main.h: in dc15eadb86 outdated

340@@ -341,7 +341,22 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime);
341  */
342 bool CheckFinalTx(const CTransaction &tx, int flags = -1);
343 
344-/** 
345+/**
346+ * Check if transaction is final per BIP 68 sequence numbers and can be included in a block.

instagibbs commented at 8:50 pm on February 6, 2016:

nit: s/in a block/in a given block/

Took me a few readings to figure out what it was saying.

instagibbs commented at 9:36 pm on February 6, 2016: member

utACK

in src/main.cpp: in dc15eadb86 outdated

775+}
776+
777+static bool EvaluateSequenceLocks(const CBlockIndex& block, std::pair<int, int64_t> lockPair)
778+{
779+    assert(block.pprev);
780+    int64_t nBlockTime = block.pprev->GetMedianTimePast();

NicolasDorier commented at 2:43 pm on February 9, 2016:

What if block is genesis ? Why not just using block.GetAncestor(std::max(nCoinHeight-1, 0)) as above ?

jtimon commented at 3:00 pm on February 9, 2016:

If it’s the genesis block we know is valid and we don’t need to check it. Hasn’t this been discussed in an outdated diff already?

morcos commented at 3:05 pm on February 9, 2016:

See #7184 (review) and #7184 (review)

NicolasDorier commented at 11:18 am on February 10, 2016: contributor

Tested ACK, reimplemented CSV from https://github.com/bitcoin/bitcoin/compare/master...btcdrak:sequenceandcsv in NBitcoin and did some tests at transaction level. (https://github.com/MetacoSA/NBitcoin/blob/csv/NBitcoin.Tests/transaction_tests.cs#L862)

sipa commented at 1:59 pm on February 10, 2016: member

A (harmless) red flag: when CalculateSequenceLocks is called from CheckSequenceLocks, its passed CBlockIndex& object is not fully initialized (only pprev and nHeight are set), so calling GetAncestor looked dangerous to me. It seems that GetAncestor deals correctly with this case, however, and will fall back to walking using pprev instead of pskip in that case.

in src/main.cpp: in dc15eadb86 outdated

789+    return EvaluateSequenceLocks(block, CalculateSequenceLocks(tx, flags, prevHeights, block));
790+}
791+
792+bool CheckSequenceLocks(const CTransaction &tx, int flags)
793+{
794+    AssertLockHeld(cs_main);

sipa commented at 2:00 pm on February 10, 2016:

Also needs a lock on the mempool, I think.

morcos commented at 5:13 pm on February 10, 2016:

added

in src/main.cpp: in dc15eadb86 outdated

800+    // height based locks because when SequenceLocks() is called within
801+    // CBlock::AcceptBlock(), the height of the block *being*
802+    // evaluated is what is used. Thus if we want to know if a
803+    // transaction can be part of the *next* block, we need to call
804+    // SequenceLocks() with one more than chainActive.Height().
805+    index.nHeight = tip->nHeight + 1;

sipa commented at 2:02 pm on February 10, 2016:

It’s also needed because of internal consistency. If x->pprev->nHeight is X, x->nHeight must be X+1.

morcos commented at 2:43 pm on February 10, 2016:

Yep. Originally the intention had been to be able to set this to a higher number if we wanted to allow the mempool to contain currently locked transactions. I discovered that that doesn’t work because of the GetAncestor use in CalculateSequenceLocks. I wrote up some minor changes to pass the height/time you want to check against directly to EvaluateSequenceLocks, but I think it’s not worth making those changes now, we can do it if/when we decide we want to keep non-final txs.

sipa commented at 3:06 pm on February 10, 2016:

Yeah, not worth changing things for. Just saying that there isn’t really any other option.

sipa commented at 2:10 pm on February 10, 2016: member

~~utACK~~

Edit: there’s a locking bug, where CheckSequenceLocks is executed without mempool lock.

in src/main.cpp: in dc15eadb86 outdated

1072@@ -951,6 +1073,12 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState &state, const C
1073         view.SetBackend(dummy);
1074         }
1075 
1076+        // Only accept BIP68 sequence locked transactions that can be mined in the next

sipa commented at 2:11 pm on February 10, 2016:

This should be moved up to be within the above mempool-locked block.

morcos commented at 5:14 pm on February 10, 2016: member

Oops. Nice catch @sipa. Fixed the missing lock. It’s sort of a shame not to reuse the CCoinsViewCache just created in ATMP, but we can save that potential improvement for another time.

btcdrak commented at 5:16 pm on February 10, 2016: contributor

Nice catch @sipa. @sipa is a rockstar! :guitar:

sipa commented at 6:36 pm on February 10, 2016: member

test/miner_tests.cpp:82

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

sipa commented at 8:27 pm on February 10, 2016: member

utACK 4315fe1afce448698732cf1bcffeb070028e4b2b after squashing

Or, utACK tree id 04ef32caaaf2b838295f39cffbdd87468f3657c9

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

A squash should retain the tree id.

Implement SequenceLocks functions

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

The majority of this code is copied from maaku in #6312
Further credit: btcdrak, sipa, NicolasDorier

c6c2f0fd78

Add RPC test exercising BIP68 (mempool only) da6ad5f684

morcos force-pushed on Feb 10, 2016

morcos commented at 8:42 pm on February 10, 2016: member

Ok squashed, same code

0git show -s --format="%T" da6ad5f684b91975cae3f37495ccbd041499e86b
104ef32caaaf2b838295f39cffbdd87468f3657c9

Bug fix to RPC test a51c79be6b

morcos commented at 9:03 pm on February 10, 2016: member

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 commented at 9:32 pm on February 10, 2016: contributor

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

sipa commented at 9:42 pm on February 10, 2016: member

@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 commented at 9:28 am on February 11, 2016: member

@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.

in src/main.cpp: in dc15eadb86 outdated

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

sdaftuar commented at 4:25 pm on February 11, 2016:

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

in src/main.cpp: in dc15eadb86 outdated

799+    // CheckSequenceLocks() uses chainActive.Height()+1 to evaluate
800+    // height based locks because when SequenceLocks() is called within
801+    // CBlock::AcceptBlock(), the height of the block *being*
802+    // evaluated is what is used. Thus if we want to know if a
803+    // transaction can be part of the *next* block, we need to call
804+    // SequenceLocks() with one more than chainActive.Height().

sdaftuar commented at 4:25 pm on February 11, 2016:

nit: This sentence is somewhat unclear, as SequenceLocks isn’t invoked here.

in src/main.cpp: in dc15eadb86 outdated

2235+            for (size_t j = 0; j < tx.vin.size(); j++) {
2236+                prevheights[j] = view.AccessCoins(tx.vin[j].prevout.hash)->nHeight;
2237+            }
2238+
2239+            if (!SequenceLocks(tx, nLockTimeFlags, &prevheights, *pindex)) {
2240+                return state.DoS(100, error("ConnectBlock(): contains a non-BIP68-final transaction", __func__),

sdaftuar commented at 4:25 pm on February 11, 2016:

Need to add a %s or drop this __func__.

in src/main.h: in dc15eadb86 outdated

349+bool SequenceLocks(const CTransaction &tx, int flags, std::vector<int>* prevHeights, const CBlockIndex& block);
350+
351+/**
352+ * Check if transaction will be BIP 68 final in the next block to be created.
353+ *
354+ * Calls SequenceLocks() with data from the tip of the current active chain.

sdaftuar commented at 4:25 pm on February 11, 2016:

This comment is incorrect (CheckSequenceLocks doesn’t actually call SequenceLocks).

in src/policy/policy.h: in dc15eadb86 outdated

44@@ -45,8 +45,9 @@ static const unsigned int STANDARD_SCRIPT_VERIFY_FLAGS = MANDATORY_SCRIPT_VERIFY
45 /** For convenience, standard but not mandatory verify flags. */
46 static const unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS = STANDARD_SCRIPT_VERIFY_FLAGS & ~MANDATORY_SCRIPT_VERIFY_FLAGS;
47 
48-/** Used as the flags parameter to CheckFinalTx() in non-consensus code */
49-static const unsigned int STANDARD_LOCKTIME_VERIFY_FLAGS = LOCKTIME_MEDIAN_TIME_PAST;
50+/** Used as the flags parameter to LockTime() in non-consensus code. */

sdaftuar commented at 4:25 pm on February 11, 2016:

s/LockTime/CheckSequenceLocks/

sipa commented at 7:19 pm on February 11, 2016: member

Mental note: verify how this interacts with the wallet

fix sdaftuar's nits again

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

b043c4b746

sdaftuar commented at 8:41 pm on February 11, 2016: member

ACK b043c4b746c8199ce948aa5e8b186e0d1a61ad68

laanwj merged this on Feb 12, 2016

laanwj closed this on Feb 12, 2016

laanwj referenced this in commit 80d1f2e483 on Feb 12, 2016

laanwj commented at 4:07 pm on February 12, 2016: member

utACK b043c4b

MarkLTZ referenced this in commit 3977804911 on Apr 27, 2019

MarcoFalke locked this on Sep 8, 2021

Implement SequenceLocks functions for BIP 68 #7184