-
Notifications
You must be signed in to change notification settings - Fork 37.8k
Defer inserting into maprelay until just before relaying. #8082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
pto->filterInventoryKnown.insert(hash); | ||
vInv.push_back(inv); | ||
{ | ||
LOCK(cs_mapRelay); | ||
if (mapRelay.find(inv.hash) == mapRelay.end()) { |
There was a problem hiding this comment.
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));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Looks like comparison tool triggers a |
@MarcoFalke Yep. It was an actual inversion with the vSend lock, it's fixed. |
Concept ACK Nit: After this mapRelay is no longer used in net at all. It could become local to main. |
@laanwj good catch with the nit, I've removed it from net.cpp/net.h. |
utACK d9d1f2e |
ut ACK d9d1f2e |
@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? |
@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. |
Can one of the admins verify this patch? |
@gmaxwell I guess that can use some comment in the code? |
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.
Rebased. |
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. |
4d8993b Defer inserting into maprelay until just before relaying. (Gregory Maxwell)
…elaying. 4d8993b Defer inserting into maprelay until just before relaying. (Gregory Maxwell)
…elaying. 4d8993b Defer inserting into maprelay until just before relaying. (Gregory Maxwell)
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
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.