Remove IsFromMe() check in CTxMemPool::accept() #2182

1. 
   gavinandresen commented at 10:10 PM on January 14, 2013: contributor

   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.

2. 
   SergioDemianLerner commented at 3:16 AM on January 15, 2013: contributor

   Line 3205 of main.cpp: if (tx.AcceptToMemoryPool(true, false, &fMissingInputs2)) does´t look good. You are skipping any fee check for orphans to enter the memory pool. Then an attacker can easily penny-flood by creating a huge orphan transaction (with a single missing parent), and then releasing the parent transaction (that will pass the rate limit test because it is very small).

   It would be better to do: if (tx.AcceptToMemoryPool(true, true, &fMissingInputs2)).

   To keep a clear meaning, dFreeCount should be understood as the maximum OUTPUT bytes/second a peer can request from me, instead as maximum INPUT bytes/second a peer can send.

3. 
   luke-jr commented at 4:00 AM on January 15, 2013: member

   You can't fee-check orphan transactions: without the parent, the value of the inputs are unknown.

4. 
   SergioDemianLerner commented at 1:48 PM on January 15, 2013: contributor

   When (tx.AcceptToMemoryPool(true, xxx, &fMissingInputs2)) is executed, then it means that at least one of their inputs have been received, so it may be complete. So it is correct to try to check the fees at that time.

5. 
   SergioDemianLerner commented at 1:49 PM on January 15, 2013: contributor

   Look at the code surrounding line 3205.

6. 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.

   ce99358f4a
7. 
   gavinandresen commented at 2:11 PM on January 15, 2013: contributor

   Sergio's correct, if we get a transaction that fulfills an orphan's missing inputs, we should treat the orphan as if we just received it over the wire.

   Changed the true to a false, and fixed the comment and message for the "orphan now has all inputs, but is rejected by the free transaction ratelimiter" else clause.

8. 
   gavinandresen commented at 9:03 PM on January 23, 2013: contributor

   Revised test plan:

   On PRE-PATCH bitcoind:

   Run testnet-in-a-box setup, with 'bc1' and 'bc2' nodes, both with -limitfreerelay=0

   send a zero-fee transaction from bc2, sending coins to bc1 EXPECT: transaction ends up in bc1's memory pool (use getrawmempool to verify)

   Apply this patch, re-run test: EXPECT: transaction does not end up in bc1's memory pool or in bc1's listtransactions

9. Let limitfreerelay=0 reject ALL free transactions 9c9f5c1303
10. 
    gavinandresen commented at 1:26 AM on January 24, 2013: contributor

    Results of testing successful, but I did have to tweak the free-transaction code so limitfreerelay=0 did what I wanted it to do (it was letting the first free transaction into the memory pool).

11. 
    BitcoinPullTester commented at 5:13 AM on January 24, 2013: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/9c9f5c1303dff0c010e9e68ba7b5619330edfb68 for binaries and test log.

12. gavinandresen referenced this in commit 434fa60d75 on Jan 26, 2013
13. gavinandresen merged this on Jan 26, 2013
14. gavinandresen closed this on Jan 26, 2013

    ---

15. laudney referenced this in commit f10dbd7051 on Mar 19, 2014
16. DrahtBot locked this on Sep 8, 2021

Milestone
0.8.0