From ce99358f4aa4182d6983fde3e33a8fdbe1dfe4c3 Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Mon, 14 Jan 2013 16:52:33 -0500 Subject: [PATCH 1/2] Remove IsFromMe() check in CTxMemPool::accept() Fixes issue #2178 : attacker could penny-flood with invalid-signature transactions to deduce which addresses belonged to your node. I'm committing this early for code review; I still need to write up a test plan. Executive summary of fix: check all transactions received from the network for penny-flood rate-limiting before adding to the memory pool. But do NOT ratelimit transactions added to the memory pool: - because of blockchain reorgs - stored in the wallet and added at startup - sent from the GUI or one of the send* RPC commands (CWallet::CommitTransaction) The limit-free-transactions code really should be a method on CNode, with counters per-peer. But that is a bigger change for another day. --- src/main.cpp | 53 +++++++++++++++++++-------------------- src/main.h | 6 ++--- src/rpcrawtransaction.cpp | 2 +- src/wallet.cpp | 2 +- 4 files changed, 31 insertions(+), 32 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index cb7975af0..f0e7ac137 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -627,7 +627,7 @@ void CTxMemPool::pruneSpent(const uint256 &hashTx, CCoins &coins) } } -bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, +bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, bool fLimitFree, bool* pfMissingInputs) { if (pfMissingInputs) @@ -733,7 +733,7 @@ bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, // Don't accept it if it can't get into a block int64 txMinFee = tx.GetMinFee(1000, true, GMF_RELAY); - if (nFees < txMinFee) + if (fLimitFree && nFees < txMinFee) return error("CTxMemPool::accept() : not enough fees %s, %"PRI64d" < %"PRI64d, hash.ToString().c_str(), nFees, txMinFee); @@ -741,25 +741,24 @@ bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, // Continuously rate-limit free transactions // This mitigates 'penny-flooding' -- sending thousands of free transactions just to // be annoying or make others' transactions take longer to confirm. - if (nFees < MIN_RELAY_TX_FEE) + if (fLimitFree && nFees < MIN_RELAY_TX_FEE) { - static CCriticalSection cs; static double dFreeCount; static int64 nLastTime; int64 nNow = GetTime(); - { - // Use an exponentially decaying ~10-minute window: - dFreeCount *= pow(1.0 - 1.0/600.0, (double)(nNow - nLastTime)); - nLastTime = nNow; - // -limitfreerelay unit is thousand-bytes-per-minute - // At default rate it would take over a month to fill 1GB - if (dFreeCount > GetArg("-limitfreerelay", 15)*10*1000 && !IsFromMe(tx)) - return error("CTxMemPool::accept() : free transaction rejected by rate limiter"); - if (fDebug) - printf("Rate limit dFreeCount: %g => %g\n", dFreeCount, dFreeCount+nSize); - dFreeCount += nSize; - } + LOCK(cs); + + // Use an exponentially decaying ~10-minute window: + dFreeCount *= pow(1.0 - 1.0/600.0, (double)(nNow - nLastTime)); + nLastTime = nNow; + // -limitfreerelay unit is thousand-bytes-per-minute + // At default rate it would take over a month to fill 1GB + if (dFreeCount > GetArg("-limitfreerelay", 15)*10*1000) + return error("CTxMemPool::accept() : free transaction rejected by rate limiter"); + if (fDebug) + printf("Rate limit dFreeCount: %g => %g\n", dFreeCount, dFreeCount+nSize); + dFreeCount += nSize; } // Check against previous transactions @@ -792,9 +791,9 @@ bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, return true; } -bool CTransaction::AcceptToMemoryPool(bool fCheckInputs, bool* pfMissingInputs) +bool CTransaction::AcceptToMemoryPool(bool fCheckInputs, bool fLimitFree, bool* pfMissingInputs) { - return mempool.accept(*this, fCheckInputs, pfMissingInputs); + return mempool.accept(*this, fCheckInputs, fLimitFree, pfMissingInputs); } bool CTxMemPool::addUnchecked(const uint256& hash, CTransaction &tx) @@ -905,9 +904,9 @@ int CMerkleTx::GetBlocksToMaturity() const } -bool CMerkleTx::AcceptToMemoryPool(bool fCheckInputs) +bool CMerkleTx::AcceptToMemoryPool(bool fCheckInputs, bool fLimitFree) { - return CTransaction::AcceptToMemoryPool(fCheckInputs); + return CTransaction::AcceptToMemoryPool(fCheckInputs, fLimitFree); } @@ -923,10 +922,10 @@ bool CWalletTx::AcceptWalletTransaction(bool fCheckInputs) { uint256 hash = tx.GetHash(); if (!mempool.exists(hash) && pcoinsTip->HaveCoins(hash)) - tx.AcceptToMemoryPool(fCheckInputs); + tx.AcceptToMemoryPool(fCheckInputs, false); } } - return AcceptToMemoryPool(fCheckInputs); + return AcceptToMemoryPool(fCheckInputs, false); } return false; } @@ -1797,7 +1796,7 @@ bool SetBestChain(CBlockIndex* pindexNew) // Resurrect memory transactions that were in the disconnected branch BOOST_FOREACH(CTransaction& tx, vResurrect) - tx.AcceptToMemoryPool(); + tx.AcceptToMemoryPool(true, false); // Delete redundant memory transactions that are in the connected branch BOOST_FOREACH(CTransaction& tx, vDelete) { @@ -3181,7 +3180,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) pfrom->AddInventoryKnown(inv); bool fMissingInputs = false; - if (tx.AcceptToMemoryPool(true, &fMissingInputs)) + if (tx.AcceptToMemoryPool(true, true, &fMissingInputs)) { SyncWithWallets(inv.hash, tx, NULL, true); RelayMessage(inv, vMsg); @@ -3203,7 +3202,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) CInv inv(MSG_TX, tx.GetHash()); bool fMissingInputs2 = false; - if (tx.AcceptToMemoryPool(true, &fMissingInputs2)) + if (tx.AcceptToMemoryPool(true, true, &fMissingInputs2)) { printf(" accepted orphan tx %s\n", inv.hash.ToString().substr(0,10).c_str()); SyncWithWallets(inv.hash, tx, NULL, true); @@ -3214,9 +3213,9 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) } else if (!fMissingInputs2) { - // invalid orphan + // invalid or too-little-fee orphan vEraseQueue.push_back(inv.hash); - printf(" removed invalid orphan tx %s\n", inv.hash.ToString().substr(0,10).c_str()); + printf(" removed orphan tx %s\n", inv.hash.ToString().substr(0,10).c_str()); } } } diff --git a/src/main.h b/src/main.h index 3290b1631..44fa5c1de 100644 --- a/src/main.h +++ b/src/main.h @@ -649,7 +649,7 @@ public: bool CheckTransaction() const; // Try to accept this transaction into the memory pool - bool AcceptToMemoryPool(bool fCheckInputs=true, bool* pfMissingInputs=NULL); + bool AcceptToMemoryPool(bool fCheckInputs=true, bool fLimitFree = true, bool* pfMissingInputs=NULL); protected: static const CTxOut &GetOutputFor(const CTxIn& input, CCoinsViewCache& mapInputs); @@ -1103,7 +1103,7 @@ public: int GetDepthInMainChain() const { CBlockIndex *pindexRet; return GetDepthInMainChain(pindexRet); } bool IsInMainChain() const { return GetDepthInMainChain() > 0; } int GetBlocksToMaturity() const; - bool AcceptToMemoryPool(bool fCheckInputs=true); + bool AcceptToMemoryPool(bool fCheckInputs=true, bool fLimitFree=true); }; @@ -1882,7 +1882,7 @@ public: std::map mapTx; std::map mapNextTx; - bool accept(CTransaction &tx, bool fCheckInputs, bool* pfMissingInputs); + bool accept(CTransaction &tx, bool fCheckInputs, bool fLimitFree, bool* pfMissingInputs); bool addUnchecked(const uint256& hash, CTransaction &tx); bool remove(const CTransaction &tx, bool fRecursive = false); bool removeConflicts(const CTransaction &tx); diff --git a/src/rpcrawtransaction.cpp b/src/rpcrawtransaction.cpp index 9531b1267..8117a1ff4 100644 --- a/src/rpcrawtransaction.cpp +++ b/src/rpcrawtransaction.cpp @@ -546,7 +546,7 @@ Value sendrawtransaction(const Array& params, bool fHelp) fHave = view.GetCoins(hashTx, existingCoins); if (!fHave) { // push to local node - if (!tx.AcceptToMemoryPool()) + if (!tx.AcceptToMemoryPool(true, false)) throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX rejected"); } } diff --git a/src/wallet.cpp b/src/wallet.cpp index 37b86c35b..0a320b6f8 100644 --- a/src/wallet.cpp +++ b/src/wallet.cpp @@ -1279,7 +1279,7 @@ bool CWallet::CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey) mapRequestCount[wtxNew.GetHash()] = 0; // Broadcast - if (!wtxNew.AcceptToMemoryPool()) + if (!wtxNew.AcceptToMemoryPool(true, false)) { // This must not fail. The transaction has already been signed and recorded. printf("CommitTransaction() : Error: Transaction not valid"); From 9c9f5c1303dff0c010e9e68ba7b5619330edfb68 Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Wed, 23 Jan 2013 20:24:10 -0500 Subject: [PATCH 2/2] Let limitfreerelay=0 reject ALL free transactions --- src/main.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index f0e7ac137..e519332da 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -754,7 +754,7 @@ bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, bool fLimitFree, nLastTime = nNow; // -limitfreerelay unit is thousand-bytes-per-minute // At default rate it would take over a month to fill 1GB - if (dFreeCount > GetArg("-limitfreerelay", 15)*10*1000) + if (dFreeCount >= GetArg("-limitfreerelay", 15)*10*1000) return error("CTxMemPool::accept() : free transaction rejected by rate limiter"); if (fDebug) printf("Rate limit dFreeCount: %g => %g\n", dFreeCount, dFreeCount+nSize);