Skip to content

Conversation

@sipa
Copy link
Member

@sipa sipa commented Oct 25, 2021

#23306 effectively changed the on-disk format in an incompatible way: old deserializers cannot deal with multiple entries for the same IP.

Introduce a V4_MULTIPORT format, and increment the compatibility base, so that old versions correctly recognize it as an incompatible future version, rather than corruption.

@laanwj
Copy link
Member

laanwj commented Oct 25, 2021

Concept ACK

@DrahtBot DrahtBot added the P2P label Oct 25, 2021
92617b7 effectively changed the
on-disk format in an incompatible way: old deserializers cannot
deal with multiple entries for the same IP.

Introduce a V4_MULTIPORT format, and increment the compatibility base,
so that old versions correctly recognize it as an incompatible future
version.
@sipa sipa force-pushed the 202110_v4addrman branch from 080a6aa to d891ae7 Compare October 25, 2021 17:48
@naumenkogs
Copy link
Contributor

ACK d891ae7

@ajtowns
Copy link
Contributor

ajtowns commented Oct 26, 2021

utACK d891ae7

@maflcko
Copy link
Member

maflcko commented Oct 26, 2021

I tested the "correct" error message (failing to load because of version). Testing the "wrong" error message (corruption) might be harder, as it requires -checkaddrman (or the implicit check), both of which are only in master, thus require re-compilation. So I think a regression-test with previous releases is not possible, at least not in the functional test framework.

So I tested the new format manually:

$ ./src/bitcoin-cli -regtest -datadir=/tmp/test_23354/ addpeeraddress "250.1.1.2" 12345 
{
  "success": true
}
$ ./src/bitcoin-cli -regtest -datadir=/tmp/test_23354/ addpeeraddress "250.1.1.2" 12367 
{
  "success": true
}

Start with previous versions prints the correct error now:

Bitcoin Core version v22.0.0 (release build)
...
ERROR: DeserializeDB: Deserialize or I/O error - Unsupported format of addrman database: 4. It is compatible with formats >=4, but the maximum supported by this version of Bitcoin Core is 3.: iostream error
Recreating peers.dat

or going back even further:

Bitcoin Core version v0.20.1 (release build)
...
ERROR: DeserializeDB: Deserialize or I/O error - Incorrect keysize in addrman deserialization: iostream error
Invalid or missing peers.dat; recreating

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK d891ae7

I tested #23306 which is essentially the same:

  1. Start latest code (with and without this PR) and use addpeeraddress RPC to create peers.dat with duplicates (either V3 or V4)
  2. Recompile 92617b7~ and start it on the new peers.dat

It works as expected.

@jnewbery
Copy link
Contributor

Concept ACK

@maflcko maflcko merged commit 5574881 into bitcoin:master Oct 29, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 29, 2021
d891ae7 Introduce new V4 format addrman (Pieter Wuille)

Pull request description:

  bitcoin#23306 effectively changed the on-disk format in an incompatible way: old deserializers cannot deal with multiple entries for the same IP.

  Introduce a `V4_MULTIPORT` format, and increment the compatibility base, so that old versions correctly recognize it as an incompatible future version, rather than corruption.

ACKs for top commit:
  naumenkogs:
    ACK d891ae7
  ajtowns:
    utACK d891ae7
  vasild:
    ACK d891ae7

Tree-SHA512: de2153beb59152504ee0656dd0cc0b879b09136eb07e3ce0426d2fea778adfabacebbce5cf1a9a65dc99ad4e99cda42ab26743fe672fb82a9fbfec49c4cccb4d
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 21, 2022
Summary:
```

For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that.

The folklore justification (eventually actually added as a comment to the codebase in #20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. It appears, at least the current network scale - and probably significantly larger - that the impact is very low at best (see calculations by vasild in #5150 (comment) e.g.). Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports). This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups).

And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.

One obstacle in moving away from a default port that is the fact that addrman is currently restricted to a single entry per IP address. If ports are no longer expected to be generally always the default one, we need to deal with the case where conflicting information is relayed. It turns out there is a very natural solution to this: treat (IP,port) combination exactly as we're treating IPs now; this automatically means that the same IP may appear with multiple ports, simply because those would be distinct entries. Given that indexing into addrman's bucket already uses the port number, the only change required is making all addrman lookup be (IP,port) (aka CService) based, rather than IP (aka CNetAddr) based.

This PR doesn't include any change to the actual outbound connection preference logic, as perhaps that's something that we want to phase in more gradually.
```

Backport of [[bitcoin/bitcoin#23306 | core#23306]] and  [[bitcoin/bitcoin#23354 | core#23354]] .

Depends on D12340.

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D12342
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
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.

8 participants