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

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

laanwj merged this on May 31, 2016

laanwj closed this on May 31, 2016

laanwj referenced this in commit 862fd24b40 on May 31, 2016

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

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 referenced this in commit 1f010c0969 on Jan 23, 2021

zkbot referenced this in commit 1d7ed06174 on Aug 13, 2021

zkbot referenced this in commit 56b5f95897 on Aug 17, 2021

MarcoFalke locked this on Sep 8, 2021