Skip to content

Conversation

gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented May 21, 2016

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.

pto->filterInventoryKnown.insert(hash);
vInv.push_back(inv);
{
LOCK(cs_mapRelay);
if (mapRelay.find(inv.hash) == mapRelay.end()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@maflcko
Copy link
Member

maflcko commented May 21, 2016

Looks like comparison tool triggers a POTENTIAL DEADLOCK DETECTED

@laanwj laanwj added the P2P label May 21, 2016
@gmaxwell
Copy link
Contributor Author

@MarcoFalke Yep. It was an actual inversion with the vSend lock, it's fixed.

@laanwj
Copy link
Member

laanwj commented May 22, 2016

Concept ACK

Nit: After this mapRelay is no longer used in net at all. It could become local to main.

@gmaxwell
Copy link
Contributor Author

gmaxwell commented May 22, 2016

@laanwj good catch with the nit, I've removed it from net.cpp/net.h.

@sipa
Copy link
Member

sipa commented May 24, 2016

utACK d9d1f2e

@sipa sipa mentioned this pull request May 24, 2016
@theuni
Copy link
Member

theuni commented May 24, 2016

ut ACK d9d1f2e

@paveljanik
Copy link
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
Copy link
Contributor Author

@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
Copy link
Contributor

arowser commented May 25, 2016

Can one of the admins verify this patch?

@sipa
Copy link
Member

sipa commented May 25, 2016

@gmaxwell I guess that can use some comment in the code?

@sipa sipa mentioned this pull request May 26, 2016
@gmaxwell
Copy link
Contributor Author

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.

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.
@gmaxwell
Copy link
Contributor Author

Rebased.

@sipa
Copy link
Member

sipa commented May 31, 2016

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 sipa merged commit 4d8993b into bitcoin:master Jun 1, 2016
sipa added a commit that referenced this pull request Jun 1, 2016
4d8993b Defer inserting into maprelay until just before relaying. (Gregory Maxwell)
@sipa sipa mentioned this pull request Jun 13, 2016
7 tasks
codablock pushed a commit to codablock/dash that referenced this pull request Dec 22, 2017
…elaying.

4d8993b Defer inserting into maprelay until just before relaying. (Gregory Maxwell)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
…elaying.

4d8993b Defer inserting into maprelay until just before relaying. (Gregory Maxwell)
zkbot added a commit to zcash/zcash that referenced this pull request Aug 17, 2021
ZIP 239 preparations 3

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#8080
- bitcoin/bitcoin#8082
- bitcoin/bitcoin#8126
- bitcoin/bitcoin#7910
  - This is the unsquashed version of bitcoin/bitcoin#8149
  - We take three cleanup commits to the protocol / `CInv` code.
- bitcoin/bitcoin#8822
- bitcoin/bitcoin#8880
  - Excluding the first commit (we don't have the comment it fixes yet).
- bitcoin/bitcoin#19322
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants