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.

1 files changed, 18 insertions, 1 deletions

diff --git a/src/main.cpp b/src/main.cpp
index efc70af908..8c60a26b38 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -365,7 +365,24 @@ bool IsStandardTx(const CTransaction& tx, string& reason)
         return false;
     }
 
-    if (!IsFinalTx(tx)) {
+    // Treat non-final transactions as non-standard to prevent a specific type
+    // of double-spend attack, as well as DoS attacks. (if the transaction
+    // can't be mined, the attacker isn't expending resources broadcasting it)
+    // Basically we don't want to propagate transactions that can't included in
+    // the next block.
+    //
+    // However, IsFinalTx() is confusing... Without arguments, it uses
+    // chainActive.Height() to evaluate nLockTime; when a block is accepted, chainActive.Height()
+    // is set to the value of nHeight in the block. However, when IsFinalTx()
+    // is called within CBlock::AcceptBlock(), the height of the block *being*
+    // evaluated is what is used. Thus if we want to know if a transaction can
+    // be part of the *next* block, we need to call IsFinalTx() with one more
+    // than chainActive.Height().
+    //
+    // Timestamps on the other hand don't get any special treatment, because we
+    // can't know what timestamp the next block will have, and there aren't
+    // timestamp applications where it matters.
+    if (!IsFinalTx(tx, chainActive.Height() + 1)) {
         reason = "non-final";
         return false;
     }