From 665bdd3bc9ba4ac566edf5ba3fa8bbd93eb4780f Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Sun, 26 Jan 2014 21:50:15 -0500 Subject: [PATCH] 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. --- src/main.cpp | 19 +++++++++++++- src/miner.cpp | 2 +- src/qt/transactiondesc.cpp | 4 +-- src/qt/transactionrecord.cpp | 4 +-- src/test/miner_tests.cpp | 51 +++++++++++++++++++++++++++++++++++- 5 files changed, 73 insertions(+), 7 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index efc70af90..8c60a26b3 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; } diff --git a/src/miner.cpp b/src/miner.cpp index ca3b65a11..f9dab4bd6 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -158,7 +158,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn) mi != mempool.mapTx.end(); ++mi) { const CTransaction& tx = mi->second.GetTx(); - if (tx.IsCoinBase() || !IsFinalTx(tx)) + if (tx.IsCoinBase() || !IsFinalTx(tx, pindexPrev->nHeight + 1)) continue; COrphan* porphan = NULL; diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp index 2c3a9e3a5..c76b29861 100644 --- a/src/qt/transactiondesc.cpp +++ b/src/qt/transactiondesc.cpp @@ -20,10 +20,10 @@ QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx) { - if (!IsFinalTx(wtx)) + if (!IsFinalTx(wtx, chainActive.Height() + 1)) { if (wtx.nLockTime < LOCKTIME_THRESHOLD) - return tr("Open for %n more block(s)", "", wtx.nLockTime - chainActive.Height() + 1); + return tr("Open for %n more block(s)", "", wtx.nLockTime - chainActive.Height()); else return tr("Open until %1").arg(GUIUtil::dateTimeStr(wtx.nLockTime)); } diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp index 6823557eb..257151b92 100644 --- a/src/qt/transactionrecord.cpp +++ b/src/qt/transactionrecord.cpp @@ -168,12 +168,12 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx) status.depth = wtx.GetDepthInMainChain(); status.cur_num_blocks = chainActive.Height(); - if (!IsFinalTx(wtx)) + if (!IsFinalTx(wtx, chainActive.Height() + 1)) { if (wtx.nLockTime < LOCKTIME_THRESHOLD) { status.status = TransactionStatus::OpenUntilBlock; - status.open_for = wtx.nLockTime - chainActive.Height() + 1; + status.open_for = wtx.nLockTime - chainActive.Height(); } else { diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 8e3091d55..d35137a66 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -51,7 +51,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) { CScript scriptPubKey = CScript() << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f") << OP_CHECKSIG; CBlockTemplate *pblocktemplate; - CTransaction tx; + CTransaction tx,tx2; CScript script; uint256 hash; @@ -204,8 +204,57 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) delete pblocktemplate; chainActive.Tip()->nHeight = nHeight; + // non-final txs in mempool + SetMockTime(chainActive.Tip()->GetMedianTimePast()+1); + + // height locked + tx.vin[0].prevout.hash = txFirst[0]->GetHash(); + tx.vin[0].scriptSig = CScript() << OP_1; + tx.vin[0].nSequence = 0; + tx.vout[0].nValue = 4900000000LL; + tx.vout[0].scriptPubKey = CScript() << OP_1; + tx.nLockTime = chainActive.Tip()->nHeight+1; + hash = tx.GetHash(); + mempool.addUnchecked(hash, CTxMemPoolEntry(tx, 11, GetTime(), 111.0, 11)); + BOOST_CHECK(!IsFinalTx(tx, chainActive.Tip()->nHeight + 1)); + + // time locked + tx2.vin.resize(1); + tx2.vin[0].prevout.hash = txFirst[1]->GetHash(); + tx2.vin[0].prevout.n = 0; + tx2.vin[0].scriptSig = CScript() << OP_1; + tx2.vin[0].nSequence = 0; + tx2.vout.resize(1); + tx2.vout[0].nValue = 4900000000LL; + tx2.vout[0].scriptPubKey = CScript() << OP_1; + tx2.nLockTime = chainActive.Tip()->GetMedianTimePast()+1; + hash = tx2.GetHash(); + mempool.addUnchecked(hash, CTxMemPoolEntry(tx2, 11, GetTime(), 111.0, 11)); + BOOST_CHECK(!IsFinalTx(tx2)); + + BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey)); + + // Neither tx should have make it into the template. + BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 1); + delete pblocktemplate; + + // However if we advance height and time by one, both will. + chainActive.Tip()->nHeight++; + SetMockTime(chainActive.Tip()->GetMedianTimePast()+2); + + BOOST_CHECK(IsFinalTx(tx, chainActive.Tip()->nHeight + 1)); + BOOST_CHECK(IsFinalTx(tx2)); + + BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey)); + BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3); + delete pblocktemplate; + + chainActive.Tip()->nHeight--; + SetMockTime(0); + BOOST_FOREACH(CTransaction *tx, txFirst) delete tx; + } BOOST_AUTO_TEST_CASE(sha256transform_equality)