Browse Source

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.
0.10
Peter Todd 11 years ago
parent
commit
665bdd3bc9
No known key found for this signature in database
GPG Key ID: 2481403DA5F091FB
  1. 19
      src/main.cpp
  2. 2
      src/miner.cpp
  3. 4
      src/qt/transactiondesc.cpp
  4. 4
      src/qt/transactionrecord.cpp
  5. 51
      src/test/miner_tests.cpp

19
src/main.cpp

@ -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;
} }

2
src/miner.cpp

@ -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;

4
src/qt/transactiondesc.cpp

@ -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));
} }

4
src/qt/transactionrecord.cpp

@ -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
{ {

51
src/test/miner_tests.cpp

@ -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)

Loading…
Cancel
Save