diff --git a/src/main.cpp b/src/main.cpp index efc70af9..8c60a26b 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 ca3b65a1..f9dab4bd 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 2c3a9e3a..c76b2986 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 6823557e..257151b9 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 8e3091d5..d35137a6 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)