Commit graph

131 commits

Author SHA1 Message Date
John Newbery
10aac24145 [tests] Make deterministic addrman use nKey = 1
addrman_tests fail when consistency checks are enabled, since the tests
set the deterministic test addrman's nKey value to zero, which is an
invalid value. Change this so that deterministic addrman's nKey value is
set to 1.

This requires updating a few tests that are using magic values derived
from nKey being set to 0.
2021-08-05 17:10:32 +01:00
John Newbery
fa9710f62c [addrman] Add deterministic argument to CAddrMan ctor
Removes the need for tests to update nKey and insecure_rand after constructing
a CAddrMan.
2021-08-05 17:10:30 +01:00
MarcoFalke
ee458d84fc Add missing const to CAddrMan::Check_()
Also: Always compile the function signature to avoid similar issues in
the future.
2021-08-05 15:24:12 +01:00
MarcoFalke
d67330d112
Merge bitcoin/bitcoin#21129: fuzz: check that ser+unser produces the same AddrMan
87651795d8 fuzz: check that ser+unser produces the same AddrMan (Vasil Dimov)
6408b24517 fuzz: move init code to the CAddrManDeterministic constructor (Vasil Dimov)

Pull request description:

  Add a fuzz test that fills addrman with a pile of randomly generated addresses, serializes it to a stream, unserializes the stream to another addrman object and compares the two.

  Some discussion of this already happened at https://github.com/jnewbery/bitcoin/pull/18.

ACKs for top commit:
  practicalswift:
    cr ACK 87651795d8
  jonatack:
    ACK 87651795d8 rebased to current master, reviewed, fuzz build, ran `FUZZ=addrman_serdeser src/test/fuzz/fuzz`

Tree-SHA512: 7eda79279f14f2649840bf752e575d7b02cbaad541f74f7254855ebd4a32da988f042d78aa9228983350283bb74dd0c71f51f04c0846889c3ba2f19f01a0c303
2021-08-05 15:17:50 +02:00
Vasil Dimov
87651795d8
fuzz: check that ser+unser produces the same AddrMan 2021-08-04 18:22:08 +02:00
fanquake
3308c61091
Merge bitcoin/bitcoin#22576: doc: Update high-level addrman description
036d7eadf5 doc: Correct description of CAddrMan::Create() (Amiti Uttarwar)
318176aff1 doc: Update high-level addrman description (Martin Zumsande)

Pull request description:

  The high-level description of `addrman` has outdated information with respect to the eviction behavior, both for the New and Tried tables (at least since #5941) - this has confused me in the past.

  This PR corrects this and also adds basic info about the bucket size and position.

ACKs for top commit:
  amitiuttarwar:
    reACK 036d7eadf5
  jnewbery:
    ACK 036d7eadf5

Tree-SHA512: 3f0635d765f5e580a1fae31187742a833cef66ef2286d40eeb28f2253521260038e16e5f1a65741464a2ddfdbeb5c0f1bc38bf73841e600639033d59c3c534e4
2021-08-04 10:53:08 +08:00
fanquake
5cf28d5203
Merge bitcoin/bitcoin#22496: addrman: Remove addrman hotfixes
65332b1178 [addrman] Remove RemoveInvalid() (John Newbery)

Pull request description:

  PRs #22179 and #22112 (EDIT: later reverted in #22497) added hotfix code to addrman to remove invalid addresses and mutate the ports of I2P entries after entering into addrman. Those hotfixes included at least two addrman data corruption bugs:

  - #22467 (Assertion `nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size()' failed)
  - #22470 (Changing I2P ports in addrman may wronly skip some entries from "new" buckets)

  Hotfixing addrman is inherently dangerous. There are many members that have implicit assumptions on each others' state, and mutating those directly can lead to violating addrman's internal invariants.

  Instead of trying to hotfix addrman, just don't insert any invalid addresses. For now, those are addresses which fail `CNetAddr::IsValid()`.

ACKs for top commit:
  sipa:
    utACK 65332b1178. I tried to reason through scenarios that could introduce inconsistencies with this code, but can't find any.
  fanquake:
    ACK 65332b1178 - Skipping the addition of invalid addresses (this code was initially added for Tor addrs) rather than adding all the invalids then removing them all when finishing unserializing seems like an improvement. Especially if it can be achieved with less code.

Tree-SHA512: 023113764cb475572f15da7bf9824b62b79e10a7e359af2eee59017df354348d2aeed88de0fd4ad7a9f89a0dad10827f99d70af6f1cb20abb0eca2714689c8d7
2021-08-03 15:40:32 +08:00
Amiti Uttarwar
036d7eadf5 doc: Correct description of CAddrMan::Create()
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
2021-08-02 22:34:30 +02:00
Martin Zumsande
318176aff1 doc: Update high-level addrman description 2021-08-02 21:24:12 +02:00
MarcoFalke
fa32024d51
Add missing GUARDED_BY to CAddrMan::insecure_rand 2021-07-23 11:31:05 +02:00
MarcoFalke
fae0c79351
refactor: Mark CAddrMan::GetAddr const 2021-07-21 16:01:14 +02:00
MarcoFalke
fa02934c8c
refactor: Mark CAddrMan::Select const 2021-07-21 16:01:09 +02:00
John Newbery
f036dfbb69 [addrman] Remove unused test_before_evict argument from Good()
This has never been used in the public interface method since it was
introduced in #9037.
2021-07-20 16:17:51 +01:00
John Newbery
65332b1178 [addrman] Remove RemoveInvalid()
Instead of deserializing addresses, placing them in the buckets, and
then removing them if they're invalid, check first and don't place in
the buckets if they're invalid.
2021-07-20 11:45:52 +01:00
fanquake
624a193330
Merge bitcoin/bitcoin#22497: scripted-diff: remove ResetI2PPorts() (revert e0a2b390c1)
d4b67c8ebc scripted-diff: remove ResetI2PPorts() (revert e0a2b390c1) (Vasil Dimov)

Pull request description:

  `CAddrMan::ResetI2PPorts()` was temporary. Remove it:
  * it has partially achieved its goal: probably ran on about half of the
    I2P nodes
  * it is hackish, deemed risky and two bugs where found in it:
    https://github.com/bitcoin/bitcoin/issues/22467
    https://github.com/bitcoin/bitcoin/issues/22470

  -BEGIN VERIFY SCRIPT-
  git show e0a2b390c1 |git apply -R
  -END VERIFY SCRIPT-

  Fixes https://github.com/bitcoin/bitcoin/issues/22467
  Fixes https://github.com/bitcoin/bitcoin/issues/22470

ACKs for top commit:
  laanwj:
    ACK d4b67c8ebc
  MarcoFalke:
    review ACK d4b67c8ebc 😲
  jonatack:
    ACK d4b67c8ebc per IRC discussions https://www.erisian.com.au/bitcoin-core-dev/log-2021-07-16.html#l-212 and https://www.erisian.com.au/bitcoin-core-dev/log-2021-07-19.html#l-210

Tree-SHA512: 60d8f0ea0f66a8fcedfcb9c8944a419b974b15509b54ddfeec58db49ae9418e6916df712bba3fbd6b29497d85f7951fb9aa2e48eb9c59f88d09435685bd00b4c
2021-07-20 09:02:34 +08:00
Vasil Dimov
d4b67c8ebc
scripted-diff: remove ResetI2PPorts() (revert e0a2b390c1)
`CAddrMan::ResetI2PPorts()` was temporary. Remove it:
* it has partially achieved its goal: probably ran on about half of the
  I2P nodes
* it is hackish, deemed risky and two bugs where found in it
  https://github.com/bitcoin/bitcoin/issues/22467
  https://github.com/bitcoin/bitcoin/issues/22470

-BEGIN VERIFY SCRIPT-
git show e0a2b390c1 |git apply -R
-END VERIFY SCRIPT-

Fixes https://github.com/bitcoin/bitcoin/issues/22467
Fixes https://github.com/bitcoin/bitcoin/issues/22470
2021-07-19 14:33:21 +02:00
Vasil Dimov
816f29eab2
addrman: detect on-disk corrupted nNew and nTried during unserialization
Negative `nNew` or `nTried` are not possible during normal operation.
So, if we read such values during unserialize, report addrman
corruption.

Fixes https://github.com/bitcoin/bitcoin/issues/22450
2021-07-15 13:40:29 +02:00
Vasil Dimov
e0a2b390c1
addrman: reset I2P ports to 0 when loading from disk
This is a temporary change to convert I2P addresses that have propagated
with port 8333 to ones with port 0.

It would cause a problem some day if indeed some bitcoin software is
listening on port 8333 only and rejects connections to port 0 and we are
still using SAM 3.1 which only supports port 0. In this case we would
replace 8333 with 0 and try to connect to such nodes.

This commit should be included in 22.0 and be reverted before 23.0 is
released.
2021-07-09 11:19:38 +02:00
W. J. van der Laan
d968616422
Merge bitcoin/bitcoin#22179: Torv2 removal followups
00b875ba94 addrman: remove invalid addresses when unserializing (Vasil Dimov)
bdb62096f0 fuzz: reduce possible networks check (Vasil Dimov)
a164cd3ba6 net: simplify CNetAddr::IsRoutable() (Vasil Dimov)

Pull request description:

  * Simplify some code, now that we know `CNetAddr::IsRFC4193()` and `CNetAddr::IsTor()` cannot be `true` at the same time.
  * Drop Tor v2 addresses when loading addrman from `peers.dat` - they would have been loaded as dummy-all-zeros IPv6 addresses and linger in addrman, wasting space.

ACKs for top commit:
  sipa:
    ACK 00b875ba94. Reviewed the code, and tested with -DDEBUG_ADDRMAN (unit tests + mainnet run with peers.dat that contained v2 onions).
  laanwj:
    Code review and lightly tested ACK 00b875ba94
  jonatack:
    ACK 00b875ba94 reviewed, debug-built with -DEBUG_ADDRMAN rebased to current master, restarted node on mainnet/signet/testnet and verified that on each chain -addrinfo shows no change in address counts (as expected). Added some sanity check asserts, rebuilt/re-ran test. Checked that the new test fails on master with "test/addrman_tests.cpp(824): error: in "addrman_tests/remove_invalid": check addrman.size() == 2 has failed [4 != 2]"
  jarolrod:
    ACK 00b875ba94

Tree-SHA512: 6ed8e6745134b1b94fffaba28482de909ea39483b46b7f57bda61cdbae7a51251d15cb674de3631772fbeabe153d77a19269f96e62a89102a2d5c01e48f0ba06
2021-07-08 17:20:35 +02:00
Hennadii Stepanov
ae98aec9c0
refactor: Make CAddrMan::cs non-recursive 2021-06-14 17:28:38 +03:00
Hennadii Stepanov
5ef1d0b698
Add thread safety annotations to CAddrMan public functions 2021-06-14 17:28:38 +03:00
Hennadii Stepanov
b138973a8b
refactor: Avoid recursive locking in CAddrMan::Clear
Co-authored-by: John Newbery <john@johnnewbery.com>
2021-06-14 17:28:37 +03:00
Hennadii Stepanov
f79a664314
refactor: Apply consistent pattern for CAddrMan::Check usage
Co-authored-by: John Newbery <john@johnnewbery.com>
2021-06-14 17:28:37 +03:00
Hennadii Stepanov
187b7d2bb3
refactor: Avoid recursive locking in CAddrMan::Check 2021-06-14 17:28:37 +03:00
Hennadii Stepanov
f77d9c79aa
refactor: Fix CAddrMan::Check style
This change improves readability, and follows Developer Notes.
2021-06-14 17:28:36 +03:00
Hennadii Stepanov
06703973c7
Make CAddrMan::Check private
Change in the addrman.h header is move-only.
2021-06-14 17:28:30 +03:00
Vasil Dimov
00b875ba94
addrman: remove invalid addresses when unserializing
The Tor v2 addresses, left over from when Tor v2 was supported will be
unserialized as a dummy, invalid `::` (all zeros) IPv6 address. Remove
them so that they do not take up space in addrman.
2021-06-07 14:42:11 +02:00
Vasil Dimov
a92485b2c2
addrman: use unordered_map instead of map
`CAddrMan` uses `std::map` internally even though it does not require
that the map's elements are sorted. `std::map`'s access time is
`O(log(map size))`. `std::unordered_map` is more suitable as it has a
`O(1)` access time.

This patch lowers the execution times of `CAddrMan`'s methods as follows
(as per `src/bench/addrman.cpp`):

```
AddrMan::Add(): -3.5%
AddrMan::GetAddr(): -76%
AddrMan::Good(): -0.38%
AddrMan::Select(): -45%
```
2021-05-28 16:40:15 +02:00
Hennadii Stepanov
8caf60dbbe
move-only: Group and re-order CAddrMan members by access type
Easy to verify with `git diff --color-moved=dimmed-zebra`.
2021-05-24 19:26:49 +03:00
Hennadii Stepanov
5cd7f8abe3
refactor: Do not expose CAddrMan members as protected without need 2021-05-23 14:46:21 +03:00
Jon Atack
a49f3ddbba
p2p: allow CAddrMan::GetAddr() by network, add doxygen 2021-05-19 13:04:11 +02:00
Jon Atack
d35ddca91e
p2p: enable CAddrMan::GetAddr_() by network, add doxygen 2021-05-19 13:04:07 +02:00
John Newbery
4676a4fb5b [addrman] Don't repeat "Bucketing method was updated" log multiple times
Thanks to Vasil Dimov <vd@FreeBSD.org> for these suggestions
2021-01-29 12:39:55 +00:00
John Newbery
436292367c [addrman] Improve serialization comments
Thanks to Vasil Dimov <vd@FreeBSD.org> for these suggestions
2021-01-29 12:39:55 +00:00
John Newbery
ac3547eddd [addrman] Improve variable naming/code style of touched code. 2021-01-18 13:23:16 +00:00
John Newbery
a5c9b04959 [addrman] Don't rebucket new table entries unnecessarily
Only rebucket if the asmap checksum has changed, not if the file format
has changed but no asmap is provided.

Also, don't try to add an entry to another bucket if it already appears
in ADDRMAN_NEW_BUCKETS_PER_ADDRESS buckets.
2021-01-18 13:23:16 +00:00
John Newbery
8062d928ce [addrman] Rename asmap version to asmap checksum
Version implies that higher numbers take precendence. This is really a
checksum, to check whether the provided asmap is the same as the one
used when the peers.dat file was serialized.

Also update the comments to explain where/why this is used.
2021-01-18 13:23:16 +00:00
John Newbery
009b8e0fdf [addrman] Improve variable naming/code style of touched code. 2021-01-18 13:23:16 +00:00
John Newbery
b4c5fda417 [addrman] Fix new table bucketing during unserialization
An addrman entry can appear in up to 8 new table buckets. We store this
entry->bucket indexing during shutdown so that on restart we can restore
the entries to their correct buckets.

Commit ec45646de9 broke the
deserialization code so that each entry could only be put in up to one
new bucket. Fix that.
2021-01-18 13:23:16 +00:00
MarcoFalke
884bde510e
Merge #20291: [net] Consolidate logic around calling CAddrMan::Connected()
0bfce9dc46 [addrman] Fix Connected() comment (John Newbery)
eefe194718 [net] Consolidate logic around calling CAddrMan::Connected() (John Newbery)

Pull request description:

  Currently, the logic around whether we called CAddrMan::Connected() for
  a peer is spread between verack processing (where we discard inbound
  peers) and FinalizeNode (where we discard misbehaving and
  block-relay-only peers). Consolidate that logic to a single place.

  Also remove the CNode.fCurrentlyConnected bool, which is now
  redundant. We can rely on CNode.fSuccessfullyConnected, since the two
  bools were only ever flipped to true in the same place.

ACKs for top commit:
  mzumsande:
    Code review ACK 0bfce9dc46
  amitiuttarwar:
    code review ACK 0bfce9dc46. nice tidy, and bonus that we get to remove an unnecessary call to `cs_main`

Tree-SHA512: 1ab74dae3bc12a6846da57c853033e546bb4f91caa39f4c50bf0cf7eca59cb917bdb2ef795da55363e7e9f70949cb28bb3be004cb3afa4389f970d2fe097d932
2020-11-19 16:30:54 +01:00
Vasil Dimov
38ada892ed
addrman: ensure old versions don't parse peers.dat
Even though the format of `peers.dat` was changed in an incompatible
way (old software versions <0.21 cannot understand the new file format),
it is not guaranteed that old versions will fail to parse it. There is a
chance that old versions parse its contents as garbage and use it.

Old versions expect the "key size" field to be 32 and fail the parsing
if it is not. Thus, we put something other than 32 in it. This will make
versions between 0.11.0 and 0.20.1 deterministically fail on the new
format. Versions prior to https://github.com/bitcoin/bitcoin/pull/5941
(<0.11.0) will still parse it as garbage.

Also, introduce a way to increment the `peers.dat` format in a way that
does not necessary make older versions refuse to read it.
2020-11-11 16:05:15 +01:00
John Newbery
0bfce9dc46 [addrman] Fix Connected() comment 2020-11-04 11:41:17 +00:00
Vasil Dimov
201a4596d9
net: CAddress & CAddrMan: (un)serialize as ADDRv2
Change the serialization of `CAddrMan` to serialize its addresses
in ADDRv2/BIP155 format by default. Introduce a new `CAddrMan` format
version (3).

Add support for ADDRv2 format in `CAddress` (un)serialization.

Co-authored-by: Carl Dong <contact@carldong.me>
2020-10-09 16:42:49 +02:00
John Newbery
f26502e9fc [addrman] Specify max addresses and pct when calling GetAddresses()
CAddrMan.GetAddr() would previously limit the number and percentage of
addresses returned (to ADDRMAN_GETADDR_MAX (1000) and
ADDRMAN_GETADDR_MAX_PCT (23) respectively). Instead, make it the callers
responsibility to specify the maximum addresses and percentage they want
returned.

For net_processing, the maximums are MAX_ADDR_TO_SEND (1000) and
MAX_PCT_ADDR_TO_SEND (23). For rpc/net, the maximum is specified by the
client.
2020-08-12 09:22:07 +01:00
Gleb Naumenko
7cc0e8101f Remove useless 2500 limit on AddrMan queries 2020-07-24 18:02:20 +03:00
Jon Atack
819fb5549b
logging: asmap logging and #include fixups
- move asmap #includes to sorted positions in addrman and init (move-only)

- remove redundant quotes in asmap InitError, update test

- remove full stops from asmap logging to be consistent with debug logging,
  update tests
2020-03-04 14:24:19 +01:00
Wladimir J. van der Laan
01fc5891fb
Merge #16702: p2p: supplying and using asmap to improve IP bucketing in addrman
3c1bc40205 Add extra logging of asmap use and bucketing (Gleb Naumenko)
e4658aa8ea Return mapped AS in RPC call getpeerinfo (Gleb Naumenko)
ec45646de9 Integrate ASN bucketing in Addrman and add tests (Gleb Naumenko)
8feb4e4b66  Add asmap utility which queries a mapping (Gleb Naumenko)

Pull request description:

  This PR attempts to solve the problem explained in #16599.
  A particular attack which encouraged us to work on this issue is explained here  [[Erebus Attack against Bitcoin Peer-to-Peer Network](https://erebus-attack.comp.nus.edu.sg/)] (by @muoitranduc)

  Instead of relying on /16 prefix to diversify the connections every node creates, we would instead rely on the (ip -> ASN) mapping, if this mapping is provided.

  A .map file can be created by every user independently based on a router dump, or provided along with the Bitcoin release. Currently we use the python scripts written by @sipa to create a .map file, which is no larger than 2MB (awesome!).

  Here I suggest adding a field to peers.dat which would represent a hash of asmap file used while serializing addrman (or 0 for /16 prefix legacy approach).
  In this case, every time the file is updated (or grouping method changed), all buckets will be re-computed.
  I believe that alternative selective re-bucketing for only updated ranges would require substantial changes.

  TODO:
  - ~~more unit tests~~
  - ~~find a way to test the code without including >1 MB mapping file in the repo.~~
  - find a way to check that mapping file is not corrupted (checksum?)
  - comments and separate tests for asmap.cpp
  - make python code for .map generation public
  - figure out asmap distribution (?)

  ~Interesting corner case: I’m using std::hash to compute a fingerprint of asmap, and std::hash returns size_t. I guess  if a user updates the OS to 64-bit, then the hash of asap will change? Does it even matter?~

ACKs for top commit:
  laanwj:
    re-ACK 3c1bc40205
  jamesob:
    ACK 3c1bc40205 ([`jamesob/ackr/16702.3.naumenkogs.p2p_supplying_and_using`](https://github.com/jamesob/bitcoin/tree/ackr/16702.3.naumenkogs.p2p_supplying_and_using))
  jonatack:
    ACK 3c1bc40205

Tree-SHA512: e2dc6171188d5cdc2ab2c022fa49ed73a14a0acb8ae4c5ffa970172a0365942a249ad3d57e5fb134bc156a3492662c983f74bd21e78d316629dcadf71576800c
2020-01-29 13:55:43 +01:00
MarcoFalke
e09c701e01 scripted-diff: Bump copyright of files changed in 2020
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
2020-01-15 02:18:00 +07:00
MarcoFalke
6cbe620964 scripted-diff: Replace CCriticalSection with RecursiveMutex
-BEGIN VERIFY SCRIPT-
 # Delete outdated alias for RecursiveMutex
 sed -i -e '/CCriticalSection/d'                 ./src/sync.h
 # Replace use of outdated alias with RecursiveMutex
 sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
2020-01-15 01:43:46 +07:00
Pieter Wuille
9250a087d2 Convert addrdb/addrman to new serialization 2020-01-02 11:05:36 -08:00