Skip to content

Commit 75a4d51

Browse files
petertoddlaanwj
petertodd
authored and
laanwj
committed
Fix off-by-one error w/ nLockTime in the wallet
Previously due to an off-by-one error the wallet ignored nLockTime-by-height transactions that would be valid in the next block even though they are accepted into the mempool. The transactions wouldn't show up until confirmed, nor would they be included in the unconfirmed balance. Similar to the mempool behavior fix in 665bdd3, the wallet code was calling IsFinalTx() directly without taking into account the fact that doing so tells you if the transaction could have been mined in the *current* block, rather than the next block. To fix this we strip IsFinalTx() of non-consensus-critical functionality, removing the default arguments, and add CheckFinalTx() to check if a transaction will be final in the next block. Github-Pull: bitcoin#6183 Rebased-From: 28bf062
1 parent 2be094e commit 75a4d51

File tree

8 files changed

+39
-38
lines changed

8 files changed

+39
-38
lines changed

‎src/main.cpp‎

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -671,14 +671,8 @@ bool IsStandardTx(const CTransaction& tx, string& reason)
671671

672672
bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
673673
{
674-
AssertLockHeld(cs_main);
675-
// Time based nLockTime implemented in 0.1.6
676674
if (tx.nLockTime == 0)
677675
return true;
678-
if (nBlockHeight == 0)
679-
nBlockHeight = chainActive.Height();
680-
if (nBlockTime == 0)
681-
nBlockTime = GetAdjustedTime();
682676
if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))
683677
return true;
684678
BOOST_FOREACH(const CTxIn& txin, tx.vin)
@@ -687,6 +681,12 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
687681
return true;
688682
}
689683

684+
bool CheckFinalTx(const CTransaction &tx)
685+
{
686+
AssertLockHeld(cs_main);
687+
return IsFinalTx(tx, chainActive.Height() + 1, GetAdjustedTime());
688+
}
689+
690690
/**
691691
* Check transaction inputs to mitigate two
692692
* potential denial-of-service attacks:
@@ -903,21 +903,8 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
903903
// Only accept nLockTime-using transactions that can be mined in the next
904904
// block; we don't want our mempool filled up with transactions that can't
905905
// be mined yet.
906-
//
907-
// However, IsFinalTx() is confusing... Without arguments, it uses
908-
// chainActive.Height() to evaluate nLockTime; when a block is accepted,
909-
// chainActive.Height() is set to the value of nHeight in the block.
910-
// However, when IsFinalTx() is called within CBlock::AcceptBlock(), the
911-
// height of the block *being* evaluated is what is used. Thus if we want
912-
// to know if a transaction can be part of the *next* block, we need to
913-
// call IsFinalTx() with one more than chainActive.Height().
914-
//
915-
// Timestamps on the other hand don't get any special treatment, because we
916-
// can't know what timestamp the next block will have, and there aren't
917-
// timestamp applications where it matters.
918-
if (!IsFinalTx(tx, chainActive.Height() + 1))
919-
return state.DoS(0,
920-
error("AcceptToMemoryPool: non-final"),
906+
if (!CheckFinalTx(tx))
907+
return state.DoS(0, error("AcceptToMemoryPool: non-final"),
921908
REJECT_NONSTANDARD, "non-final");
922909

923910
// is it already in the memory pool?

‎src/main.h‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,18 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state);
324324
*/
325325
bool IsStandardTx(const CTransaction& tx, std::string& reason);
326326

327-
bool IsFinalTx(const CTransaction &tx, int nBlockHeight = 0, int64_t nBlockTime = 0);
327+
/**
328+
* Check if transaction is final and can be included in a block with the
329+
* specified height and time. Consensus critical.
330+
*/
331+
bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime);
332+
333+
/**
334+
* Check if transaction will be final in the next block to be created.
335+
*
336+
* Calls IsFinalTx() with current block height and appropriate block time.
337+
*/
338+
bool CheckFinalTx(const CTransaction &tx);
328339

329340
/**
330341
* Closure representing one script verification

‎src/miner.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
138138
LOCK2(cs_main, mempool.cs);
139139
CBlockIndex* pindexPrev = chainActive.Tip();
140140
const int nHeight = pindexPrev->nHeight + 1;
141+
pblock->nTime = GetAdjustedTime();
141142
CCoinsViewCache view(pcoinsTip);
142143

143144
// Priority order to process transactions
@@ -152,7 +153,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
152153
mi != mempool.mapTx.end(); ++mi)
153154
{
154155
const CTransaction& tx = mi->second.GetTx();
155-
if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight))
156+
if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight, pblock->nTime))
156157
continue;
157158

158159
COrphan* porphan = NULL;

‎src/qt/transactiondesc.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ using namespace std;
2626
QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx)
2727
{
2828
AssertLockHeld(cs_main);
29-
if (!IsFinalTx(wtx, chainActive.Height() + 1))
29+
if (!CheckFinalTx(wtx))
3030
{
3131
if (wtx.nLockTime < LOCKTIME_THRESHOLD)
3232
return tr("Open for %n more block(s)", "", wtx.nLockTime - chainActive.Height());

‎src/qt/transactionrecord.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx)
188188
status.depth = wtx.GetDepthInMainChain();
189189
status.cur_num_blocks = chainActive.Height();
190190

191-
if (!IsFinalTx(wtx, chainActive.Height() + 1))
191+
if (!CheckFinalTx(wtx))
192192
{
193193
if (wtx.nLockTime < LOCKTIME_THRESHOLD)
194194
{

‎src/test/miner_tests.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
223223
tx.nLockTime = chainActive.Tip()->nHeight+1;
224224
hash = tx.GetHash();
225225
mempool.addUnchecked(hash, CTxMemPoolEntry(tx, 11, GetTime(), 111.0, 11));
226-
BOOST_CHECK(!IsFinalTx(tx, chainActive.Tip()->nHeight + 1));
226+
BOOST_CHECK(!CheckFinalTx(tx));
227227

228228
// time locked
229229
tx2.vin.resize(1);
@@ -237,7 +237,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
237237
tx2.nLockTime = chainActive.Tip()->GetMedianTimePast()+1;
238238
hash = tx2.GetHash();
239239
mempool.addUnchecked(hash, CTxMemPoolEntry(tx2, 11, GetTime(), 111.0, 11));
240-
BOOST_CHECK(!IsFinalTx(tx2));
240+
BOOST_CHECK(!CheckFinalTx(tx2));
241241

242242
BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey));
243243

@@ -249,8 +249,10 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
249249
chainActive.Tip()->nHeight++;
250250
SetMockTime(chainActive.Tip()->GetMedianTimePast()+2);
251251

252-
BOOST_CHECK(IsFinalTx(tx, chainActive.Tip()->nHeight + 1));
253-
BOOST_CHECK(IsFinalTx(tx2));
252+
// FIXME: we should *actually* create a new block so the following test
253+
// works; CheckFinalTx() isn't fooled by monkey-patching nHeight.
254+
//BOOST_CHECK(CheckFinalTx(tx));
255+
//BOOST_CHECK(CheckFinalTx(tx2));
254256

255257
BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey));
256258
BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3);

‎src/wallet/rpcwallet.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -588,7 +588,7 @@ Value getreceivedbyaddress(const Array& params, bool fHelp)
588588
for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it)
589589
{
590590
const CWalletTx& wtx = (*it).second;
591-
if (wtx.IsCoinBase() || !IsFinalTx(wtx))
591+
if (wtx.IsCoinBase() || !CheckFinalTx(wtx))
592592
continue;
593593

594594
BOOST_FOREACH(const CTxOut& txout, wtx.vout)
@@ -642,7 +642,7 @@ Value getreceivedbyaccount(const Array& params, bool fHelp)
642642
for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it)
643643
{
644644
const CWalletTx& wtx = (*it).second;
645-
if (wtx.IsCoinBase() || !IsFinalTx(wtx))
645+
if (wtx.IsCoinBase() || !CheckFinalTx(wtx))
646646
continue;
647647

648648
BOOST_FOREACH(const CTxOut& txout, wtx.vout)
@@ -666,7 +666,7 @@ CAmount GetAccountBalance(CWalletDB& walletdb, const string& strAccount, int nMi
666666
for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it)
667667
{
668668
const CWalletTx& wtx = (*it).second;
669-
if (!IsFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0 || wtx.GetDepthInMainChain() < 0)
669+
if (!CheckFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0 || wtx.GetDepthInMainChain() < 0)
670670
continue;
671671

672672
CAmount nReceived, nSent, nFee;
@@ -1109,7 +1109,7 @@ Value ListReceived(const Array& params, bool fByAccounts)
11091109
{
11101110
const CWalletTx& wtx = (*it).second;
11111111

1112-
if (wtx.IsCoinBase() || !IsFinalTx(wtx))
1112+
if (wtx.IsCoinBase() || !CheckFinalTx(wtx))
11131113
continue;
11141114

11151115
int nDepth = wtx.GetDepthInMainChain();

‎src/wallet/wallet.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1318,7 +1318,7 @@ CAmount CWalletTx::GetChange() const
13181318
bool CWalletTx::IsTrusted() const
13191319
{
13201320
// Quick answer in most cases
1321-
if (!IsFinalTx(*this))
1321+
if (!CheckFinalTx(*this))
13221322
return false;
13231323
int nDepth = GetDepthInMainChain();
13241324
if (nDepth >= 1)
@@ -1424,7 +1424,7 @@ CAmount CWallet::GetUnconfirmedBalance() const
14241424
for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
14251425
{
14261426
const CWalletTx* pcoin = &(*it).second;
1427-
if (!IsFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0))
1427+
if (!CheckFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0))
14281428
nTotal += pcoin->GetAvailableCredit();
14291429
}
14301430
}
@@ -1469,7 +1469,7 @@ CAmount CWallet::GetUnconfirmedWatchOnlyBalance() const
14691469
for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
14701470
{
14711471
const CWalletTx* pcoin = &(*it).second;
1472-
if (!IsFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0))
1472+
if (!CheckFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0))
14731473
nTotal += pcoin->GetAvailableWatchOnlyCredit();
14741474
}
14751475
}
@@ -1504,7 +1504,7 @@ void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const
15041504
const uint256& wtxid = it->first;
15051505
const CWalletTx* pcoin = &(*it).second;
15061506

1507-
if (!IsFinalTx(*pcoin))
1507+
if (!CheckFinalTx(*pcoin))
15081508
continue;
15091509

15101510
if (fOnlyConfirmed && !pcoin->IsTrusted())
@@ -2291,7 +2291,7 @@ std::map<CTxDestination, CAmount> CWallet::GetAddressBalances()
22912291
{
22922292
CWalletTx *pcoin = &walletEntry.second;
22932293

2294-
if (!IsFinalTx(*pcoin) || !pcoin->IsTrusted())
2294+
if (!CheckFinalTx(*pcoin) || !pcoin->IsTrusted())
22952295
continue;
22962296

22972297
if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0)

0 commit comments

Comments
 (0)