net: improve encapsulation of CNetAddr #19360

pull vasild wants to merge 1 commits into bitcoin:master from vasild:improve_encapsulation_of_cnetaddr changing 2 files +9 −7

vasild commented at 9:27 AM on June 23, 2020: contributor

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).

(chopped off from #19031 to ease review)
fanquake added the label P2P on Jun 23, 2020
vasild commented at 9:32 AM on June 23, 2020: contributor

All modified code is covered by existent tests. For example: CService::GetKey().
vasild cross-referenced this on Jun 23, 2020 from issue Implement ADDRv2 support (part of BIP155) by vasild
DrahtBot commented at 9:50 AM on June 23, 2020: contributor



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



Conflicts

No conflicts as of last run.
naumenkogs commented at 12:32 PM on June 23, 2020: member

utACK 567f98ea5d33d8cec699148be83b079dd5189101
DrahtBot cross-referenced this on Jun 25, 2020 from issue netaddress: Simplify reachability logic by dongcarl
vasild cross-referenced this on Jul 6, 2020 from issue Tor v3 support by vasild

net: improve encapsulation of CNetAddr

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>

bc74a40a56

vasild force-pushed on Jul 10, 2020
vasild commented at 2:02 PM on July 10, 2020: contributor

Updated to use CNetAddr::GetAddrBytes(), see #19031 (comment) for longer explanation.

Now this PR is even simpler.
dongcarl commented at 4:27 PM on July 10, 2020: contributor

ACK bc74a40a56128f81f11151d5966f53b82f19038c

Restarted Travis job

in src/netaddress.cpp:729 in bc74a40a56

 730 | -     vKey.resize(18);
 731 | -     memcpy(vKey.data(), ip, 16);
 732 | -     vKey[16] = port / 0x100; // most significant byte of our port
 733 | -     vKey[17] = port & 0x0FF; // least significant byte of our port
 734 | -     return vKey;
 735 | +    auto key = GetAddrBytes();

jonatack commented at 12:42 PM on July 11, 2020:

I like auto, but if this is critical code, maybe it would be prudent to use the explicit std::vector<unsigned char> type.

jonatack commented at 12:43 PM on July 11, 2020: contributor

ACK bc74a40a56128f81f11151d5966f53b82f19038c
naumenkogs commented at 7:19 AM on July 14, 2020: member

ACK bc74a40
fjahr commented at 12:11 PM on July 15, 2020: contributor

Code review ACK bc74a40

in src/netaddress.h:165 in bc74a40a56

 159 | @@ -160,7 +160,11 @@ class CService : public CNetAddr
 160 |          CService(const struct in6_addr& ipv6Addr, uint16_t port);
 161 |          explicit CService(const struct sockaddr_in6& addr);
 162 |  
 163 | -        SERIALIZE_METHODS(CService, obj) { READWRITE(obj.ip, Using<BigEndianFormatter<2>>(obj.port)); }
 164 | +        SERIALIZE_METHODS(CService, obj)
 165 | +        {
 166 | +            READWRITEAS(CNetAddr, obj);

jnewbery commented at 4:55 PM on July 15, 2020:

This should be updated to use the new AsBase<> syntax from #19503 if that PR gets merged first.

jnewbery commented at 4:56 PM on July 15, 2020: member

ACK bc74a40a5
laanwj commented at 7:17 PM on July 15, 2020: member

code review ACK bc74a40a56128f81f11151d5966f53b82f19038c
laanwj merged this on Jul 15, 2020
laanwj closed this on Jul 15, 2020
MarcoFalke cross-referenced this on Jul 15, 2020 from issue Add parameter feature to serialization and use it for CAddress by sipa
sidhujag referenced this in commit effb941b99 on Jul 16, 2020
vasild deleted the branch on Jul 16, 2020
vasild commented at 10:16 AM on July 16, 2020: contributor

Thanks for reviewing!

The reward for work well done is the opportunity to do more. -- Jonas Salk

So, the next one for review is at #19534 :-)
PastaPastaPasta referenced this in commit e8c6989e95 on Jan 16, 2021
UdjinM6 referenced this in commit bce94b7837 on Jan 18, 2021
deadalnix referenced this in commit 2184cc687d on Feb 5, 2021
UdjinM6 referenced this in commit 6e5210adeb on May 28, 2021
furszy cross-referenced this on Jun 7, 2021 from issue Road to Tor v3 support (BIP155) by furszy
random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18884 Tor v3 support #19031 Implement ADDRv2 support (part of BIP155)#19503 Add parameter feature to serialization and use it for CAddress #19534 net: save the network type explicitly in CNetAddr

net: improve encapsulation of CNetAddr #19360

Conflicts