RFC 9637 was newly introduced in 2024 but we don't handle this yet. This pull handles it in the same way as RFC 3849 of which 9637 is an extension.
This came up in #36196 because the new GetMappedAS() benchmark from #35285 asserts on these addresses being unmapped. New maps using --fill couldn't be embedded because the filling assigns these addresses. Making this range invalid in the code fixes the benchmarks.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36199.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | willcl-ark, hodlinator, sedited, 0xB10C |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
439 | @@ -434,7 +440,7 @@ bool CNetAddr::IsValid() const 440 | } 441 | 442 | // documentation IPv6 address 443 | - if (IsRFC3849()) 444 | + if (IsRFC3849() || IsRFC9637())
In 30e6eb3b2d77a21a9163f20d3e9d7e20f5bfaac7
If we remove this addition, no tests fail as we don't do any kind of BOOST_CHECK(!ResolveIP("3FFF::").IsValid()) in netbase_tests.cpp.
Added, thanks!
Rather than checking both
BOOST_CHECK(!ResolveIP("3FFF::").IsRoutable());
and
BOOST_CHECK(!ResolveIP("3FFF::").IsValid());
I think it would be better to only test IsValid() as initially suggested and also add a test for the general relationship between IsValid() and IsRoutable() in the fuzz test:
--- a/src/test/fuzz/netaddress.cpp
+++ b/src/test/fuzz/netaddress.cpp
@@ -72,7 +72,8 @@ FUZZ_TARGET(netaddress)
(void)net_addr.IsRFC6598();
(void)net_addr.IsRFC7343();
(void)net_addr.IsRFC9637();
- if (!net_addr.IsRoutable()) {
+ const bool routable{net_addr.IsRoutable()};
+ if (!routable) {
assert(net_addr.GetNetwork() == Network::NET_UNROUTABLE || net_addr.GetNetwork() == Network::NET_INTERNAL);
}
if (net_addr.IsTor()) {
@@ -84,7 +85,9 @@ FUZZ_TARGET(netaddress)
if (net_addr.IsCJDNS()) {
assert(net_addr.GetNetwork() == Network::NET_CJDNS);
}
- (void)net_addr.IsValid();
+ if (!net_addr.IsValid()) {
+ assert(!routable);
+ }
(void)net_addr.ToStringAddr();
const CSubNet sub_net{net_addr, fuzzed_data_provider.ConsumeIntegral<uint8_t>()};
Good suggestion, I dropped the IsRoutable check and added the fuzz coverage in a separate commit.
Concept ACK
ACK 30e6eb3b2d77a21a9163f20d3e9d7e20f5bfaac7
Adds calls in the same places as IsRFC3849() (and similarly to IsRFC5737() which is IPv4).
Makes the ASMapGetMappedASUnmappedv6() which verifies RFC 9637 address 3fff::1 confirm that it is unmapped, regardless of whether the ASMap binary blob contains it. Call chain verified in debugger:
ASMapGetMappedASUnmappedv6() -> BenchGetMappedAS() -> NetGroupManager::GetMappedAS() -> CNetAddr::GetNetClass() -> CNetAddr::IsRoutable() -> CNetAddr::IsValid() -> CNetAddr::IsRFC9637()
One could possibly prevent the ASMap filling optimization from assigning specific ranges to begin with. But the current PR approach should be implemented regardless.
Added test coverage as suggested by @willcl-ark , thanks!
One could possibly prevent the ASMap filling optimization from assigning specific ranges to begin with. But the current PR approach should be implemented regardless.
Seems doable but we should still not make assumptions about the user-supplied asmap file being clean, so we need to handle it on the core side either way, I think.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
ACK b48a27691ded8c9014be59b190726159b0bff038
Having the IsValid() test covering the changed behavior and the fuzz assertion checking that invalid addresses are never routable is neat!
re-ACK b48a27691ded8c9014be59b190726159b0bff038
Thanks for incorporating my fuzz test suggestion.
ACK b48a27691ded8c9014be59b190726159b0bff038
ACK b48a27691ded8c9014be59b190726159b0bff038
Thanks!
Milestone
32.0