Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Backport P2P refactor #3305

Merged
merged 18 commits into from
Jan 31, 2020
Merged

Backport P2P refactor #3305

merged 18 commits into from
Jan 31, 2020

Conversation

PastaPastaPasta
Copy link
Member

Backports 11363 and 13946

I decided to backport these somewhat out of order so that once v2 P2P system is merged into bitcoin it will be easier to backport.

laanwj added 6 commits January 25, 2020 10:46
@laanwj @PastaPastaPasta
Merge bitcoin#11363: net: Split socket create/connect
7f88a67 
3830b6e net: use CreateSocket for binds (Cory Fields)
df3bcf8 net: pass socket closing responsibility up to caller for outgoing connections (Cory Fields)
9e3b2f5 net: Move IsSelectableSocket check into socket creation (Cory Fields)
1729c29 net: split socket creation out of connection (Cory Fields)

Pull request description:

  Requirement for bitcoin#11227.

  We'll need to create sockets and perform the actual connect in separate steps, so break them up.

  bitcoin#11227 adds an RAII wrapper around connection attempts, as a belt-and-suspenders in case a CloseSocket is missed.

Tree-SHA512: de675bb718cc56d68893c303b8057ca062c7431eaa17ae7c4829caed119fa3f15b404d8f52aca22a6bca6e73a26fb79e898b335d090ab015bf6456cf417fc694
@laanwj @PastaPastaPasta
Merge bitcoin#13946: p2p: Clarify control flow in ProcessMessage
c261173 
fa6c3de p2p: Clarify control flow in ProcessMessage() (MarcoFalke)

Pull request description:

  `ProcessMessage` is effectively a massive switch case construct. In the past there were attempts to clarify the control flow in `ProcessMessage()` by moving each case into a separate static function (see bitcoin#9608). It was closed because it wasn't clear if moving each case into a function was the right approach.
  Though, we can quasi treat each case as a function by adding a return statement to each case. (Can be seen as a continuation of bugfix bitcoin#13162)

  This patch does exactly that.

  Also note that this patch is a subset of previous approaches such as bitcoin#9608 and bitcoin#10145.

  Review suggestion: `git diff HEAD~ --function-context`

Tree-SHA512: 91f6106840de2f29bb4f10d27bae0616b03a91126e6c6013479e1dd79bee53f22a78902b631fe85517dd5dc0fa7239939b4fefc231851a13c819458559f6c201
@laanwj @PastaPastaPasta
Merge bitcoin#11448: [gui] reset addrProxy/addrSeparateProxyTor if co…
88197c7 
…lon char missing

ce2418f [gui] reset addrProxy/addrSeparateProxyTor if colon char missing (Cristian Mircea Messel)

Pull request description:

  If addrProxy or addrSeparateProxyTor do not have a colon in the string
  somewhere in the QSettings storage, then attempting to open the options
  dialog will cause the entire program to crash.

  Fixes bitcoin#11209

Tree-SHA512: 2d9e6987cf05af3f41033290b61d00920f7fe4a65bea7efd96ed417a8ca7866d248f091e09947cc8aad3a6a4aa8b7777211cfff7f379a62188be50df2c46d4b2
@laanwj @PastaPastaPasta
Merge bitcoin#11512: Use GetDesireableServiceFlags in seeds, dnsseeds…
94e9944 
…, fixing static seed adding

2b839ab Update chainparams comment for more info on service bits per dnsseed (Matt Corallo)
62e7642 Fall back to oneshot for DNS Seeds which don't support filtering. (Matt Corallo)
51ae766 Use GetDesireableServiceFlags in static seeds, document this. (Matt Corallo)
fb6f6b1 bluematt's testnet-seed now supports x9 (and is just a static list) (Matt Corallo)

Pull request description:

  4440710 broke inserting entries into addrman from dnsseeds which
  did not support service bits, as well as static seeds. Static seeds
  were already being filtered by UA for 0.13.1+ (ie NODE_WITNESS), so
  simply changing the default service bits to include NODE_WITNESS
  (and updating docs appropriately) is sufficient. For DNS Seeds, not
  supporting NODE_WITNESS is no longer useful, so instead use
  non-filtering seeds as oneshot hosts irrespective of named proxy.

  I've set my testnet-seed to also support x9, though because it is simply a static host, it may be useful to leave the support off so that it is used as a oneshot to get addresses from a live node instead. I'm fine with either.

Tree-SHA512: 3f17d4d2b0b84d876981c962d2b44cb0c8f95f52c56a48c6b35fd882f6d7a40805f320ec452985a1c0b34aebddb1922709156c3ceccd1b9f8363fd7cb537d21d
@laanwj @PastaPastaPasta
Merge bitcoin#11577: Fix warnings (-Wsign-compare) when building with…
fccf28b 
… DEBUG_ADDRMAN

6eddd43 Fix warnings when building with DEBUG_ADDRMAN (practicalswift)

Pull request description:

  Fix warnings when building with `DEBUG_ADDRMAN`.

  Warnings prior to this commit:

  ```
  addrman.cpp:390:24: warning: comparison of integers of different signs: 'size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
      if (vRandom.size() != nTried + nNew)
          ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~
  addrman.cpp:411:52: warning: comparison of integers of different signs: 'int' and 'size_type' (aka 'unsigned long') [-Wsign-compare]
          if (info.nRandomPos < 0 || info.nRandomPos >= vRandom.size() || vRandom[info.nRandomPos] != n)
                                     ~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
  addrman.cpp:419:25: warning: comparison of integers of different signs: 'size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
      if (setTried.size() != nTried)
          ~~~~~~~~~~~~~~~ ^  ~~~~~~
  addrman.cpp:421:23: warning: comparison of integers of different signs: 'size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
      if (mapNew.size() != nNew)
          ~~~~~~~~~~~~~ ^  ~~~~
  4 warnings generated.
  ```

Tree-SHA512: 0316faecfe95066d2c9a0b6b3960086e43824f21a67086a895ea45fbce1327f8d6df5945fe923c2dbe4efce430bc1384d515d317c3930d97d24965e507cf734d
@laanwj @PastaPastaPasta
Merge bitcoin#11583: Do not make it trivial for inbound peers to gene…
f4fb2c1 
…rate log entries

be9f38c Do not make it trivial for inbound peers to generate log entries (Matt Corallo)

Pull request description:

  Based on bitcoin#11580 because I'm lazy.

  We should generally avoid writing to debug.log unconditionally for
  inbound peers which misbehave (the peer being about to be banned
  being an exception, since they cannot do this twice).

Tree-SHA512: 8e59c8d08d00b1527951b30f4842d010a4c2fc440503ade112baa2c1b9afd0e0d1c5c2df83dde25183a242af45089cf9b9f873b71796771232ffb6c5fc6cc0cc
@PastaPastaPasta
Copy link
Member Author

I decided to add more prs related to p2p code and refactoring of it. If you don't think it makes sense to backport some of these prs at this point I can drop them

@PastaPastaPasta PastaPastaPasta force-pushed the backport-11363 branch 2 times, most recently from ac77b23 to 60a0aad Compare January 27, 2020 02:42
laanwj added 7 commits January 26, 2020 20:44
@laanwj @PastaPastaPasta
Merge bitcoin#11744: net: Add missing locks in net.{cpp,h}
500a60a 
bfb0c0a Add Clang thread safety analysis annotations (practicalswift)
63f21d2 net: Add missing locks in net.{cpp,h} (practicalswift)

Pull request description:

  Add missing locks in `net.{cpp,h}`:
  * writing variable `nTotalBytesRecv` requires holding mutex `cs_totalBytesRecv` exclusively
  * writing variables `nTotalBytesSent`, `nMaxOutboundTotalBytesSentInCycle` and `nMaxOutboundCycleStartTime` require holding mutex `cs_totalBytesSent` exclusively
  * writing variables `nMaxOutboundTimeframe` and `nMaxOutboundLimit` require holding mutex `cs_totalBytesSent` exclusively
  * writing variable `vAddedNodes` requires holding mutex `cs_vAddedNodes` exclusively

Tree-SHA512: 54a5b4bc6dc6f404dacf403af2ddd7b2214cc0a17d1d32a282def1c6b536105dada56bfabbc8606f56755f2d24874abba09913b51c8d13b0f2b000149551f0b0
@laanwj @PastaPastaPasta
Merge bitcoin#12218: net: Move misbehaving logging to net logging cat…
4954939 
…egory

d3a185a net: Move misbehaving logging to net logging category (Wladimir J. van der Laan)

Pull request description:

  This moves the error messages for misbehavior (when available) into the line that reports the misbehavior, as well as moves the logging to the `net` category.

  This is a continuation of bitcoin#11583 and avoids serious-looking errors due to misbehaving peers. As it is impossible to correlate the `peer=X` numbers to specific incoming connections now without enabling the `net` category, it doesn't really help to see these messages by default.

  To do this, Misbehaving() gains an optional `message` argument.

  E.g. change:

      2018-01-18 16:02:27 Misbehaving: x.x.x.x:62174 peer=164603 (80 -> 100) BAN THRESHOLD EXCEEDED
      2018-01-18 16:02:27 ERROR: non-continuous headers sequence

  to

      2018-01-18 16:02:27 Misbehaving: x.x.x.x:62174 peer=164603 (80 -> 100) BAN THRESHOLD EXCEEDED: non-continuous headers sequence

  When there is a category for "important" net messages (see bitcoin#12219 ), we should move it there.

Tree-SHA512: 51c97e9a649bf5409f2fd4625fa1243a036e9c9de6037bb064244207408c2e0eb025e3af80866df673cdc006b8f35dc4078d074033f0d4c6a73bbb03949a269f
@laanwj @PastaPastaPasta
Merge bitcoin#12326: net: initialize socket to avoid closing random fd's
211c8a3 
96dbd38 net: initialize socket to avoid closing random fd's (Cory Fields)

Pull request description:

  An excellent spot by @david60.

  Even if it isn't causing the fd issue we're looking for, this should be fixed.

Tree-SHA512: 062a8f2cdd39d895213e1263dbd7b8391473ddaea2f93c82c211a9bb6ea6744d48a6c84c8ff804b16b865d14145492635c500a9fd138d0988fee5e4f719ebb91
@laanwj @PastaPastaPasta
Merge bitcoin#12329: net: don't retry failed oneshot connections forever
0357621 
660f5f1 net: don't retry failed oneshot connections forever (Cory Fields)

Pull request description:

  As introduced by (my suggestion, sorry, in) bitcoin#11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves.

  Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it.

  Maybe @sipa can shed a light on what the original intention was.

Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd

fix 12329 backport

Signed-off-by: Pasta <pasta@dashboost.org>

fix 12329 backport

Signed-off-by: Pasta <pasta@dashboost.org>

fix 12329 backport

Signed-off-by: Pasta <pasta@dashboost.org>
@laanwj @PastaPastaPasta
Merge bitcoin#12342: Extend bitcoin#11583 to include "version handsha…
12a9809 
…ke timeout" message

c887f87 Extend bitcoin#11583 to include the most common message generated by non-contributing peers (port scanners?) 37% of the log default log entries for a node that has been up for ~24hrs was "version handshake timeout..." (Clem Taylor)

Pull request description:

  37% of the default log entries for a node that has been up for ~24hrs was "version handshake timeout..."

Tree-SHA512: dceeee5d55a9ff7570174aeb63faac9beda239087220522adefef7ed11e0eeffa008ca28726011247c8834c1a222d37817baf895635ab874a95ebc435959070e
@laanwj @PastaPastaPasta
Merge bitcoin#12626: Limit the number of IPs addrman learns from each…
a3ea0e9 
… DNS seeder

46e7f80 Limit the number of IPs we use from each DNS seeder (e0)

Pull request description:

  A risk exists where a malicious DNS seeder eclipses a node by returning an enormous number of IP addresses. In this commit we mitigate this risk by limiting the number of IP addresses addrman learns to 256 per DNS seeder.

  As discussed with @theuni

Tree-SHA512: 949e870765b1470200f2c650341d9e3308a973a7d1a6e557b944b0a2b8ccda49226fc8c4ff7d2a05e5854c4014ec0b67e37a3f2287556fe7dfa2048ede1f2e6f
@laanwj @PastaPastaPasta
Merge bitcoin#12855: net: Minor accumulated cleanups
e7beff5 
2c084a6 net: Minor accumulated cleanups (Thomas Snider)

Pull request description:

  From now-derelict larger changes I had been working on, here are a series of DRY refactors/cleanups.  Net loss of 35 lines of code - a small step in the good fight.

  In particular I think operator!= should only ever be implemented as a negation of operator==.  Lower chance for errors, and removes the possibility of divergent behavior.

Tree-SHA512: 58bf4b542a4e8e5bc465b508aaa16e9ab51448c3f9bee52cd9db0a64a5c6c5a13e4b4286d0a5aa864934fc58064799f6a88a40a87154fd3a4bd731a72e254393
@UdjinM6 UdjinM6 added this to the 16 milestone Jan 27, 2020
@UdjinM6
Copy link

UdjinM6 commented Jan 28, 2020

pls see https://github.com/UdjinM6/dash/commits/pr3305

UdjinM6 and others added 5 commits January 28, 2020 15:42
@UdjinM6 @PastaPastaPasta
Fix "\n"s
0255027
@UdjinM6 @PastaPastaPasta
More of 12218 for Dash-specific code
6eb6298
@UdjinM6 @PastaPastaPasta
More of 11583 for Dash-specific code
67c8484
@UdjinM6 @PastaPastaPasta
More of 13946
0d56f80
@laanwj @PastaPastaPasta
Merge bitcoin#13162: [net] Don't incorrectly log that REJECT messages…
5cc46c3 
… are unknown.

fad63eb [logging] Don't incorrectly log that REJECT messages are unknown. (John Newbery)

Pull request description:

  Reject messages are logged to debug.log if NET debug logging is enabled.

  Because of the way the `ProcessMessages()` function is structured,
  processing for REJECT messages will also drop through to the default
  branch and incorrectly log `Unknown command "reject" from peer-?`. Fix
  that by exiting from `ProcessMessages()` early.

  without this PR:
  ```
  2018-05-03T17:37:00.930600Z received: reject (21 bytes) peer=0
  2018-05-03T17:37:00.930620Z Reject message code 16: spammy spam
  2018-05-03T17:37:00.930656Z Unknown command "reject" from peer=0
  ```
  with this PR:
  ```
  2018-05-03T17:35:04.751246Z received: reject (21 bytes) peer=0
  2018-05-03T17:35:04.751274Z Reject message code 16: spammy spam
  ```

Tree-SHA512: 5c84c98433ab99e0db2dd481f9c2db6f87ff0d39022ff317a791737e918714bbcb4a23e81118212ed8e594ebcf098ab7f52f7fd5e21ebc3f07b1efb279b9b30b
UdjinM6
UdjinM6 approved these changes Jan 28, 2020
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

This was referenced Jan 30, 2020
Merged
Merged
codablock
codablock approved these changes Jan 31, 2020
View reviewed changes
Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

utACK. Only a question in inline comments

src/chainparams.cpp Show resolved Hide resolved
@UdjinM6 UdjinM6 merged commit 3bcf23f into dashpay:develop Jan 31, 2020
@PastaPastaPasta PastaPastaPasta deleted the backport-11363 branch January 31, 2020 15:19
@UdjinM6 UdjinM6 mentioned this pull request Feb 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@codablock codablock codablock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
16
Development

Successfully merging this pull request may close these issues.
4 participants
@PastaPastaPasta @UdjinM6 @codablock @laanwj