Skip to content

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Sep 15, 2016

After #8594 the addrFrom sent by a node is not used anymore at all, so don't bother sending it (it was already not used if it is an invalid address so this doesn't disadvantage older versions either).

Also mitigates the privacy issue in (#8616). It doesn't completely solve the issue as GetLocalAddress is also called in AdvertiseLocal, but at least when advertising addresses it stands out less as our address.

After bitcoin#8594 the addrFrom sent by a node is not used anymore at all,
so don't bother sending it.

Also mitigates the privacy issue in (bitcoin#8616). It doesn't completely solve
the issue as GetLocalAddress is also called in AdvertiseLocal, but at
least when advertising addresses it stands out less as *our* address.
@laanwj laanwj added the P2P label Sep 15, 2016
Copy link
Member

@sipa sipa left a comment

Choose a reason for hiding this comment

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

utACK. The addrFrom value deserialized from the VERSION message (which this corresponds to) is unused.

Copy link
Contributor

@dcousens dcousens left a comment

Choose a reason for hiding this comment

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

utACK

@paveljanik
Copy link
Contributor

@gmaxwell
Copy link
Contributor

ACK (perhaps backport too)

@laanwj laanwj merged commit d9c99c3 into bitcoin:master Sep 19, 2016
laanwj added a commit that referenced this pull request Sep 19, 2016
d9c99c3 net: No longer send local address in addrMe (Wladimir J. van der Laan)
@maflcko maflcko added this to the 0.13.1 milestone Sep 19, 2016
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Sep 21, 2016
After bitcoin#8594 the addrFrom sent by a node is not used anymore at all,
so don't bother sending it.

Also mitigates the privacy issue in (bitcoin#8616). It doesn't completely solve
the issue as GetLocalAddress is also called in AdvertiseLocal, but at
least when advertising addresses it stands out less as *our* address.

Github-Pull: bitcoin#8740
Rebased-From: d9c99c3
@laanwj
Copy link
Member Author

laanwj commented Sep 26, 2016

This is backported in #8772, removing tag

@sipa sipa mentioned this pull request Jan 10, 2017
18 tasks
pyritepirate referenced this pull request in pyritepirate/pyrite Jan 14, 2019
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 12, 2020
…ak the local address

fa404f1 test: Check that the version message does not leak the local address of the node (MarcoFalke)

Pull request description:

  Add test for bitcoin#8740

ACKs for top commit:
  theStack:
    ACK bitcoin@fa404f1

Tree-SHA512: 4d1c10d1c02fba4b51bd8b9eb3a0d9a682b6aac8c3f6924e295fdca3faefa5ecc3eaa87d347cfec5d2b2bc49963c10fe0a37c463f36088ed0304a2e3716b963b
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 27, 2020
e733939 [Cleanup] Remove CConnman::Copy(Release)NodeVector, now unused (random-zebra)
b09da57 [Refactor] Proper CConnman encapsulation of mnsync (random-zebra)
219061e [Refactor] Decouple SyncWithNode from CMasternodeSync::Process() (random-zebra)
e1f3620 [Refactor] Proper CConnman encapsulation of proposals / votes sync (random-zebra)
f38c9f9 miner: don't try to access connman if it doesn't exist (Fuzzbawls)
92ab619 Address feedback (Fuzzbawls)
9d5346a Do not set an addr time penalty when a peer advertises itself. (Gregory Maxwell)
fe12ff9 net: No longer send local address in addrMe (Wladimir J. van der Laan)
14d6917 net: misc header cleanups (Cory Fields)
34ba0de net: make proxy receives interruptible (Cory Fields)
1bd97ef net: make net processing interruptable (Fuzzbawls)
e24b4cc net: make net interruptible (Cory Fields)
814d0de net: add CThreadInterrupt and InterruptibleSleep (Cory Fields)
1443f2a net: a few small cleanups before replacing boost threads (Cory Fields)
58eabe4 net: move MAX_FEELER_CONNECTIONS into connman (Cory Fields)
874baba Convert ForEachNode* functions to take a templated function argument rather than a std::function to eliminate std::function overhead (Jeremy Rubin)
9485ab0 Made the ForEachNode* functions in src/net.cpp more pragmatic and self documenting (Jeremy Rubin)
c1e59ad minor net cleanups (Fuzzbawls)
07ae004 net: move vNodesDisconnected into CConnman (Cory Fields)
276c946 net: add nSendBufferMaxSize/nReceiveFloodSize to CConnection::Options (Cory Fields)
22a9aff net: Introduce CConnection::Options to avoid passing so many params (Cory Fields)
e4891bf net: Drop StartNode/StopNode and use CConnman directly (Cory Fields)
431575c net: pass CClientUIInterface into CConnman (Cory Fields)
48de47e net: Pass best block known height into CConnman (Cory Fields)
15eed91 net: move max/max-outbound to CConnman (Cory Fields)
2bf0921 net: move semOutbound to CConnman (Cory Fields)
481929f net: move nLocalServices/nRelevantServices to CConnman (Cory Fields)
bcee6ae net: move SendBufferSize/ReceiveFloodSize to CConnman (Cory Fields)
6865469 net: move send/recv statistics to CConnman (Cory Fields)
1cec418 net: SocketSendData returns written size (Cory Fields)
2bb9dfa net: move messageHandlerCondition to CConnman (Cory Fields)
9c5a0df net: move nLocalHostNonce to CConnman (Cory Fields)
a1394ef net: move nLastNodeId to CConnman (Cory Fields)
3804c29 net: move whitelist functions into CConnman (Cory Fields)
dbde9be net: create generic functor accessors and move vNodes to CConnman (Cory Fields)
2e02467 net: Add most functions needed for vNodes to CConnman (Cory Fields)
5667e61 net: move added node functions to CConnman (Cory Fields)
37487ed net: Add oneshot functions to CConnman (Cory Fields)
facf878 net: move ban and addrman functions into CConnman (Cory Fields)
091aaf2 net: handle nodesignals in CConnman (Cory Fields)
1e9fa0f net: move OpenNetworkConnection into CConnman (Cory Fields)
573200f net: Move socket binding into CConnman (Cory Fields)
7762b97 net: Pass CConnection to wallet rather than using the global (Fuzzbawls)
00591b8 net: Pass CConnman around as needed (Cory Fields)
2cd3d39 net: Add rpc error for missing/disabled p2p functionality (Cory Fields)
f08e316 net: Create CConnman to encapsulate p2p connections (Cory Fields)
66337dc net: move CBanDB and CAddrDB out of net.h/cpp (Fuzzbawls)
10969f6 gui: add NodeID to the peer table (Cory Fields)
58044ac Fix some locks (Pieter Wuille)
f9f8926 Do not shadow variables in networking code (Pavel Janík)

Pull request description:

  This is the finalization of the upstream PRs being tracked in #1374 to update much of our networking code to more closely follow upstream improvements. The following PRs are included here:

  - bitcoin#8466
    Do not shadow variables in networking code
  - bitcoin#8606
    Fix some locks
  - bitcoin#8085
    Begin encapsulation
  - bitcoin#9289
    drop boost::thread_group
  - bitcoin#8740
    No longer send local address in addrMe
  - bitcoin#8661
    Do not set an addr time penalty when a peer advertises itself

  Additionally, during conflict resolution and backporting of 8085, the following additional upstream PR was included:
  - bitcoin#8715
    only delete CConnman if it's been created

  Still TODO in future PRs:
  - bitcoin#9037
  - bitcoin#9441
  - bitcoin#9609
  - bitcoin#9626
  - bitcoin#9708
  - bitcoin#12381
  - bitcoin#18584

ACKs for top commit:
  furszy:
    same here, code ACK e733939. Nice work ☕ .
  furszy:
    ACK e733939 .
  random-zebra:
    ACK e733939 and merging...

Tree-SHA512: 0fc3cca76d9ddc13f75fc9d48c48d215e6c0e0381377c0318436176a0e0ead73b511e0061a4b63f7052874730b6da8b0ffc2c94cba034bcc39aad4212b69ee22
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Aug 24, 2021
… does not leak the local address

fa404f1 test: Check that the version message does not leak the local address of the node (MarcoFalke)

Pull request description:

  Add test for bitcoin#8740

ACKs for top commit:
  theStack:
    ACK bitcoin@fa404f1

Tree-SHA512: 4d1c10d1c02fba4b51bd8b9eb3a0d9a682b6aac8c3f6924e295fdca3faefa5ecc3eaa87d347cfec5d2b2bc49963c10fe0a37c463f36088ed0304a2e3716b963b
PastaPastaPasta pushed a commit to dashpay/dash that referenced this pull request Aug 30, 2021
…eak the local address (#4368)

* Backporting Merge bitcoin#18584: test: Check that the version message does not leak the local address

fa404f1 test: Check that the version message does not leak the local address of the node (MarcoFalke)

Pull request description:

  Add test for bitcoin#8740

ACKs for top commit:
  theStack:
    ACK bitcoin@fa404f1

Tree-SHA512: 4d1c10d1c02fba4b51bd8b9eb3a0d9a682b6aac8c3f6924e295fdca3faefa5ecc3eaa87d347cfec5d2b2bc49963c10fe0a37c463f36088ed0304a2e3716b963b

* Merge bitcoin#18584: test: Check that the version message does not leak the local address

Co-authored-by: MarcoFalke <falke.marco@gmail.com>
@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.

6 participants