Merge pull request #2342

665bdd3 Fix off-by-one errors in use of IsFinalTx() (Peter Todd)
This commit is contained in:
Wladimir J. van der Laan 2014-01-27 10:15:18 +01:00
commit ca1913e8f6
No known key found for this signature in database
GPG Key ID: 74810B012346C9A6
5 changed files with 73 additions and 7 deletions

View File

@ -365,7 +365,24 @@ bool IsStandardTx(const CTransaction& tx, string& reason)
return false; 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"; reason = "non-final";
return false; return false;
} }

View File

@ -158,7 +158,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
mi != mempool.mapTx.end(); ++mi) mi != mempool.mapTx.end(); ++mi)
{ {
const CTransaction& tx = mi->second.GetTx(); const CTransaction& tx = mi->second.GetTx();
if (tx.IsCoinBase() || !IsFinalTx(tx)) if (tx.IsCoinBase() || !IsFinalTx(tx, pindexPrev->nHeight + 1))
continue; continue;
COrphan* porphan = NULL; COrphan* porphan = NULL;

View File

@ -20,10 +20,10 @@
QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx) QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx)
{ {
if (!IsFinalTx(wtx)) if (!IsFinalTx(wtx, chainActive.Height() + 1))
{ {
if (wtx.nLockTime < LOCKTIME_THRESHOLD) 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 else
return tr("Open until %1").arg(GUIUtil::dateTimeStr(wtx.nLockTime)); return tr("Open until %1").arg(GUIUtil::dateTimeStr(wtx.nLockTime));
} }

View File

@ -168,12 +168,12 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx)
status.depth = wtx.GetDepthInMainChain(); status.depth = wtx.GetDepthInMainChain();
status.cur_num_blocks = chainActive.Height(); status.cur_num_blocks = chainActive.Height();
if (!IsFinalTx(wtx)) if (!IsFinalTx(wtx, chainActive.Height() + 1))
{ {
if (wtx.nLockTime < LOCKTIME_THRESHOLD) if (wtx.nLockTime < LOCKTIME_THRESHOLD)
{ {
status.status = TransactionStatus::OpenUntilBlock; status.status = TransactionStatus::OpenUntilBlock;
status.open_for = wtx.nLockTime - chainActive.Height() + 1; status.open_for = wtx.nLockTime - chainActive.Height();
} }
else else
{ {

View File

@ -51,7 +51,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
{ {
CScript scriptPubKey = CScript() << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f") << OP_CHECKSIG; CScript scriptPubKey = CScript() << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f") << OP_CHECKSIG;
CBlockTemplate *pblocktemplate; CBlockTemplate *pblocktemplate;
CTransaction tx; CTransaction tx,tx2;
CScript script; CScript script;
uint256 hash; uint256 hash;
@ -204,8 +204,57 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
delete pblocktemplate; delete pblocktemplate;
chainActive.Tip()->nHeight = nHeight; 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) BOOST_FOREACH(CTransaction *tx, txFirst)
delete tx; delete tx;
} }
BOOST_AUTO_TEST_CASE(sha256transform_equality) BOOST_AUTO_TEST_CASE(sha256transform_equality)