Skip to content

Commit 665bdd3

Browse files
petertoddpetertodd
committed
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 parent d0a94f2 commit 665bdd3

File tree

5 files changed

+73
-7
lines changed

5 files changed

+73
-7
lines changed

‎src/main.cpp‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,24 @@ bool IsStandardTx(const CTransaction& tx, string& reason)
365365
return false;
366366
}
367367

368-
if (!IsFinalTx(tx)) {
368+
// Treat non-final transactions as non-standard to prevent a specific type
369+
// of double-spend attack, as well as DoS attacks. (if the transaction
370+
// can't be mined, the attacker isn't expending resources broadcasting it)
371+
// Basically we don't want to propagate transactions that can't included in
372+
// the next block.
373+
//
374+
// However, IsFinalTx() is confusing... Without arguments, it uses
375+
// chainActive.Height() to evaluate nLockTime; when a block is accepted, chainActive.Height()
376+
// is set to the value of nHeight in the block. However, when IsFinalTx()
377+
// is called within CBlock::AcceptBlock(), the height of the block *being*
378+
// evaluated is what is used. Thus if we want to know if a transaction can
379+
// be part of the *next* block, we need to call IsFinalTx() with one more
380+
// than chainActive.Height().
381+
//
382+
// Timestamps on the other hand don't get any special treatment, because we
383+
// can't know what timestamp the next block will have, and there aren't
384+
// timestamp applications where it matters.
385+
if (!IsFinalTx(tx, chainActive.Height() + 1)) {
369386
reason = "non-final";
370387
return false;
371388
}

‎src/miner.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
158158
mi != mempool.mapTx.end(); ++mi)
159159
{
160160
const CTransaction& tx = mi->second.GetTx();
161-
if (tx.IsCoinBase() || !IsFinalTx(tx))
161+
if (tx.IsCoinBase() || !IsFinalTx(tx, pindexPrev->nHeight + 1))
162162
continue;
163163

164164
COrphan* porphan = NULL;

‎src/qt/transactiondesc.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,10 @@
2020

2121
QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx)
2222
{
23-
if (!IsFinalTx(wtx))
23+
if (!IsFinalTx(wtx, chainActive.Height() + 1))
2424
{
2525
if (wtx.nLockTime < LOCKTIME_THRESHOLD)
26-
return tr("Open for %n more block(s)", "", wtx.nLockTime - chainActive.Height() + 1);
26+
return tr("Open for %n more block(s)", "", wtx.nLockTime - chainActive.Height());
2727
else
2828
return tr("Open until %1").arg(GUIUtil::dateTimeStr(wtx.nLockTime));
2929
}

‎src/qt/transactionrecord.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -168,12 +168,12 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx)
168168
status.depth = wtx.GetDepthInMainChain();
169169
status.cur_num_blocks = chainActive.Height();
170170

171-
if (!IsFinalTx(wtx))
171+
if (!IsFinalTx(wtx, chainActive.Height() + 1))
172172
{
173173
if (wtx.nLockTime < LOCKTIME_THRESHOLD)
174174
{
175175
status.status = TransactionStatus::OpenUntilBlock;
176-
status.open_for = wtx.nLockTime - chainActive.Height() + 1;
176+
status.open_for = wtx.nLockTime - chainActive.Height();
177177
}
178178
else
179179
{

‎src/test/miner_tests.cpp‎

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
5151
{
5252
CScript scriptPubKey = CScript() << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f") << OP_CHECKSIG;
5353
CBlockTemplate *pblocktemplate;
54-
CTransaction tx;
54+
CTransaction tx,tx2;
5555
CScript script;
5656
uint256 hash;
5757

@@ -204,8 +204,57 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
204204
delete pblocktemplate;
205205
chainActive.Tip()->nHeight = nHeight;
206206

207+
// non-final txs in mempool
208+
SetMockTime(chainActive.Tip()->GetMedianTimePast()+1);
209+
210+
// height locked
211+
tx.vin[0].prevout.hash = txFirst[0]->GetHash();
212+
tx.vin[0].scriptSig = CScript() << OP_1;
213+
tx.vin[0].nSequence = 0;
214+
tx.vout[0].nValue = 4900000000LL;
215+
tx.vout[0].scriptPubKey = CScript() << OP_1;
216+
tx.nLockTime = chainActive.Tip()->nHeight+1;
217+
hash = tx.GetHash();
218+
mempool.addUnchecked(hash, CTxMemPoolEntry(tx, 11, GetTime(), 111.0, 11));
219+
BOOST_CHECK(!IsFinalTx(tx, chainActive.Tip()->nHeight + 1));
220+
221+
// time locked
222+
tx2.vin.resize(1);
223+
tx2.vin[0].prevout.hash = txFirst[1]->GetHash();
224+
tx2.vin[0].prevout.n = 0;
225+
tx2.vin[0].scriptSig = CScript() << OP_1;
226+
tx2.vin[0].nSequence = 0;
227+
tx2.vout.resize(1);
228+
tx2.vout[0].nValue = 4900000000LL;
229+
tx2.vout[0].scriptPubKey = CScript() << OP_1;
230+
tx2.nLockTime = chainActive.Tip()->GetMedianTimePast()+1;
231+
hash = tx2.GetHash();
232+
mempool.addUnchecked(hash, CTxMemPoolEntry(tx2, 11, GetTime(), 111.0, 11));
233+
BOOST_CHECK(!IsFinalTx(tx2));
234+
235+
BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey));
236+
237+
// Neither tx should have make it into the template.
238+
BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 1);
239+
delete pblocktemplate;
240+
241+
// However if we advance height and time by one, both will.
242+
chainActive.Tip()->nHeight++;
243+
SetMockTime(chainActive.Tip()->GetMedianTimePast()+2);
244+
245+
BOOST_CHECK(IsFinalTx(tx, chainActive.Tip()->nHeight + 1));
246+
BOOST_CHECK(IsFinalTx(tx2));
247+
248+
BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey));
249+
BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3);
250+
delete pblocktemplate;
251+
252+
chainActive.Tip()->nHeight--;
253+
SetMockTime(0);
254+
207255
BOOST_FOREACH(CTransaction *tx, txFirst)
208256
delete tx;
257+
209258
}
210259

211260
BOOST_AUTO_TEST_CASE(sha256transform_equality)

0 commit comments

Comments
 (0)