Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/67c7dab125e98a0c7b5e55035ddc5c90ab3b369d for binaries and test log.

I like this idea, but it does need unit tests and some investigation for potential effects, as noted in the message.

Here's a list of all effectively-height-locked transactions in the main chain (that means: excluding those where all inputs have nSequence=UINT_MAX): http://bitcoin.sipa.be/lockheight.txt - It seems the inclusion height is always at least two more than the number in nLockTime - one more than strictly required.

Not many people uses nLockTime; about 75% of those transactions are mine.

Can you run your script on testnet? I did one "min-height" tx myself, I wonder if anyone else did any.

Mainly I think that BlueMatt's tests need more nLockTime stuff, IE the https://github.com/TheBlueMatt/test-scripts stuff. I don't think I'll have time to write any myself until after the conference.

Status? Still has a FIXME. Let's close this or get it updated and merged.

@jgarzik Updated, removed FIXME.

Min-height nLockTime has been tested on testnet without any issues.

Thinking about this some more. Regardless of the correctness, I don't like the addition of magic numbers (+1, +2) in various code locations.

The +1 isn't a magic number; note how in one place the patch removes a +1. Rather it's there from how nBestHeight is the height of the current best block, and in some places we want to check IsFinal() against the next block.

I'd argue the +2 isn't a magic number either, as it's a combination of +1 to get the next block, and another +1 from the assumption that no-one controls >50% of the hashing power as the comment mentions.

Speaking of, while I was thinking about this and working on my mempool rewrite, it occurred to me that given that nBestHeight can decrements under the highly unusual condition of a re-org right on a re-target boundary passing around not quite yet final tx's might help exercise mempool and related code dealing with non-final txs. Not a particularly strong argument, but it's worth thinking about.

@petertodd Needs rebase.

Rebased and added CreateNewBlock() unittests

I did another scan of the blockchain looking for nLockTime using transactions: https://s3.amazonaws.com/peter.todd/nLockTime.log

Still none mined at the minimum possible heights, and very few that have used lock-by-time. However this has been tested on testnet.

ACK

FYI I rebased this here laanwj@c3858c5 , needed changes to the main and GUI code were:

nBestHeight -> chainActive.Height()

Needed changes miner_tests:

pindexBest -> chainActive.Tip()
mempool.addUnchecked(hash, tx) -> mempool.addUnchecked(hash, CTxMemPoolEntry(tx, ...))
reservekey -> fixed scriptPubKey

The tests pass successfully.

Seeing that @sipa already acked this 5 months ago, if you can check that my rebase is correct we may be able to merge this before the 0.9 release.

@laanwj Thanks.

ACK

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/665bdd3bc9ba4ac566edf5ba3fa8bbd93eb4780f for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.