fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses #26859

vasild · 2023-01-09T17:14:28Z

In the process of doing so, refactor ConsumeNetAddr() to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in RandAddr().

DrahtBot · 2023-01-09T17:14:30Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, brunoerg, achow101
Concept ACK	dergoegge, jonatack, chinggg
Approach ACK	w0xlt
Stale ACK	stratospher

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

dergoegge

Concept ACK

src/test/fuzz/addrman.cpp

vasild · 2023-01-10T10:17:44Z

0c8daaf752...46a7c30871: fix the build on 32 bit archs and add a comment

w0xlt

Approach ACK

mzumsande

Concept ACK

It would be good to also update the netaddress fuzz target with the added networks. At the minimum, we could check / assert IsI2P() for Network::NET_I2P there, maybe we could also assert that addresses from Tor, I2P and CJDNS are always valid, which I believe is guaranteed by the creation process?

vasild · 2023-02-02T14:06:46Z

46a7c30871...74fea1e088: expand fuzz/netaddress.cpp with I2P and CJDNS, suggested above. Retry a few times to get a valid address inside RandAddr(), discussion.

src/test/fuzz/util/net.cpp

vasild · 2023-02-10T11:23:09Z

Don't merge this before the problems it uncovered are fixed by #27071 (the CI is failing anyway).

jonatack · 2023-02-14T21:22:59Z

Concept ACK. Reviewed prerequisite PR #27071.

chinggg · 2023-02-15T04:44:06Z

Concept ACK

vasild · 2023-09-08T11:42:48Z

74fea1e088...8acdbb5a22: rebase due to conflicts

This PR extends the tests which uncovers a bug. The CI will likely fail due to that bug. The fix of the bug is pending review at #27071.

0e6f6eb net: remove unused CConnman::FindNode(const CSubNet&) (Vasil Dimov) 9482cb7 netbase: possibly change the result of LookupSubNet() to CJDNS (Vasil Dimov) 53afa68 net: move MaybeFlipIPv6toCJDNS() from net to netbase (Vasil Dimov) 6e30865 net: move IsReachable() code to netbase and encapsulate it (Vasil Dimov) c42ded3 fuzz: ConsumeNetAddr(): avoid IPv6 addresses that look like CJDNS (Vasil Dimov) 64d6f77 net: put CJDNS prefix byte in a constant (Vasil Dimov) Pull request description: `LookupSubNet()` would treat addresses that start with `fc` as IPv6 even if `-cjdnsreachable` is set. This creates the following problems where it is called: * `NetWhitelistPermissions::TryParse()`: otherwise `-whitelist=` fails to white list CJDNS addresses: when a CJDNS peer connects to us, it will be matched against IPv6 `fc...` subnet and the match will never succeed. * `BanMapFromJson()`: CJDNS bans are stored as just IPv6 addresses in `banlist.json`. Upon reading from disk they have to be converted back to CJDNS, otherwise, after restart, a ban entry like (`fc00::1`, IPv6) would not match a peer (`fc00::1`, CJDNS). * `RPCConsole::unbanSelectedNode()`: in the GUI the ban entries go through `CSubNet::ToString()` and back via `LookupSubNet()`. Then it must match whatever is stored in `BanMan`, otherwise it is impossible to unban via the GUI. These were uncovered by bitcoin/bitcoin#26859. Thus, flip the result of `LookupSubNet()` to CJDNS if the network base address starts with `fc` and `-cjdnsreachable` is set. Since subnetting/masking does not make sense for CJDNS (the address is "random" bytes, like Tor and I2P, there is no hierarchy) treat `fc.../mask` as an invalid `CSubNet`. To achieve that, `MaybeFlipIPv6toCJDNS()` has to be moved from `net` to `netbase` and thus also `IsReachable()`. In the process of moving `IsReachable()`, `SetReachable()` and `vfLimited[]` encapsulate those in a class. ACKs for top commit: jonatack: Code review ACK 0e6f6eb achow101: ACK 0e6f6eb mzumsande: re-ACK 0e6f6eb Tree-SHA512: 4767a60dc882916de4c8b110ce8de208ff3f58daaa0b560e6547d72e604d07c4157e72cf98b237228310fc05c0a3922f446674492e2ba02e990a272d288bd566

vasild · 2023-10-31T09:59:19Z

8acdbb5a22...722fdf14a7: rebase and take out of draft because #27071 has been merged. Ready for review.

vasild · 2024-01-22T16:30:47Z

To reproduce: Base64: IAEgdZ/fIAAAAAAaAAQAAUlJSUlZAAAAAALfAAAAGgAEAAH/+/5lAAAAgAAAIwCAKQ==.

@mzumsande, right! What about this fix (git diff -w):

--- i/src/test/fuzz/banman.cpp
+++ w/src/test/fuzz/banman.cpp
@@ -67,18 +67,20 @@ FUZZ_TARGET(banman, .init = initialize_banman)
         LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)
         {   
             CallOneOf(
                 fuzzed_data_provider,
                 [&] {
                     CNetAddr net_addr{ConsumeNetAddr(fuzzed_data_provider)};
+                    if (!net_addr.IsCJDNS()) {
                         const std::optional<CNetAddr>& addr{LookupHost(net_addr.ToStringAddr(), /*fAllowLookup=*/false
                         if (addr.has_value() && addr->IsValid()) {
                             net_addr = *addr;
                         } else {
                             contains_invalid = true;
                         }
+                    }
                     ban_man.Ban(net_addr, ConsumeBanTimeOffset(fuzzed_data_provider), fuzzed_data_provider.ConsumeBool
                 },
                 [&] {
                     CSubNet subnet{ConsumeSubNet(fuzzed_data_provider)};

@brunoerg?

brunoerg · 2024-01-23T10:27:34Z

Concept ACK

brunoerg · 2024-01-23T10:31:05Z

@brunoerg?

Sounds good to me. I didn't check the code yet but is the CJDNS address from ConsumeNetAddr always valid?

In the process of doing so, refactor `ConsumeNetAddr()` to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in `RandAddr()`.

vasild · 2024-01-23T10:54:13Z

790f0cb60d...b851c5385d: rebase and adjust fuzz/banman.cpp due to #26859 (review).

is the CJDNS address from ConsumeNetAddr always valid

Yes, I think so. Anyway, for safety wrt future changes I added IsValid() to the check. That is - omit lookup for valid CJDNS addresses.

mzumsande

ACK b851c53

I let the banman fuzzer run for a while, and it didn't crash anymore.

brunoerg

utACK b851c53

achow101 · 2024-01-31T21:35:40Z

ACK b851c53

jonatack

Post-merge ACK b851c53

jonatack · 2024-02-02T17:02:52Z

src/netaddress.h

+    /**
+     * BIP155 network ids recognized by this software.
+     */
+    enum BIP155Network : uint8_t {


Nice, making BIP155Network a public enum facilitates updating #27385.

DrahtBot added the Tests label Jan 9, 2023

vasild mentioned this pull request Jan 9, 2023

fuzz: Make ConsumeNetAddr always produce valid onion addresses #26497

Merged

dergoegge reviewed Jan 9, 2023

View reviewed changes

src/test/fuzz/addrman.cpp Outdated Show resolved Hide resolved

This was referenced Jan 9, 2023

ci: Use clang-15 and IWYU v0.19 in "tidy" task #26766

Closed

net: Use serialization parameters for CAddress serialization #25284

Merged

vasild force-pushed the ConsumeNetAddr_I2P_CJDNS branch from 0c8daaf to 46a7c30 Compare January 10, 2023 10:16

w0xlt reviewed Jan 10, 2023

View reviewed changes

mzumsande reviewed Jan 31, 2023

View reviewed changes

vasild force-pushed the ConsumeNetAddr_I2P_CJDNS branch from 46a7c30 to 74fea1e Compare February 2, 2023 14:04

mzumsande reviewed Feb 2, 2023

View reviewed changes

src/test/fuzz/util/net.cpp Outdated Show resolved Hide resolved

vasild mentioned this pull request Feb 10, 2023

Handle CJDNS from LookupSubNet() #27071

Merged

DrahtBot mentioned this pull request Apr 1, 2023

net, refactor: extract Network and BIP155Network logic to node/network #27385

Closed

achow101 marked this pull request as draft April 25, 2023 16:17

DrahtBot added the Needs rebase label Sep 7, 2023

vasild force-pushed the ConsumeNetAddr_I2P_CJDNS branch from 74fea1e to 8acdbb5 Compare September 8, 2023 11:40

DrahtBot added CI failed and removed Needs rebase labels Sep 8, 2023

This was referenced Sep 11, 2023

Use serialization parameters for CTransaction #28438

Merged

refactor: Serialization parameter cleanups #28473

Merged

DrahtBot added the Needs rebase label Sep 15, 2023

vasild force-pushed the ConsumeNetAddr_I2P_CJDNS branch from 8acdbb5 to 722fdf1 Compare October 31, 2023 09:58

DrahtBot requested review from w0xlt and removed request for w0xlt January 22, 2024 16:30

DrahtBot requested review from w0xlt and removed request for w0xlt January 23, 2024 10:27

DrahtBot requested review from w0xlt and removed request for w0xlt January 23, 2024 10:31

vasild force-pushed the ConsumeNetAddr_I2P_CJDNS branch from 790f0cb to b851c53 Compare January 23, 2024 10:50

DrahtBot removed the CI failed label Jan 23, 2024

mzumsande reviewed Jan 25, 2024

View reviewed changes

DrahtBot requested review from brunoerg, stratospher and w0xlt and removed request for w0xlt January 25, 2024 23:24

brunoerg reviewed Jan 26, 2024

View reviewed changes

DrahtBot requested review from w0xlt and removed request for w0xlt January 26, 2024 01:02

DrahtBot requested review from w0xlt and removed request for w0xlt January 31, 2024 21:35

achow101 merged commit aa9231f into bitcoin:master Jan 31, 2024

vasild deleted the ConsumeNetAddr_I2P_CJDNS branch February 1, 2024 08:59

jonatack reviewed Feb 2, 2024

View reviewed changes

bitcoin locked and limited conversation to collaborators Feb 1, 2025

fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses #26859

fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses #26859

Uh oh!

Conversation

vasild commented Jan 9, 2023

Uh oh!

DrahtBot commented Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

dergoegge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Jan 10, 2023

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Feb 2, 2023

Uh oh!

Uh oh!

vasild commented Feb 10, 2023

Uh oh!

jonatack commented Feb 14, 2023

Uh oh!

chinggg commented Feb 15, 2023

Uh oh!

vasild commented Sep 8, 2023

Uh oh!

vasild commented Oct 31, 2023

Uh oh!

vasild commented Jan 22, 2024

Uh oh!

brunoerg commented Jan 23, 2024

Uh oh!

brunoerg commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild commented Jan 23, 2024

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jan 31, 2024

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

DrahtBot commented Jan 9, 2023 •

edited

Loading

brunoerg commented Jan 23, 2024 •

edited

Loading