Bisq's bitcoinj "addr" message support audit

[oscarguindzberg]

• Commits to audit
  • 659f3c1
• How it works now
  • PeerGroup: When "addr" msg is received, add a new peer to the "inactives" collection.
  • PeerGroup property addPeersFromAddressMessage can be used to disable the feature.
  • bisq/bitcoinj node never sends “getaddr” msgs.
  • bisq/bitcoinj node receives from time to time unsolicited “addr” msgs from peers.
  • bisq sets addPeersFromAddressMessage to false (disables the feature) if preferences.getBitcoinNodes() is not empty.
• upstream bitcoinj support for addr/getaddr messages
  • upstream bitcoinj does not implement this feature
    • Peer class can send "getaddr" msg and process incoming "addr" msg, but the feature is not used.
    • This is a missing feature on bitcoinj.
  • Related work on upstream bitcoinj
    • https://groups.google.com/d/msg/bitcoinj/3Y93LIDfJaM/SOYnpM9hlkYJ
    • addrman-based PeerDBDiscovery bitcoinj/bitcoinj#2
  • New conversation I started: https://groups.google.com/forum/#!topic/bitcoinj/W3gkXT7Hk2M
• Bitcoin core support: https://en.bitcoin.it/wiki/Satoshi_Client_Node_Discovery
• Comments by Dan (commit author)
  • I modified bitcoinj to listen to the addr p2p messages and add nodes dynamically. I was rather surprised mainline bitcoinj didn't already do that since it is standard bitcoind functionality.
  • This was in context of enabling bitcoinj p2p traffic to work over Tor, including DNS lookups which was a particularly tricky aspect.
  • btc nodes come and go, so hard-coded seeds become unavailable over time. thus unreliable.
    • Oscar comment: it's true, but same non critical issue is present on every app that uses bitcoinj.
  • DNS provided nodes are a tiny subset of total bitcoin p2p network, and using only them is a point of centralization. dns maintainers become gatekeepers.
    • Oscar comment: but same non critical issue is present on every app that uses bitcoinj.
  • DNS over Tor is slow and some record-sets from bitcoin DNS servers are too large, which result in empty result set. (if memory serves, sipa's dns node was one such.) In prior testing, we would sometimes have issues with few connections being made to p2p network, and then it never corrects itself back up to the target 8 or 10. Using the addr message fixed that issue as I recall.
    • Oscar comment: Dan has a point here. Since Bisq uses by default the curated btc nodes harcoded in the bisq code instead of dns discovery, this is not as critical as it could be.
  • Bitcoin DNS servers do not provide .onion addresses. So addr messages are best way to become informed of nodes running behind Tor.
    • Oscar comment: Why is important to connect to btc nodes running behind tor?.
  • It is a way to bootstrap p2p connections from a local peer, eg bitcoind on same machine or subnet, even if DNS is being flaky/slow over Tor.
    • Oscar comment: If a user is connected to a local bitcoind, she probably does not want to connect to any other bitcoin node for privacy/trust issues.
  • It's a part of the bitcoin p2p protocol. Not implementing it in bitcoinj seems incomplete at best.
    • Oscar comment: agree.
  • It was not hard to do. low hanging fruit.
    • Oscar comment: See "Implementation problems/limitations" section.
• Implementation problems/limitations.
  • If a user wants to connect just to the Bisq curated node list in BtcNodes.java for privacy issues, this mechanism might have her connected to other nodes. This could probably be solved by improving when PeerGroup.setAddPeersFromAddressMessage() is set to true.
  • getaddr messages are never sent, so bitcoinj depends on the unsolicited "addr" msgs peers send from time to time.
  • An attacker could send a million "addr" msgs with fake peers causing a DoS on the bitcoinj node.
  • Received addresses are not persisted.
  • Full support of peer discovery using getaddr/addr msgs might be difficult to implement. See upstream bitcoinj attempts to do it detailed above.
• Actions:
  • Disable "add peers from addr message" custom bitcoinj feature when Bisq curated node list in BtcNodes.java is used: Disable "add peers from addr message" custom bitcoinj feature when a … bisq#2591
  • Add a link to this audit in bitcoinj source code: Add comment explaining "addr" message handling. #29