Skip to content
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

[tests] Test that nodes respond to getdata with notfound #14571

Merged
merged 1 commit into from Oct 27, 2018

Conversation

Projects
None yet
6 participants
@MarcoFalke
Copy link
Member

commented Oct 25, 2018

If a node has not announced a tx at all, then it should respond to
getdata messages for that tx with notfound, to avoid leaking tx
origination privacy.

In the future this could be adjusted such that a node responds with
notfound when a tx has not been announced to us, but that seems
to be a more involved change. See e.g.
https://github.com/jnewbery/bitcoin/commits/pr14220.1

@MarcoFalke MarcoFalke added the Tests label Oct 25, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-qaNotfound branch Oct 25, 2018

@jnewbery

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Concept ACK. Great test.

test/functional/test_framework/messages.py Outdated
if vec is None:
self.vec = []
else:
self.vec = vec

This comment has been minimized.

Copy link
@jamesob

jamesob Oct 25, 2018

Member

nit: more concise (if not exactly equivalent) as self.vec = vec or []

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 25, 2018

Member

I think Marco's version is clearer.

Show resolved Hide resolved test/functional/p2p_leak_tx.py
test/functional/p2p_leak_tx.py Outdated
def run_test(self):
gen_node = self.nodes[0] # The block and tx generating node
gen_node.generate(1)
self.sync_all()

This comment has been minimized.

Copy link
@sanket1729

sanket1729 Oct 25, 2018

Contributor

I am new here, but is sync_all() necessary when there is only 1 node?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 25, 2018

Member

Entirely correct! This can be removed

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-qaNotfound branch Oct 26, 2018

test/functional/test_framework/messages.py Outdated
@@ -1232,6 +1232,23 @@ def __repr__(self):
return "msg_mempool()"


class msg_notfound:
__slots__ = ("vec")

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 26, 2018

Member

This should be __slots__ = ("vec",) to force tuple instead of str :-)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 26, 2018

Author Member

Ugh, I always get this wrong. Thx

[tests] Test that nodes respond to getdata with notfound
If a node has not announced a tx at all, then it should respond to
getdata messages for that tx with notfound, to avoid leaking tx
origination privacy.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1810-qaNotfound branch to fa78a2f Oct 26, 2018

@jnewbery

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

Tested ACK fa78a2f

1 similar comment
@sanket1729

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Tested ACK fa78a2f

@naumenkogs

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Tested ACK fa78a2f

@MarcoFalke MarcoFalke merged commit fa78a2f into bitcoin:master Oct 27, 2018

1 of 2 checks passed

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

MarcoFalke added a commit that referenced this pull request Oct 27, 2018

Merge #14571: [tests] Test that nodes respond to getdata with notfound
fa78a2f [tests] Test that nodes respond to getdata with notfound (MarcoFalke)

Pull request description:

  If a node has not announced a tx at all, then it should respond to
  getdata messages for that tx with notfound, to avoid leaking tx
  origination privacy.

  In the future this could be adjusted such that a node responds with
  notfound when a tx has not been announced to us, but that seems
  to be a more involved change. See e.g.
  https://github.com/jnewbery/bitcoin/commits/pr14220.1

Tree-SHA512: 6244afa5bd5d8fec9b89dfc02c9958bc370195145a0f3715f33200d6cf73a376c94193d44bf4523867196e6591c53ede8f9b6a77cb296b48c114a117b8c8b1fa

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1810-qaNotfound branch Oct 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.