From 28bf06236d3b385e95fe26a7a742395b30efd6ee Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Mon, 25 May 2015 00:48:33 -0400 Subject: [PATCH] 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 665bdd3b, 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. --- src/main.cpp | 29 ++++++++--------------------- src/main.h | 13 ++++++++++++- src/miner.cpp | 3 ++- src/qt/transactiondesc.cpp | 2 +- src/qt/transactionrecord.cpp | 2 +- src/test/miner_tests.cpp | 10 ++++++---- src/wallet/rpcwallet.cpp | 8 ++++---- src/wallet/wallet.cpp | 10 +++++----- 8 files changed, 39 insertions(+), 38 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index d3956fafd..9eec21edd 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -658,14 +658,8 @@ bool IsStandardTx(const CTransaction& tx, string& reason) bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime) { - AssertLockHeld(cs_main); - // Time based nLockTime implemented in 0.1.6 if (tx.nLockTime == 0) return true; - if (nBlockHeight == 0) - nBlockHeight = chainActive.Height(); - if (nBlockTime == 0) - nBlockTime = GetAdjustedTime(); if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime)) return true; BOOST_FOREACH(const CTxIn& txin, tx.vin) @@ -674,6 +668,12 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime) return true; } +bool CheckFinalTx(const CTransaction &tx) +{ + AssertLockHeld(cs_main); + return IsFinalTx(tx, chainActive.Height() + 1, GetAdjustedTime()); +} + /** * Check transaction inputs to mitigate two * potential denial-of-service attacks: @@ -890,21 +890,8 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa // Only accept nLockTime-using transactions that can be mined in the next // block; we don't want our mempool filled up with transactions that can't // be mined yet. - // - // 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)) - return state.DoS(0, - error("AcceptToMemoryPool: non-final"), + if (!CheckFinalTx(tx)) + return state.DoS(0, error("AcceptToMemoryPool: non-final"), REJECT_NONSTANDARD, "non-final"); // is it already in the memory pool? diff --git a/src/main.h b/src/main.h index 07a709b75..b789ea95e 100644 --- a/src/main.h +++ b/src/main.h @@ -334,7 +334,18 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state); */ bool IsStandardTx(const CTransaction& tx, std::string& reason); -bool IsFinalTx(const CTransaction &tx, int nBlockHeight = 0, int64_t nBlockTime = 0); +/** + * Check if transaction is final and can be included in a block with the + * specified height and time. Consensus critical. + */ +bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime); + +/** + * Check if transaction will be final in the next block to be created. + * + * Calls IsFinalTx() with current block height and appropriate block time. + */ +bool CheckFinalTx(const CTransaction &tx); /** * Closure representing one script verification diff --git a/src/miner.cpp b/src/miner.cpp index 804a68852..fb417ac62 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -137,6 +137,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn) LOCK2(cs_main, mempool.cs); CBlockIndex* pindexPrev = chainActive.Tip(); const int nHeight = pindexPrev->nHeight + 1; + pblock->nTime = GetAdjustedTime(); CCoinsViewCache view(pcoinsTip); // Priority order to process transactions @@ -151,7 +152,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn) mi != mempool.mapTx.end(); ++mi) { const CTransaction& tx = mi->second.GetTx(); - if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight)) + if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight, pblock->nTime)) continue; COrphan* porphan = NULL; diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp index 4fffd03ad..5662b1665 100644 --- a/src/qt/transactiondesc.cpp +++ b/src/qt/transactiondesc.cpp @@ -26,7 +26,7 @@ using namespace std; QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx) { AssertLockHeld(cs_main); - if (!IsFinalTx(wtx, chainActive.Height() + 1)) + if (!CheckFinalTx(wtx)) { if (wtx.nLockTime < LOCKTIME_THRESHOLD) return tr("Open for %n more block(s)", "", wtx.nLockTime - chainActive.Height()); diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp index 7f1db58e5..15d13e9fc 100644 --- a/src/qt/transactionrecord.cpp +++ b/src/qt/transactionrecord.cpp @@ -188,7 +188,7 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx) status.depth = wtx.GetDepthInMainChain(); status.cur_num_blocks = chainActive.Height(); - if (!IsFinalTx(wtx, chainActive.Height() + 1)) + if (!CheckFinalTx(wtx)) { if (wtx.nLockTime < LOCKTIME_THRESHOLD) { diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index d7ea91607..4ceda0462 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -222,7 +222,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) 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)); + BOOST_CHECK(!CheckFinalTx(tx)); // time locked tx2.vin.resize(1); @@ -236,7 +236,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) 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(!CheckFinalTx(tx2)); BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey)); @@ -248,8 +248,10 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) chainActive.Tip()->nHeight++; SetMockTime(chainActive.Tip()->GetMedianTimePast()+2); - BOOST_CHECK(IsFinalTx(tx, chainActive.Tip()->nHeight + 1)); - BOOST_CHECK(IsFinalTx(tx2)); + // FIXME: we should *actually* create a new block so the following test + // works; CheckFinalTx() isn't fooled by monkey-patching nHeight. + //BOOST_CHECK(CheckFinalTx(tx)); + //BOOST_CHECK(CheckFinalTx(tx2)); BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey)); BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index dd5240e3c..3f55b0427 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -588,7 +588,7 @@ Value getreceivedbyaddress(const Array& params, bool fHelp) for (map::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) { const CWalletTx& wtx = (*it).second; - if (wtx.IsCoinBase() || !IsFinalTx(wtx)) + if (wtx.IsCoinBase() || !CheckFinalTx(wtx)) continue; BOOST_FOREACH(const CTxOut& txout, wtx.vout) @@ -642,7 +642,7 @@ Value getreceivedbyaccount(const Array& params, bool fHelp) for (map::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) { const CWalletTx& wtx = (*it).second; - if (wtx.IsCoinBase() || !IsFinalTx(wtx)) + if (wtx.IsCoinBase() || !CheckFinalTx(wtx)) continue; BOOST_FOREACH(const CTxOut& txout, wtx.vout) @@ -666,7 +666,7 @@ CAmount GetAccountBalance(CWalletDB& walletdb, const string& strAccount, int nMi for (map::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) { const CWalletTx& wtx = (*it).second; - if (!IsFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0 || wtx.GetDepthInMainChain() < 0) + if (!CheckFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0 || wtx.GetDepthInMainChain() < 0) continue; CAmount nReceived, nSent, nFee; @@ -1109,7 +1109,7 @@ Value ListReceived(const Array& params, bool fByAccounts) { const CWalletTx& wtx = (*it).second; - if (wtx.IsCoinBase() || !IsFinalTx(wtx)) + if (wtx.IsCoinBase() || !CheckFinalTx(wtx)) continue; int nDepth = wtx.GetDepthInMainChain(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3396a3a18..94687d289 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1317,7 +1317,7 @@ CAmount CWalletTx::GetChange() const bool CWalletTx::IsTrusted() const { // Quick answer in most cases - if (!IsFinalTx(*this)) + if (!CheckFinalTx(*this)) return false; int nDepth = GetDepthInMainChain(); if (nDepth >= 1) @@ -1423,7 +1423,7 @@ CAmount CWallet::GetUnconfirmedBalance() const for (map::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) { const CWalletTx* pcoin = &(*it).second; - if (!IsFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0)) + if (!CheckFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0)) nTotal += pcoin->GetAvailableCredit(); } } @@ -1468,7 +1468,7 @@ CAmount CWallet::GetUnconfirmedWatchOnlyBalance() const for (map::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) { const CWalletTx* pcoin = &(*it).second; - if (!IsFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0)) + if (!CheckFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0)) nTotal += pcoin->GetAvailableWatchOnlyCredit(); } } @@ -1503,7 +1503,7 @@ void CWallet::AvailableCoins(vector& vCoins, bool fOnlyConfirmed, const const uint256& wtxid = it->first; const CWalletTx* pcoin = &(*it).second; - if (!IsFinalTx(*pcoin)) + if (!CheckFinalTx(*pcoin)) continue; if (fOnlyConfirmed && !pcoin->IsTrusted()) @@ -2290,7 +2290,7 @@ std::map CWallet::GetAddressBalances() { CWalletTx *pcoin = &walletEntry.second; - if (!IsFinalTx(*pcoin) || !pcoin->IsTrusted()) + if (!CheckFinalTx(*pcoin) || !pcoin->IsTrusted()) continue; if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0)