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
Reduce the condition `IsRFC4193() && !IsTor()` to `IsRFC4193()`. We know
that if `IsRFC4193()` is `true` then, for sure, the address is not Tor,
so `!IsTor()` is also `true`.
If a scope id is provided, return it back in the string representation.
Also bring back the test. Closes#21982.
Co-authored-by: Jon Atack <jon@atack.com>
Recognize also I2P addresses in the form `base32hashofpublickey.b32.i2p`
from `CNetAddr::SetSpecial()`.
This makes `Lookup()` support them, which in turn makes it possible to
manually connect to an I2P node by using
`-proxy=i2p_socks5_proxy:port -addnode=i2p_address.b32.i2p:port`
Co-authored-by: Lucas Ontivero <lucasontivero@gmail.com>
Allow creation of valid `CSubNet` objects of non-IP networks and only
match the single address they were created from (like /32 for IPv4 or
/128 for IPv6).
This fixes a deficiency in `CConnman::DisconnectNode(const CNetAddr& addr)`
and in `BanMan` which assume that creating a subnet from any address
using the `CSubNet(CNetAddr)` constructor would later match that address
only. Before this change a non-IP subnet would be invalid and would not
match any address.
886be97af5 Ignore incorrectly-serialized banlist.dat entries (Pieter Wuille)
883cea7dea Restore compatibility with old CSubNet serialization (Pieter Wuille)
Pull request description:
#19628 changed CSubNet for IPv4 netmasks, using the first 4 bytes of `netmask` rather than the last 4 to store the actual mask. Unfortunately, CSubNet objects are serialized on disk in banlist.dat, breaking compatibility with existing banlists (and bringing them into an inconsistent state where entries reported in `listbanned` cannot be removed).
Fix this by reverting to the old format (just for serialization). Also add a sanity check to the deserializer so that nonsensical banlist.dat entries are ignored (which would otherwise be possible if someone added IPv4 entries after #19628 but without this PR).
Reported by Greg Maxwell.
ACKs for top commit:
laanwj:
Code review ACK 886be97af5
vasild:
ACK 886be97af
Tree-SHA512: d3fb91e8ecd933406e527187974f22770374ee2e12a233e7870363f52ecda471fb0b7bae72420e8ff6b6b1594e3037a5115984c023dbadf38f86aeaffcd681e7
3984b78cd7 test: Add tests for CNode::ConnectedThroughNetwork (Hennadii Stepanov)
49fba9c1aa net: Add CNode::ConnectedThroughNetwork member function (Hennadii Stepanov)
d4dde24034 net: Add CNode::m_inbound_onion data member (Hennadii Stepanov)
Pull request description:
This PR:
- adds `CNode::ConnectedThroughNetwork` member function
- is based on #19991, and only last two commits belong to it
- is required for https://github.com/bitcoin-core/gui/pull/86 and #20002
ACKs for top commit:
jonatack:
re-ACK 3984b78cd7 per `git diff 3989fcf 3984b78c`
laanwj:
Code review ACK 3984b78cd7
Tree-SHA512: 23a9c8bca8dca75113b5505fe443b294f2d42d03c98c7e34919da12d8396beb8d0ada3a58ae16e3da04b7044395f72cf9c216625afc078256cd6c897ac42bf3d
dcf0cb4776 tor: make a TORv3 hidden service instead of TORv2 (Vasil Dimov)
353a3fdaad net: advertise support for ADDRv2 via new message (Vasil Dimov)
201a4596d9 net: CAddress & CAddrMan: (un)serialize as ADDRv2 (Vasil Dimov)
1d3ec2a1fd Support bypassing range check in ReadCompactSize (Pieter Wuille)
Pull request description:
This PR contains the two remaining commits from #19031 to complete the [BIP155](https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki) implementation:
`net: CAddress & CAddrMan: (un)serialize as ADDRv2`
`net: advertise support for ADDRv2 via new message`
plus one more commit:
`tor: make a TORv3 hidden service instead of TORv2`
ACKs for top commit:
jonatack:
re-ACK dcf0cb4776 per `git diff 9b56a68 dcf0cb4` only change since last review is an update to the release notes which partially picked up the suggested text. Running a node on this branch and addnode-ing to 6 other Tor v3 nodes, I see "addrv2" and "sendaddrv2" messages in getpeerinfo in both the "bytesrecv_per_msg" and "bytessent_per_msg" JSON objects.
sipa:
ACK dcf0cb4776
hebasto:
re-ACK dcf0cb4776, the node works flawlessly in all of the modes: Tor-only, clearnet-only, mixed.
laanwj:
Edit: I have to retract this ACK for now, I'm having some problems with this PR on a FreeBSD node. It drops all outgoing connections with this dcf0cb4776 merged on master (12a1c3ad1a).
ariard:
Code Review ACK dcf0cb4
Tree-SHA512: 28d4d0d817b8664d2f4b18c0e0f31579b2f0f2d23310ed213f1f436a4242afea14dfbf99e07e15889bc5c5c71ad50056797e9307ff8a90e96704f588a6171308
Introduce a new message `sendaddrv2` to signal support for ADDRv2.
Send the new message immediately after sending the `VERACK` message.
Add support for receiving and parsing ADDRv2 messages.
Send ADDRv2 messages (instead of ADDR) to a peer if he has
advertised support for it.
Co-authored-by: Carl Dong <contact@carldong.me>
Recognizing addresses from those networks allows us to accept and gossip
them, even though we don't know how to connect to them (yet).
Co-authored-by: eriknylund <erik@daychanged.com>
7b6d0f10a7 Remove old check for 3-byte shifted IP addresses from pre-0.2.9 node messages (Raúl Martínez (RME))
Pull request description:
The change removes an old check for IPv6 addresses in range ::ff:ff00:0:0:0/72 that were created due to a bug in size field of addr messages for 0.2.8 nodes and before.
This check is no longer needed as they are no more pre 0.2.9 nodes on the network (as per bitnodes network snapshot).
Credits for discovering this go to sipa in https://github.com/bitcoin/bitcoin/pull/19628#discussion_r475907453
Thanks for the attention!
ACKs for top commit:
sipa:
utACK 7b6d0f10a7
vasild:
ACK 7b6d0f1
Tree-SHA512: c5fab59dda2acafe143f607a4c5b636a54ac76fba651cad1ad1b09c94e88ab39503a31c2244c8f2664da68456c2a870c601d8894139c55cde9ece8161913ed2e
The change removes an old check for IPv6 addresses in range ::ff:ff00:0:0:0/72 that were created due to a bug in size field of addr messages for 0.2.8 nodes and before.
This check is no longer needed as they are no more pre 0.2.9 nodes on the network (as per bitnodes network snapshot).
Credits for discovering this go to sipa.
Before this change `CNetAddr::ip` was a fixed-size array of 16 bytes,
not being able to store larger addresses (e.g. TORv3) and encoded
smaller ones as 16-byte IPv6 addresses.
Change its type to `prevector`, so that it can hold larger addresses and
do not disguise non-IPv6 addresses as IPv6. So the IPv4 address
`1.2.3.4` is now encoded as `01020304` instead of
`00000000000000000000FFFF01020304`.
Rename `CNetAddr::ip` to `CNetAddr::m_addr` because it is not an "IP" or
"IP address" (TOR addresses are not IP addresses).
In order to preserve backward compatibility with serialization (where
e.g. `1.2.3.4` is serialized as `00000000000000000000FFFF01020304`)
introduce `CNetAddr` dedicated legacy serialize/unserialize methods.
Adjust `CSubNet` accordingly. Still use `CSubNet::netmask[]` of fixed 16
bytes, but use the first 4 for IPv4 (not the last 4). Only allow
subnetting for IPv4 and IPv6.
Co-authored-by: Carl Dong <contact@carldong.me>
A netmask that contains 1-bits after 0-bits (the 1-bits are not
contiguous on the left side) is invalid [1] [2].
The code before this PR used to parse and accept such
non-left-contiguous netmasks. However, a coming change that will alter
`CNetAddr::ip` to have flexible size would make juggling with such
netmasks more difficult, thus drop support for those.
[1] https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing#Subnet_masks
[2] https://tools.ietf.org/html/rfc4632#section-5.1
Before this change, we would analyze the contents of `CNetAddr::ip[16]`
in order to tell which type is an address. Change this by introducing a
new member `CNetAddr::m_net` that explicitly tells the type of the
address.
This is necessary because in BIP155 we will not be able to tell the
address type by just looking at its raw representation (e.g. both TORv3
and I2P are "seemingly random" 32 bytes).
As a side effect of this change we no longer need to store IPv4
addresses encoded as IPv6 addresses - we can store them in proper 4
bytes (will be done in a separate commit). Also the code gets
somewhat simplified - instead of
`memcmp(ip, pchIPv4, sizeof(pchIPv4)) == 0` we can use
`m_net == NET_IPV4`.
Co-authored-by: Carl Dong <contact@carldong.me>
Do not access `CNetAddr::ip` directly from `CService` methods.
This improvement will help later when we change the type of
`CNetAddr::ip` (in the BIP155 implementation).
Co-authored-by: Carl Dong <contact@carldong.me>
748977690e Add asmap_direct fuzzer that tests Interpreter directly (Pieter Wuille)
7cf97fda15 Make asmap Interpreter errors fatal and fuzz test it (Pieter Wuille)
c81aefc537 Add additional effiency checks to sanity checker (Pieter Wuille)
fffd8dca2d Add asmap sanity checker (Pieter Wuille)
5feefbe6e7 Improve asmap Interpret checks and document failures (Pieter Wuille)
2b3dbfa5a6 Deal with decoding failures explicitly in asmap Interpret (Pieter Wuille)
1479007a33 Introduce Instruction enum in asmap (Pieter Wuille)
Pull request description:
This improves/documents the failure cases inside the asmap interpreter. None of the changes are bug fixes (they only change behavior for corrupted asmap files), but they may make things easier to follow.
In a second step, a sanity checker is added that effectively executes every potential code path through the asmap file, checking the same failure cases as the interpreter, and more. It takes around 30 ms to run for me for a 1.2 MB asmap file.
I've verified that this accepts asmap files constructed by https://github.com/sipa/asmap/blob/master/buildmap.py with a large dataset, and no longer accepts it with 1 bit changed in it.
ACKs for top commit:
practicalswift:
ACK 748977690e modulo feedback below.
jonatack:
ACK 748977690e code review, regular build/tests/ran bitcoin with -asmap, fuzz build/ran both fuzzers overnight.
fjahr:
ACK 748977690e
Tree-SHA512: d876df3859735795c857c83e7155ba6851ce839bdfa10c18ce2698022cc493ce024b5578c1828e2a94bcdf2552c2f46c392a251ed086691b41959e62a6970821
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
