Defer inserting into maprelay until just before relaying. #8082

pull gmaxwell wants to merge 1 commits into bitcoin:master from gmaxwell:just_in_time_maprelay changing 3 files +30 −31

gmaxwell commented at 9:48 AM on May 21, 2016: contributor

This reduces the rate of not founds by better matching the far end expectations, it also improves privacy by removing the ability to use getdata to probe for a node having a txn before it has been relayed.

in src/main.cpp:None in ef5854122b outdated

5900 | +                    if (pto->pfilter && !pto->pfilter->IsRelevantAndUpdate(tx)) continue;
5901 |                      pto->filterInventoryKnown.insert(hash);
5902 |                      vInv.push_back(inv);
5903 | +                    {
5904 | +                        LOCK(cs_mapRelay);
5905 | +                        if (mapRelay.find(inv.hash) == mapRelay.end()) {

sipa commented at 9:52 AM on May 21, 2016:

More efficient:

auto ret = mapRelay.insert(std::make_pair(inv.hash, tx));
if (ret.second) {
    vRelayExpiration.push_back(std::make_pair(GetTime() + 16 * 60, inv.hash));
}

gmaxwell commented at 10:04 AM on May 21, 2016:

Done.

MarcoFalke commented at 11:07 AM on May 21, 2016: member

Looks like comparison tool triggers a POTENTIAL DEADLOCK DETECTED
laanwj added the label P2P on May 21, 2016
gmaxwell commented at 1:27 PM on May 21, 2016: contributor

@MarcoFalke Yep. It was an actual inversion with the vSend lock, it's fixed.
laanwj commented at 11:14 AM on May 22, 2016: member

Concept ACK

Nit: After this mapRelay is no longer used in net at all. It could become local to main.
gmaxwell commented at 10:35 PM on May 22, 2016: contributor

@laanwj good catch with the nit, I've removed it from net.cpp/net.h.
sipa commented at 4:22 PM on May 24, 2016: member

utACK d9d1f2ec345b3d7ac37ce194032940b513b5e3f0
sipa cross-referenced this on May 24, 2016 from issue p2p: Begin encapsulation by theuni
theuni commented at 7:41 PM on May 24, 2016: member

ut ACK d9d1f2ec345b3d7ac37ce194032940b513b5e3f0
paveljanik commented at 8:33 PM on May 24, 2016: contributor

@gmaxwell Can you please be more explicit in the commit message? In "This reduces the rate of not founds", "This" means "this PR" or you mean the 15-> 16 minutes change (why not separate commit, BTW)?

What is the logic behind 15 -> 16 anyway?
gmaxwell commented at 9:27 PM on May 24, 2016: contributor

@paveljanik "better matching far end expectations"-- the far end retries on a two minute interval; 15 minutes is dead between counts-- starting the counter before the transaction has been offered to anyone also makes it more likely to time out first.
arowser commented at 8:43 AM on May 25, 2016: contributor

Can one of the admins verify this patch?
sipa commented at 5:24 PM on May 25, 2016: member

@gmaxwell I guess that can use some comment in the code?
sipa cross-referenced this on May 26, 2016 from issue Addrman offline attempts by gmaxwell
gmaxwell commented at 11:25 AM on May 28, 2016: contributor

I changed it back to 15 minutes, -- I think the time there should be adjusted but it can be done in another pull that reworks the mapaskfor handling a bit.

Defer inserting into maprelay until just before relaying.

This reduces the rate of not founds by better matching the far
 end expectations, it also improves privacy by removing the
 ability to use getdata to probe for a node having a txn before
 it has been relayed.

4d8993b346

gmaxwell commented at 3:46 PM on May 31, 2016: contributor

Rebased.
sipa commented at 5:34 PM on May 31, 2016: member

Lightly tested ACK. Setup: two mainnet full nodes with this patch (A publicly reachable, B -connect'ed to A) and a lightweight node C (connected to the public network). Tested block synchronization/relay of B from A, relay of transactions to from A to B, relay of newly created transactions by B and C through A. Nothing unusual.
sipa cross-referenced this on May 31, 2016 from issue std::shared_ptr based CTransaction storage in mempool by sipa
sipa merged this on Jun 1, 2016
sipa closed this on Jun 1, 2016
sipa referenced this in commit 01d8359983 on Jun 1, 2016
MarcoFalke cross-referenced this on Jun 2, 2016 from issue Potential deadlock assertion in net code on 0.12RC3 by GIJensen
sipa cross-referenced this on Jun 13, 2016 from issue Segregated witness by sipa
OlegGirko cross-referenced this on Jul 18, 2017 from issue Backport Bitcoin PR#8085: p2p: Begin encapsulation by OlegGirko
codablock referenced this in commit cfe6493630 on Sep 16, 2017
codablock referenced this in commit 005111b608 on Sep 19, 2017
codablock referenced this in commit 51fa05ac33 on Dec 22, 2017
andvgal referenced this in commit 9be6b85a52 on Jan 6, 2019
sdaftuar cross-referenced this on Jan 22, 2020 from issue Use rolling bloom filter of recent block txs for AlreadyHave() check by sdaftuar
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)

#7470 Potential deadlock assertion in net code on 0.12RC3 #7910 Segregated witness #8065 Addrman offline attempts #8085 p2p: Begin encapsulation #8126 std::shared_ptr based CTransaction storage in mempool #17951 Use rolling bloom filter of recent block txs for AlreadyHave() check