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

Defer inserting into maprelay until just before relaying. #8082

Merged
merged 1 commit into from Jun 1, 2016

Conversation

Projects
None yet
7 participants
@gmaxwell
Member

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.

@sipa

View changes

Show outdated Hide outdated src/main.cpp
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 21, 2016

Member

Looks like comparison tool triggers a POTENTIAL DEADLOCK DETECTED

Member

MarcoFalke commented May 21, 2016

Looks like comparison tool triggers a POTENTIAL DEADLOCK DETECTED

@laanwj laanwj added the P2P label May 21, 2016

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 21, 2016

Member

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

Member

gmaxwell commented May 21, 2016

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 22, 2016

Member

Concept ACK

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

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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 22, 2016

Member

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

Member

gmaxwell commented May 22, 2016

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 24, 2016

Member

utACK d9d1f2e

Member

sipa commented May 24, 2016

utACK d9d1f2e

@sipa sipa referenced this pull request May 24, 2016

Merged

p2p: Begin encapsulation #8085

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni May 24, 2016

Member

ut ACK d9d1f2e

Member

theuni commented May 24, 2016

ut ACK d9d1f2e

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik 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?

Contributor

paveljanik commented May 24, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 24, 2016

Member

@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.

Member

gmaxwell commented May 24, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@arowser

arowser May 25, 2016

Contributor

Can one of the admins verify this patch?

Contributor

arowser commented May 25, 2016

Can one of the admins verify this patch?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 25, 2016

Member

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

Member

sipa commented May 25, 2016

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

@sipa sipa referenced this pull request May 26, 2016

Merged

Addrman offline attempts #8065

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 28, 2016

Member

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.

Member

gmaxwell commented May 28, 2016

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.
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 31, 2016

Member

Rebased.

Member

gmaxwell commented May 31, 2016

Rebased.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa 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.

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Jun 1, 2016

Merge #8082: Defer inserting into maprelay until just before relaying.
4d8993b Defer inserting into maprelay until just before relaying. (Gregory Maxwell)

@sipa sipa referenced this pull request Jun 13, 2016

Closed

Segregated witness #7910

5 of 7 tasks complete

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #8082: Defer inserting into maprelay until just before relaying.
4d8993b Defer inserting into maprelay until just before relaying. (Gregory Maxwell)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8082: Defer inserting into maprelay until just before relaying.
4d8993b Defer inserting into maprelay until just before relaying. (Gregory Maxwell)

codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017

Merge #8082: Defer inserting into maprelay until just before relaying.
4d8993b Defer inserting into maprelay until just before relaying. (Gregory Maxwell)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment