Do not use mempool for GETDATA for tx accepted after the last mempool req. #8080

pull gmaxwell wants to merge 1 commits into bitcoin:master from gmaxwell:nonmempool_getdata changing 5 files +19 −2

gmaxwell commented at 5:54 AM on May 21, 2016: contributor

The ability to GETDATA a transaction which has not (yet) been relayed is a privacy loss vector.

The use of the mempool for this was added as part of the mempool p2p message and is only needed to fetch transactions returned by it.
jonasschnelli added the label P2P on May 21, 2016
jonasschnelli added the label Mempool on May 21, 2016

in src/main.cpp:None in ef6c31142e outdated

4502 | @@ -4503,9 +4503,12 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam
4503 |                  }
4504 |                  if (!pushed && inv.type == MSG_TX) {
4505 |                      CTransaction tx;
4506 | -                    if (mempool.lookup(inv.hash, tx)) {
4507 | -                        pfrom->PushMessage(NetMsgType::TX, tx);
4508 | -                        pushed = true;
4509 | +                    int64_t txtime;
4510 | +                    if (mempool.lookup(inv.hash, tx, txtime)) {
4511 | +                        if (txtime <= pfrom->timeLastMempool) {

jonasschnelli commented at 5:58 AM on May 21, 2016:

wouldn't if (mempool.lookup(inv.hash, tx, textile) && txtime <= pfrom->timeLastMempool) { also work?

gmaxwell commented at 6:12 AM on May 21, 2016:

Indeed, current layout is an artifact of my local copy having an else for logging.

jonasschnelli commented at 6:01 AM on May 21, 2016: contributor

utACK ef6c31142e52e56af93486986319f5499e72c7df
sipa commented at 6:52 AM on May 21, 2016: member

utACK ef6c31142e52e56af93486986319f5499e72c7df
gmaxwell commented at 5:34 AM on May 23, 2016: contributor

Changed the variable name and added a comment in response to Petertodd's comments, also avoided the nested if per jonasschnelli's comments.
petertodd commented at 8:28 PM on May 23, 2016: contributor

utACK https://github.com/bitcoin/bitcoin/pull/8080/commits/8e9890f4ecbc08676d66479ed473d06036ca82be

in src/net.h:None in 8e9890f4ec outdated

 413 | @@ -413,6 +414,8 @@ class CNode
 414 |      // Used for BIP35 mempool sending, also protected by cs_inventory
 415 |      bool fSendMempool;
 416 |  
 417 | +    // Last time a mempool

paveljanik commented at 5:22 AM on May 24, 2016:

Please extend the comment here, e.g. Last time of mempool BIP35 request?

in src/txmempool.cpp:None in 8e9890f4ec outdated

 797 | @@ -798,6 +798,16 @@ bool CTxMemPool::lookup(uint256 hash, CTransaction& result) const
 798 |      return true;
 799 |  }
 800 |  
 801 | +bool CTxMemPool::lookup(uint256 hash, CTransaction& result, int64_t& time) const

paveljanik commented at 5:28 AM on May 24, 2016:

Have you considered adding int64_t& time to the previous lookup? Both functions are almost the same...

paveljanik commented at 5:29 AM on May 24, 2016: contributor

Concept ACK.
arowser commented at 8:43 AM on May 25, 2016: contributor

Can one of the admins verify this patch?

Do not use mempool for GETDATA for tx accepted after the last mempool req.

The ability to GETDATA a transaction which has not (yet) been relayed
 is a privacy loss vector.

The use of the mempool for this was added as part of the mempool p2p
 message and is only needed to fetch transactions returned by it.

7e908c7b82

sipa cross-referenced this on May 26, 2016 from issue Addrman offline attempts by gmaxwell
laanwj merged this on May 31, 2016
laanwj closed this on May 31, 2016
laanwj referenced this in commit 862fd24b40 on May 31, 2016
sipa cross-referenced this on May 31, 2016 from issue std::shared_ptr based CTransaction storage in mempool by sipa
sipa cross-referenced this on Jun 13, 2016 from issue Segregated witness by sipa
codablock referenced this in commit 06ed9c7726 on Sep 16, 2017
codablock referenced this in commit f476adc2f8 on Sep 19, 2017
codablock referenced this in commit dfa0bd2341 on Dec 22, 2017
codablock referenced this in commit 8a97d7a7fd on Feb 7, 2018
codablock cross-referenced this on Feb 7, 2018 from issue Update timeLastMempoolReq when responding to MEMPOOL request by codablock
UdjinM6 referenced this in commit 120893c63d on Feb 8, 2018
andvgal referenced this in commit d503ee3976 on Jan 6, 2019
andvgal referenced this in commit 1c3d1b7791 on Jan 6, 2019
CryptoCentric referenced this in commit 887c3672aa on Feb 28, 2019
CryptoCentric referenced this in commit b93921164c on Mar 2, 2019
furszy cross-referenced this on Dec 26, 2020 from issue Up to CTransactionRef point back ports to do list. by furszy
furszy cross-referenced this on Dec 31, 2020 from issue [NET] Inv, mempool and validation improvements by furszy
furszy referenced this in commit 1f010c0969 on Jan 23, 2021
str4d cross-referenced this on Aug 13, 2021 from issue ZIP 239 preparations 3 by str4d
zkbot referenced this in commit 1d7ed06174 on Aug 13, 2021
zkbot referenced this in commit 56b5f95897 on Aug 17, 2021
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7910 Segregated witness #8065 Addrman offline attempts #8126 std::shared_ptr based CTransaction storage in mempool