Merge pull request #6183

28bf062 Fix off-by-one error w/ nLockTime in the wallet (Peter Todd)
This commit is contained in:
Wladimir J. van der Laan 2015-06-01 11:30:20 +02:00
commit 87550eefc1
No known key found for this signature in database
GPG Key ID: 74810B012346C9A6
8 changed files with 39 additions and 38 deletions

View File

@ -671,14 +671,8 @@ bool IsStandardTx(const CTransaction& tx, string& reason)
bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime) 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) if (tx.nLockTime == 0)
return true; 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)) if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))
return true; return true;
BOOST_FOREACH(const CTxIn& txin, tx.vin) BOOST_FOREACH(const CTxIn& txin, tx.vin)
@ -687,6 +681,12 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
return true; return true;
} }
bool CheckFinalTx(const CTransaction &tx)
{
AssertLockHeld(cs_main);
return IsFinalTx(tx, chainActive.Height() + 1, GetAdjustedTime());
}
/** /**
* Check transaction inputs to mitigate two * Check transaction inputs to mitigate two
* potential denial-of-service attacks: * potential denial-of-service attacks:
@ -903,21 +903,8 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
// Only accept nLockTime-using transactions that can be mined in the next // 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 // block; we don't want our mempool filled up with transactions that can't
// be mined yet. // be mined yet.
// if (!CheckFinalTx(tx))
// However, IsFinalTx() is confusing... Without arguments, it uses return state.DoS(0, error("AcceptToMemoryPool: non-final"),
// 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"),
REJECT_NONSTANDARD, "non-final"); REJECT_NONSTANDARD, "non-final");
// is it already in the memory pool? // is it already in the memory pool?

View File

@ -324,7 +324,18 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state);
*/ */
bool IsStandardTx(const CTransaction& tx, std::string& reason); 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 * Closure representing one script verification

View File

@ -138,6 +138,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
LOCK2(cs_main, mempool.cs); LOCK2(cs_main, mempool.cs);
CBlockIndex* pindexPrev = chainActive.Tip(); CBlockIndex* pindexPrev = chainActive.Tip();
const int nHeight = pindexPrev->nHeight + 1; const int nHeight = pindexPrev->nHeight + 1;
pblock->nTime = GetAdjustedTime();
CCoinsViewCache view(pcoinsTip); CCoinsViewCache view(pcoinsTip);
// Priority order to process transactions // Priority order to process transactions
@ -152,7 +153,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, nHeight)) if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight, pblock->nTime))
continue; continue;
COrphan* porphan = NULL; COrphan* porphan = NULL;

View File

@ -26,7 +26,7 @@ using namespace std;
QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx) QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx)
{ {
AssertLockHeld(cs_main); AssertLockHeld(cs_main);
if (!IsFinalTx(wtx, chainActive.Height() + 1)) if (!CheckFinalTx(wtx))
{ {
if (wtx.nLockTime < LOCKTIME_THRESHOLD) if (wtx.nLockTime < LOCKTIME_THRESHOLD)
return tr("Open for %n more block(s)", "", wtx.nLockTime - chainActive.Height()); return tr("Open for %n more block(s)", "", wtx.nLockTime - chainActive.Height());

View File

@ -188,7 +188,7 @@ 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, chainActive.Height() + 1)) if (!CheckFinalTx(wtx))
{ {
if (wtx.nLockTime < LOCKTIME_THRESHOLD) if (wtx.nLockTime < LOCKTIME_THRESHOLD)
{ {

View File

@ -223,7 +223,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
tx.nLockTime = chainActive.Tip()->nHeight+1; tx.nLockTime = chainActive.Tip()->nHeight+1;
hash = tx.GetHash(); hash = tx.GetHash();
mempool.addUnchecked(hash, CTxMemPoolEntry(tx, 11, GetTime(), 111.0, 11)); mempool.addUnchecked(hash, CTxMemPoolEntry(tx, 11, GetTime(), 111.0, 11));
BOOST_CHECK(!IsFinalTx(tx, chainActive.Tip()->nHeight + 1)); BOOST_CHECK(!CheckFinalTx(tx));
// time locked // time locked
tx2.vin.resize(1); tx2.vin.resize(1);
@ -237,7 +237,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
tx2.nLockTime = chainActive.Tip()->GetMedianTimePast()+1; tx2.nLockTime = chainActive.Tip()->GetMedianTimePast()+1;
hash = tx2.GetHash(); hash = tx2.GetHash();
mempool.addUnchecked(hash, CTxMemPoolEntry(tx2, 11, GetTime(), 111.0, 11)); mempool.addUnchecked(hash, CTxMemPoolEntry(tx2, 11, GetTime(), 111.0, 11));
BOOST_CHECK(!IsFinalTx(tx2)); BOOST_CHECK(!CheckFinalTx(tx2));
BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey)); BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey));
@ -249,8 +249,10 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
chainActive.Tip()->nHeight++; chainActive.Tip()->nHeight++;
SetMockTime(chainActive.Tip()->GetMedianTimePast()+2); SetMockTime(chainActive.Tip()->GetMedianTimePast()+2);
BOOST_CHECK(IsFinalTx(tx, chainActive.Tip()->nHeight + 1)); // FIXME: we should *actually* create a new block so the following test
BOOST_CHECK(IsFinalTx(tx2)); // works; CheckFinalTx() isn't fooled by monkey-patching nHeight.
//BOOST_CHECK(CheckFinalTx(tx));
//BOOST_CHECK(CheckFinalTx(tx2));
BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey)); BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey));
BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3); BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3);

View File

@ -588,7 +588,7 @@ Value getreceivedbyaddress(const Array& params, bool fHelp)
for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it)
{ {
const CWalletTx& wtx = (*it).second; const CWalletTx& wtx = (*it).second;
if (wtx.IsCoinBase() || !IsFinalTx(wtx)) if (wtx.IsCoinBase() || !CheckFinalTx(wtx))
continue; continue;
BOOST_FOREACH(const CTxOut& txout, wtx.vout) BOOST_FOREACH(const CTxOut& txout, wtx.vout)
@ -642,7 +642,7 @@ Value getreceivedbyaccount(const Array& params, bool fHelp)
for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it)
{ {
const CWalletTx& wtx = (*it).second; const CWalletTx& wtx = (*it).second;
if (wtx.IsCoinBase() || !IsFinalTx(wtx)) if (wtx.IsCoinBase() || !CheckFinalTx(wtx))
continue; continue;
BOOST_FOREACH(const CTxOut& txout, wtx.vout) BOOST_FOREACH(const CTxOut& txout, wtx.vout)
@ -666,7 +666,7 @@ CAmount GetAccountBalance(CWalletDB& walletdb, const string& strAccount, int nMi
for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it) for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin(); it != pwalletMain->mapWallet.end(); ++it)
{ {
const CWalletTx& wtx = (*it).second; const CWalletTx& wtx = (*it).second;
if (!IsFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0 || wtx.GetDepthInMainChain() < 0) if (!CheckFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0 || wtx.GetDepthInMainChain() < 0)
continue; continue;
CAmount nReceived, nSent, nFee; CAmount nReceived, nSent, nFee;
@ -1109,7 +1109,7 @@ Value ListReceived(const Array& params, bool fByAccounts)
{ {
const CWalletTx& wtx = (*it).second; const CWalletTx& wtx = (*it).second;
if (wtx.IsCoinBase() || !IsFinalTx(wtx)) if (wtx.IsCoinBase() || !CheckFinalTx(wtx))
continue; continue;
int nDepth = wtx.GetDepthInMainChain(); int nDepth = wtx.GetDepthInMainChain();

View File

@ -1318,7 +1318,7 @@ CAmount CWalletTx::GetChange() const
bool CWalletTx::IsTrusted() const bool CWalletTx::IsTrusted() const
{ {
// Quick answer in most cases // Quick answer in most cases
if (!IsFinalTx(*this)) if (!CheckFinalTx(*this))
return false; return false;
int nDepth = GetDepthInMainChain(); int nDepth = GetDepthInMainChain();
if (nDepth >= 1) if (nDepth >= 1)
@ -1424,7 +1424,7 @@ CAmount CWallet::GetUnconfirmedBalance() const
for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
{ {
const CWalletTx* pcoin = &(*it).second; const CWalletTx* pcoin = &(*it).second;
if (!IsFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0)) if (!CheckFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0))
nTotal += pcoin->GetAvailableCredit(); nTotal += pcoin->GetAvailableCredit();
} }
} }
@ -1469,7 +1469,7 @@ CAmount CWallet::GetUnconfirmedWatchOnlyBalance() const
for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
{ {
const CWalletTx* pcoin = &(*it).second; const CWalletTx* pcoin = &(*it).second;
if (!IsFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0)) if (!CheckFinalTx(*pcoin) || (!pcoin->IsTrusted() && pcoin->GetDepthInMainChain() == 0))
nTotal += pcoin->GetAvailableWatchOnlyCredit(); nTotal += pcoin->GetAvailableWatchOnlyCredit();
} }
} }
@ -1504,7 +1504,7 @@ void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const
const uint256& wtxid = it->first; const uint256& wtxid = it->first;
const CWalletTx* pcoin = &(*it).second; const CWalletTx* pcoin = &(*it).second;
if (!IsFinalTx(*pcoin)) if (!CheckFinalTx(*pcoin))
continue; continue;
if (fOnlyConfirmed && !pcoin->IsTrusted()) if (fOnlyConfirmed && !pcoin->IsTrusted())
@ -2291,7 +2291,7 @@ std::map<CTxDestination, CAmount> CWallet::GetAddressBalances()
{ {
CWalletTx *pcoin = &walletEntry.second; CWalletTx *pcoin = &walletEntry.second;
if (!IsFinalTx(*pcoin) || !pcoin->IsTrusted()) if (!CheckFinalTx(*pcoin) || !pcoin->IsTrusted())
continue; continue;
if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0) if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0)