Net: Turn net structures into dumb storage classes by theuni · Pull Request #8128 · bitcoin/bitcoin

theuni · 2016-05-31T22:51:26Z

This finishes the work started in #7868. The changed lines count is high, but it's mostly tests.

CNetAddr/CService/CSubNet lose their string constructors, they must now have lookup operations performed externally. This means that functions/classes that depend on them are no longer dependent on any particular lookup mechanism.

From a high level: New resolvers/parsers may now be used for net operations. libbtcnet will replace what remains of netbase with async versions.

sipa · 2016-06-01T16:24:49Z

Concept ACK. I'll review for move and correctness later.

jonasschnelli · 2016-06-01T19:01:01Z

Concept ACK.

nits:

dcousens · 2016-06-02T03:08:16Z

concept ACK (utACK only up to 1394611 for now)

theuni · 2016-06-25T14:40:19Z

Rebased

sipa · 2016-07-21T12:43:13Z

src/netbase.cpp

-                {
-                    valid = false;
-                }
+            if (ParseInt32(strNetmask, &n)) { // If valid number, assume /24 symtex


symtex? syntax?

sipa · 2016-07-21T12:53:54Z

utACK 584dcdc with a tiny nit.
Verified d374dc0 to be move-only.

theuni · 2016-07-27T21:11:09Z

@sipa Thanks for the review. I added a separate commit for the nit rather than clobbering the verified move-only commit hashes. I'm happy to squash for merge.

sipa · 2016-07-30T00:24:26Z

Needs rebase.

sipa · 2016-07-30T21:15:41Z

@theuni Trivial rebase: https://github.com/sipa/bitcoin/commits/pr8128.

Net functionality is no longer needed for CAddress/CAddrman/etc. now that CNetAddr/CService/CSubNet are dumb storage classes.

theuni · 2016-07-31T18:02:34Z

Rebased and squashed the nit while I was at it.

@sipa: I rebased on the same commit as you, so the diff should be null.

paveljanik · 2016-07-31T19:02:33Z

src/Makefile.am

  miner.h \
  net.h \
  netbase.h \
+  netaddress.h \


nit: Please move netaddress.h one line up to match the alphabet and the cpp files order below.

paveljanik · 2016-07-31T20:20:43Z

utACK 21ba407

This also helped reducing shadow warnings from the net* codes (e.g. for randomize_credentials: from 309 occurrences in the build log to 42 only).

laanwj · 2016-08-01T06:58:55Z

src/httpserver.cpp

        uint16_t port = 0;
        evhttp_connection_get_peer(con, (char**)&address, &port);
-        peer = CService(address, port);
+        LookupNumeric(address, peer, port);


You ignore the return value of Lookup* almost everywhere. Is this on purpose?
I think the old behavior was to return a service with !IsValid() when the lookup fails. But if that is still the case we could just as well have the API be:

peer = LookupNumeric(address, port);

Then check for IsValid afterward.

Yes, this is inconsistent. The return value was intended to be a shortcut to avoid having to check for IsValid afterwards, but most places end up forwarding the result elsewhere even if invalid.

I'll just remove the return.

Also fix up a few small issues: - Lookup with "badip:port" now sets the port to 0 - Don't allow assert to have side-effects

sipa · 2016-08-05T19:09:43Z

utACK 8945384

sipa · 2016-08-10T17:45:18Z

@theuni Can you address @paveljanik's comments above?

theuni · 2016-08-11T02:00:12Z

Whoops, missed @paveljanik's batch.
@paveljanik Logic for the cstr's only was that they're going to be used as pointers for the lookup anyway. That kinda breaks down though since we end up using strings for pre-processing. I can add a string version as well if you'd like.

Will fix up the nits.

paveljanik · 2016-08-11T11:45:40Z

@theuni I think it can simplify a lot of code... But I'll leave the decision on you, of course.

theuni · 2016-08-12T18:20:29Z

@paveljanik Looking at the others, I'd rather not add a string method here. The most logical (imo) choices are:

Use char* and std::string for Lookup*. Lots of duplication for functionality that just ends up acting on c strings anyway.
Use std::string (by value) everywhere. Dangerous when passing in null pointers.
Use char* everywhere. Means using c_str() to call in from std::string, but otherwise harmless.

I think I'd prefer to leave it as-is unless there's a compelling reason otherwise.

paveljanik · 2016-08-12T18:35:05Z

@theuni: OK, agreed.

utACK 9e9d644

laanwj · 2016-08-13T15:12:52Z

Agree with the decision to use char* here. C string manipulation, because of the inherent buffer overflow risks, should not be used in any new code, but there is no need to refactor this right now in old code, or new code closely modeled after the old code. And these strings are not manipulated anyhow, but directly passed through to libevent/the OS, so nothing would be won by making them std::string.

laanwj · 2016-08-15T11:26:17Z

utACK 9e9d644

9e9d644 net: fixup nits (Cory Fields) 8945384 net: Have LookupNumeric return a CService directly (Cory Fields) 21ba407 net: narrow include scope after moving to netaddress (Cory Fields) 21e5b96 net: move CNetAddr/CService/CSubNet out of netbase (Cory Fields) 1017b8a net: Add direct tests for new CSubNet constructors (Cory Fields) b6c3ff3 net: Split resolving out of CSubNet (Cory Fields) f96c7c4 net: Split resolving out of CService (Cory Fields) 31d6b1d net: Split resolving out of CNetAddr (Cory Fields)

b36f743 Fix masternode service lookup (furszy) 65feb7f fixup styling (Fuzzbawls) 059ca2c net: fixup nits (Cory Fields) 5a51a5f net: Have LookupNumeric return a CService directly (Cory Fields) acd22b7 net: narrow include scope after moving to netaddress (Cory Fields) e5bea4b net: move CNetAddr/CService/CSubNet out of netbase (Cory Fields) 840d826 net: Add direct tests for new CSubNet constructors (Cory Fields) 63c46c6 net: Split resolving out of CSubNet (Fuzzbawls) a801a9e net: layer 2 CService abstraction (Fuzzbawls) 94732d2 Simplify checking of masternode.conf (Fuzzbawls) d6f81b5 RPC: Remove masternodeconnect command (Fuzzbawls) c4fe27e net: Split resolving out of CService (Cory Fields) 502343a net: Split resolving out of CNetAddr (Cory Fields) Pull request description: Backport of bitcoin#8128 as part of #1374. Based on top of #1640 Original Description: > CNetAddr/CService/CSubNet lose their string constructors, they must now have lookup operations performed externally. This means that functions/classes that depend on them are no longer dependent on any particular lookup mechanism. > > From a high level: New resolvers/parsers may now be used for net operations. libbtcnet will replace what remains of netbase with async versions. Additional work has been done in the following commits: * fcef585 - Removes a useless RPC command * 235c33e - Refactors the masternode.conf parsing with regards to valid ports * 3ddd35e - Adds additional sanity checks during client init for MN port numbers and listen options. ACKs for top commit: furszy: tested ACK b36f743 random-zebra: ACK b36f743 and merging... Tree-SHA512: 1663dc0591e3a4eeb50c1d3265ae125451bd9185e1e44e0cf36d0675fe93daf21d873ba0baa48f0e0e50b20f9313de2da5ee257eeb75c779cd07cebca61a3f99

jonasschnelli added the P2P label Jun 1, 2016

theuni force-pushed the split-resolve branch from 8b55ec9 to 491080a Compare June 1, 2016 18:52

theuni force-pushed the split-resolve branch from 491080a to 584dcdc Compare June 25, 2016 06:46

sipa reviewed Jul 21, 2016
View reviewed changes

src/netbase.cpp Outdated

{

valid = false;

}

if (ParseInt32(strNetmask, &n)) { // If valid number, assume /24 symtex

Copy link

Member

sipa Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

symtex? syntax?

theuni added 2 commits July 31, 2016 13:24

net: Split resolving out of CNetAddr

31d6b1d

net: Split resolving out of CService

f96c7c4

theuni force-pushed the split-resolve branch 2 times, most recently from a66f94e to 21ba407 Compare July 31, 2016 18:01

theuni added 4 commits July 31, 2016 14:01

net: Split resolving out of CSubNet

b6c3ff3

net: Add direct tests for new CSubNet constructors

1017b8a

net: move CNetAddr/CService/CSubNet out of netbase

21e5b96

net: narrow include scope after moving to netaddress

21ba407

Net functionality is no longer needed for CAddress/CAddrman/etc. now that CNetAddr/CService/CSubNet are dumb storage classes.

paveljanik reviewed Jul 31, 2016
View reviewed changes

laanwj reviewed Aug 1, 2016
View reviewed changes

net: Have LookupNumeric return a CService directly

8945384

Also fix up a few small issues: - Lookup with "badip:port" now sets the port to 0 - Don't allow assert to have side-effects

theuni mentioned this pull request Aug 5, 2016

Fix AddRef() usage for CNode #8372

Closed

theuni mentioned this pull request Aug 5, 2016

Make sure all ports are 16 bit numbers #8394

Closed

net: fixup nits

9e9d644

laanwj merged commit 9e9d644 into bitcoin:master Aug 15, 2016

This was referenced Aug 15, 2016

[Trivial] Do not shadow variables in networking code #8466

Merged

net: Feeler connections to increase online addrs in the tried table. #8282

Merged

UdjinM6 mentioned this pull request Jul 26, 2017

Backport Bitcoin PR#8708: net: have CConnman handle message sending dashpay/dash#1553

Merged

This was referenced Sep 2, 2017

Backport Bitcoin PR#7868: net: Split DNS resolving functionality out of net structures dashpay/dash#1601

Merged

Backport Bitcoin PR#8128: Net: Turn net structures into dumb storage classes dashpay/dash#1604

Merged

Fuzzbawls mentioned this pull request Mar 4, 2020

Tracking: Backport upstream network encapsulation PIVX-Project/PIVX#1374

Closed

21 tasks

Fuzzbawls mentioned this pull request May 25, 2020

[Net] Turn net structures into dumb storage classes PIVX-Project/PIVX#1646

Merged

PRCYDev mentioned this pull request Jan 25, 2021

Tracking: Backport upstream network encapsulation PRCYCoin/PRCYCoin#4

Open

21 tasks

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

theuni commented May 31, 2016

Uh oh!

sipa commented Jun 1, 2016

Uh oh!

jonasschnelli commented Jun 1, 2016

Uh oh!

dcousens commented Jun 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Jun 25, 2016

Uh oh!

sipa Jul 21, 2016

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 21, 2016

Uh oh!

theuni commented Jul 27, 2016

Uh oh!

sipa commented Jul 30, 2016

Uh oh!

sipa commented Jul 30, 2016

Uh oh!

theuni commented Jul 31, 2016

Uh oh!

paveljanik Jul 31, 2016

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Jul 31, 2016

Uh oh!

laanwj Aug 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni Aug 1, 2016

Choose a reason for hiding this comment

Uh oh!

sipa commented Aug 5, 2016

Uh oh!

sipa commented Aug 10, 2016

Uh oh!

theuni commented Aug 11, 2016

Uh oh!

paveljanik commented Aug 11, 2016

Uh oh!

theuni commented Aug 12, 2016

Uh oh!

paveljanik commented Aug 12, 2016

Uh oh!

laanwj commented Aug 13, 2016

Uh oh!

laanwj commented Aug 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dcousens commented Jun 2, 2016 •

edited

Loading

laanwj Aug 1, 2016 •

edited

Loading