WIP: A draft PR for collaborating on ZIP155 #5276
Conversation
zancas
force-pushed
the
zip-155-bcaddrv2
branch
from
9230dc1
to
a2239eb
Compare
zancas
force-pushed
the
zip-155-bcaddrv2
branch
from
a2239eb
to
19e0610
Compare
src/net.cpp
Outdated
| @@ -144,7 +144,7 @@ static std::vector<CAddress> convertSeed6(const std::vector<SeedSpec6> &vSeedsIn | |||
| { | |||
| struct in6_addr ip; | |||
| memcpy(&ip, i->addr, sizeof(ip)); | |||
| CAddress addr(CService(ip, i->port)); | |||
| CAddress addr(CService(ip, i->port), NODE_NETWORK); | |||
There was a problem hiding this comment.
@AloeareV I am still skeptical about the definition of NODE_NETWORK:
NODE_NETWORK = (1 << 0)
but your hypothesis that it's to make it explicit that the value is to be used as a bit-flag is consistent with other patterns we've seen in C++ code (e.g. type hints in names), so it's plausible.
There was a problem hiding this comment.
https://www.stroustrup.com/C++11FAQ.html <-- Let's stay aware of the strong enum pattern.
src/net.cpp
Outdated
| ///* We want -addnode to work even for nodes that don't provide all | ||
| // * wanted services, so pass in nServices=0 to CAddress. */ | ||
| //OpenNetworkConnection(CAddress(vserv[i % vserv.size()], 0), false, &grant); | ||
| //>>>>>>> 15bf863219 (Don't require services in -addnode) |
There was a problem hiding this comment.
We can probably remove the commented diff, the bool in the commented arg is only something other than false in bitcoin in a single call in ThreadOpenConnections, which looks to have been significantly refactored in zcash.
There was a problem hiding this comment.
Alright let's leave this comment thread open for now. Likely we'll circle back and clean up, per your recommendation.
src/protocol.h
Outdated
| @@ -87,7 +87,7 @@ class CAddress : public CService | |||
| { | |||
| public: | |||
| CAddress(); | |||
| explicit CAddress(CService ipIn, uint64_t nServicesIn = NODE_NETWORK); | |||
| explicit CAddress(CService ipIn, uint64_t nServicesIn); | |||
There was a problem hiding this comment.
IMHO this move toward explicitness is healthy.
src/protocol.cpp
Outdated
| @@ -79,7 +79,7 @@ CAddress::CAddress(CService ipIn, uint64_t nServicesIn) : CService(ipIn) | |||
|
|
|||
| void CAddress::Init() | |||
| { | |||
| nServices = NODE_NETWORK; | |||
| nServices = 0; | |||
zancas
force-pushed
the
zip-155-bcaddrv2
branch
from
19e0610
to
cdd3ec7
Compare
…n be shared and reused (cherry picked from commit 5e10922)
To make it clear where DNS resolves are happening (cherry picked from commit e9fc71e)
Note: Some seeds aren't actually returning an IP for their name entries, so they're being added to addrman with a source of [::]. This commit shouldn't change that behavior, for better or worse. (cherry picked from commit a98cd1f)
Rather than allowing CNetAddr/CService/CSubNet to launch DNS queries, require that addresses are already resolved. This greatly simplifies async resolve logic, and makes it harder to accidentally leak DNS queries. (cherry picked from commit 3675699)
CNetAddr/CService/CSubnet can no longer resolve DNS. (cherry picked from commit d39f5b4)
zancas
force-pushed
the
zip-155-bcaddrv2
branch
from
cdd3ec7
to
af2f2cd
Compare
|
I am reorganizing this code, will track elsewhere. |
High Level
@AloeareV, @dannasessha and I began this project by inspecting the PR that merged TorV3 addresses into bitcoin
main.From the perspective of "bitcoin main" circa 2019, there are two upgrades that were required. Add support for BIP155-style version 2 network addresses, part of BIP155:
And, given support for BIP155
addrv2s, add support for TorV3 addresses:Adding Support for BIP155
The addition of
addrv2support was initiated by adding regression tests:When we inspected that regression test code, we saw that the
netbasecode had been updated (relative to zcashmaincirca 2021) to reduce the responsibilities of theCNetAddr,CService, andCSubNetclasses. In particular newer versions are not responsible for name resolution.We discovered that we minimally required:
bitcoin/bitcoin@b6c3ff3
In order to proceed with the backport of 19351.
We noted the coherent structure of:
We surmised that all of 8128 would eventually be relevant for TorV3 support, and further noted it's dependence on:
Following the above reasoning we backported 7868 which depended on a small upgrade to error message formatting:
The
We need this code for
NODE_NONE:bitcoin/bitcoin@ee06e04
We cherry-picked bitcoin/bitcoin@07c493f for the
NET_TORtoNET_ONIONchange.This commit introduces GetAddrBytes: bitcoin/bitcoin@b691f2d
This commit introduces the
NET_INTERNALnetwork variant: bitcoin/bitcoin@7f31762The FLEXIBLE_SIZE change is part of prepation for us:
bitcoin/bitcoin@1ea57ad
We're going to need to backport span, prior to other bps (Making
CNetAddrmembers variable length), becauseSpanis used in core methods.ADDING SUPPORT FOR BITCOIN STYLE SERIALIZATION:
https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+is%3Amerged+%22Serialization+improvements%22+in%3Atitle
NOTES:
mainas slang formasterwhere I deem it to be (sufficiently) unambiguous.