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

Only use AddInventoryKnown for transactions #7960

Merged
merged 1 commit into from Jun 1, 2016

Conversation

Projects
None yet
6 participants
@sdaftuar
Member

sdaftuar commented Apr 27, 2016

filterInventoryKnown is only used when relaying transactions, so stop adding block hashes to the filter.

Also updates a comment that is no longer correct.

Only use AddInventoryKnown for transactions
filterInventoryKnown is only used when relaying transactions,
so stop adding block hashes to the filter.

@MarcoFalke MarcoFalke added the P2P label Apr 27, 2016

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Apr 27, 2016

Member

May as well skip in the fBlocksOnly case too?

Member

theuni commented Apr 27, 2016

May as well skip in the fBlocksOnly case too?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 28, 2016

Member

May as well skip in the fBlocksOnly case too?

Agree, seems to be no reason to add it to AddInventoryKnown in that case.

Member

laanwj commented Apr 28, 2016

May as well skip in the fBlocksOnly case too?

Agree, seems to be no reason to add it to AddInventoryKnown in that case.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 28, 2016

Member

Concept ACK

Member

laanwj commented Apr 28, 2016

Concept ACK

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Apr 28, 2016

Member

Yeah, I think that makes sense. I thought at first that maybe we'd want filterInventoryKnown to be as correct as possible even in the fBlocksOnly case, since we might still relay transactions to that peer (eg if we have a whitelisted peer and -whitelistrelay is on), but given that it's a protocol violation this seems like a silly thing to worry about.

I was thinking we could probably use a p2p regression test that exercises filterInventoryKnown, as I don't believe any of our tests would fail if it were broken?

I'll plan to update this PR at some point with a test and the change for the fBlocksOnly case.

Member

sdaftuar commented Apr 28, 2016

Yeah, I think that makes sense. I thought at first that maybe we'd want filterInventoryKnown to be as correct as possible even in the fBlocksOnly case, since we might still relay transactions to that peer (eg if we have a whitelisted peer and -whitelistrelay is on), but given that it's a protocol violation this seems like a silly thing to worry about.

I was thinking we could probably use a p2p regression test that exercises filterInventoryKnown, as I don't believe any of our tests would fail if it were broken?

I'll plan to update this PR at some point with a test and the change for the fBlocksOnly case.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 5, 2016

Member

utACK 383fc10

Member

sipa commented May 5, 2016

utACK 383fc10

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 17, 2016

Member

utACK.

I think blocks only will later be generalized to more degrees than just literally blocks only yes or no, and we probably shouldn't go around adding too much specialization in the codebase for a global blocks only mode unless it's a pretty big win. (and maybe saving the memory of ever allocating some transaction related filters in the first place might be... but insertions? not so much)

Member

gmaxwell commented May 17, 2016

utACK.

I think blocks only will later be generalized to more degrees than just literally blocks only yes or no, and we probably shouldn't go around adding too much specialization in the codebase for a global blocks only mode unless it's a pretty big win. (and maybe saving the memory of ever allocating some transaction related filters in the first place might be... but insertions? not so much)

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 18, 2016

Member

Ok I'll leave this alone then.

Member

sdaftuar commented May 18, 2016

Ok I'll leave this alone then.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 25, 2016

Member

Given that the scope of AddInventoryKnown changes, perhaps it should be renamed to AddInventoryTxKnown, and only take a uint256 rather than a CInv? That may reduce confusion in upcoming refactors.

Member

sipa commented May 25, 2016

Given that the scope of AddInventoryKnown changes, perhaps it should be renamed to AddInventoryTxKnown, and only take a uint256 rather than a CInv? That may reduce confusion in upcoming refactors.

@sipa sipa referenced this pull request May 26, 2016

Merged

Addrman offline attempts #8065

@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 383fc10 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 #7960: Only use AddInventoryKnown for transactions
383fc10 Only use AddInventoryKnown for transactions (Suhas Daftuar)

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

Merge #7960: Only use AddInventoryKnown for transactions
383fc10 Only use AddInventoryKnown for transactions (Suhas Daftuar)

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

Merge #7960: Only use AddInventoryKnown for transactions
383fc10 Only use AddInventoryKnown for transactions (Suhas Daftuar)

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

Merge #7960: Only use AddInventoryKnown for transactions
383fc10 Only use AddInventoryKnown for transactions (Suhas Daftuar)

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

Remove unnecessary call to AddInventoryKnown in INV message handling
This should have been part of Bitcoin #7960 but was missed in merge
conflict resolution.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment