Fix off-by-one errors in the interpretation of IsFinal()

petertodd commented at 10:22 am on March 4, 2013: contributor

Basically AcceptBlock() checks IsFinal() for transactions in a block with nHeight of the block being tested, or essentially nBestHeight+1 however CreateNewBlock() was using just nBestHeight, so transactions were being included into blocks one block later than they could be.

Additionally the new IsFinal() test in IsStandard() had the same issue, as well as the UI. (ironically, a fix I wrote a few months ago, lead astray by simply watching to see when transactions got mined on testnet)

The bug in CreateNewBlock() is especially nasty for fidelity bond sacrifice transactions, because a miner who knew the trick could collect all the fees for himself, one block before anyone else had a chance. (subject to hash power of course)

Anyway, I should update the patch with unittests and investigate the issue more carefully; for instance, have there ever been any nLockTime’d transactions ever mined in the main chain at the minimum possible block? Also, can the same logic be fixed for time-locked transactions?

BitcoinPullTester commented at 12:24 pm on March 4, 2013: none

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

in src/main.cpp: in 67c7dab125 outdated

359@@ -360,7 +360,30 @@ bool CTransaction::IsStandard() const
360     if (nVersion > CTransaction::CURRENT_VERSION)
361         return false;
362 
363-    if (!IsFinal())
364+    // Treat non-final transactions as non-standard to prevent a specific type
365+    // of double-spend attack, as well as DoS attacks. (if the transaction
366+    // can't be mined, the attacker isn't expending resources broadcasting it)
367+    // Basically we don't want to propagate transactions can't included in the

sipa commented at 9:09 pm on April 12, 2013:

nit: transactions that can’t be included

sipa commented at 9:12 pm on April 12, 2013: member

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

sipa commented at 11:02 pm on April 12, 2013: member

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.

petertodd commented at 1:34 am on April 14, 2013: contributor

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.

jgarzik commented at 4:51 pm on May 30, 2013: contributor

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

petertodd commented at 7:30 pm on June 1, 2013: contributor

@jgarzik Updated, removed FIXME.

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

jgarzik commented at 7:10 pm on June 19, 2013: contributor

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

petertodd commented at 3:45 am on June 20, 2013: contributor

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.

luke-jr commented at 3:47 am on July 17, 2013: member

@petertodd Needs rebase.

petertodd commented at 6:02 pm on August 25, 2013: contributor

Rebased and added CreateNewBlock() unittests

petertodd commented at 11:34 pm on August 25, 2013: contributor

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.

sipa commented at 11:43 pm on August 25, 2013: member

ACK

petertodd referenced this in commit e9124c0fb8 on Sep 15, 2013

laanwj commented at 1:05 pm on January 24, 2014: member

FYI I rebased this here https://github.com/laanwj/bitcoin/commit/c3858c56c8485ce7d39fb4905948bda63b80b965 , needed changes to the main and GUI code were:

0nBestHeight -> chainActive.Height()

Needed changes miner_tests:

0pindexBest -> chainActive.Tip()
1mempool.addUnchecked(hash, tx) -> mempool.addUnchecked(hash, CTxMemPoolEntry(tx, ...))
2reservekey -> 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.

Fix off-by-one errors in use of IsFinalTx()

Previously CreateNewBlock() didn't take into account the fact that
IsFinalTx() without any arguments tests if the transaction is considered
final in the *current* block, when both those functions really needed to
know if the transaction would be final in the *next* block.

Additionally the UI had a similar misunderstanding.

Also adds some basic tests to check that CreateNewBlock() is in fact
mining nLockTime-using transactions correctly.

Thanks to Wladimir J. van der Laan for rebase.

665bdd3bc9

petertodd commented at 3:09 am on January 27, 2014: contributor

@laanwj Thanks.

ACK

BitcoinPullTester commented at 3:30 am on January 27, 2014: none

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.

laanwj referenced this in commit ca1913e8f6 on Jan 27, 2014

laanwj merged this on Jan 27, 2014

laanwj closed this on Jan 27, 2014

DrahtBot locked this on Sep 8, 2021

Fix off-by-one errors in the interpretation of IsFinal() #2342