Skip to content

Backport More P2P#3415

Merged
UdjinM6 merged 19 commits intodashpay:developfrom
PastaPastaPasta:backport-13417
Jun 11, 2020
Merged

Backport More P2P#3415
UdjinM6 merged 19 commits intodashpay:developfrom
PastaPastaPasta:backport-13417

Conversation

@PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Apr 16, 2020

this builds on #3414 and will be rebased once that is merged. This is a number of PRs which prepare us for a couple of critical backports, namely the v2 message protocol once that comes out as well as the separation between wallet and node code in bitcoin#10244

@PastaPastaPasta PastaPastaPasta marked this pull request as draft April 16, 2020 01:52
@PastaPastaPasta PastaPastaPasta mentioned this pull request Apr 16, 2020
Merged
@PastaPastaPasta PastaPastaPasta force-pushed the backport-13417 branch 2 times, most recently from 35e67cb to 334f68c Compare April 20, 2020 03:34
@PastaPastaPasta PastaPastaPasta marked this pull request as ready for review April 20, 2020 04:26
@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Apr 20, 2020

I am unable to reproduce these test failures locally

@UdjinM6
Copy link

UdjinM6 commented Apr 24, 2020 •
edited
Loading

There is a merge conflict because of d235364 - that this 29eada8#diff-6becc62f79dca411ca82cc6c57b76030R77 (in 14733) should be bumped to 3 while doing so to make tests pass. Moreover it looks like 14024 (which 14733 relies on too btw) requires at least 13022, probably more. If there are many more PRs need to be backported I guess it might be a good idea to just drop these two and backport them later with other required PRs in correct order.

@PastaPastaPasta PastaPastaPasta changed the title Backport More P2P [0.17] Backport More P2P Apr 25, 2020
@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Apr 25, 2020

I'm gonna delay this until 0.16 is branched.

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented May 12, 2020

Just so I don't forget, when I force-pushed two days ago, dropped14024 and 14733.

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented May 12, 2020

rebased on develop

@PastaPastaPasta PastaPastaPasta added this to the 17 milestone May 14, 2020
@PastaPastaPasta PastaPastaPasta mentioned this pull request May 17, 2020
Merged
sipa and others added 12 commits June 8, 2020 20:27
@sipa @PastaPastaPasta
Merge bitcoin#13417: [net] Tighten scope in net_processing
5b72c19 
3339ba2 Make g_enable_bip61 a member variable of PeerLogicValidation (Jesse Cohen)
6690a28 Restrict as much as possible in net_processing to translation unit (Jesse Cohen)
1d4df02 [move-only] Move things only referenced in net_processing out of header file (Jesse Cohen)
02bbc05 Rescope g_enable_bip61 to net_processing (Jesse Cohen)

Pull request description:

  As part of a larger effort to decouple net_processing and validation a bit, these are a bunch of simple scope cleanups. I've moved things out of the header file that are only referenced in net_processing and added static (or anonymous namespace) modifiers to everything possible in net_processing.

  There are a handful of functions which could be static except that they are exposed for the sake of unit testing - these are explicitly commented. There has been some discussion of a compile time annotation, but no conclusion has been reached on that yet.

  This is somewhat related to other prs bitcoin#12934 bitcoin#13413 bitcoin#13407 and will be followed by prs that reduce reliance on cs_main to synchronize data structures which are translation unit local to net_processing

Tree-SHA512: 46c9660ee4e06653feb42ba92189565b0aea17aac2375c20747c0d091054c63829cbf66d2daddf65682b58ce1d6922e23aefea051a7f2c8abbb6db253a609082
Signed-off-by: Pasta <pasta@dashboost.org>

# Conflicts:
#	src/init.cpp
#	src/net_processing.cpp
#	src/net_processing.h
#	src/test/test_dash.cpp
@PastaPastaPasta
Adjust STALE_CHECK_INTERVAL to be correct for Dash
67c0459 
Signed-off-by: Pasta <pasta@dashboost.org>
@PastaPastaPasta
move Misbehaving declaration into the header since we use it outside …
eb33bd1 
…of net_processing.cpp

Signed-off-by: Pasta <pasta@dashboost.org>
@PastaPastaPasta
comment out extern declaration in DoS_tests.cpp
ea3871e 
Signed-off-by: Pasta <pasta@dashboost.org>
@PastaPastaPasta
fix bip 61 in Dash Specific code, 13417 continued
be08490 
Signed-off-by: Pasta <pasta@dashboost.org>
@laanwj @PastaPastaPasta
Merge bitcoin#13423: [net] Thread safety annotations in net_processing
19ac12e 
1e3bcd2 [net_processing] Add thread safety annotations (Jesse Cohen)
f393a53 Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety analysis (Jesse Cohen)

Pull request description:

  (note that this depends on bitcoin#13417)
  This commit fully annotates all globals in net_processing with clang thread safety annotations. Subsequent commits will begin transitioning some of this data away from cs_main into locks that are local to net_processing. Static thread safety analysis should it easier to verify correctness of that process.

Tree-SHA512: b47aa410cb9ada21072370176aea9a74c575643fa1ee8cf1d43c8e28675eef17f33e5242ac422f840e8178e132ecb58412034c6334b68f1b57c686df80d4e8e2
@PastaPastaPasta
add EXCLUSIVE_LOCKS_REQUIRED(cs_main) for Misbehaving and include val…
409238b 
…idation.h

Signed-off-by: Pasta <pasta@dashboost.org>
@laanwj @PastaPastaPasta
Merge bitcoin#13532: Log warning when deprecated network name 'tor' i…
0f7f679 
…s used

9f8c54b Log warning message when deprecated network name 'tor' is used (e.g. option onlynet=tor) (wodry)

Pull request description:

  As @laanwj mentioned [here](bitcoin#13418 (comment)), using option `onlynet=tor` is deprecated.

  I think it would be good to give the user a depcreaction warning feedback, so users can switch to `onlynet=onion` so there is a perspective for removing the deprecated `tor` in the future to decrease confusion.

  Currently, users maybe just wonder that they can use a undocumented option, or they are not aware that they use a deprecated option.

  Alternatively for the log warning message, I think at least this deprecetaion should be documented in the source code in a comment for readers of the source code.

Tree-SHA512: f4889793cdd62a0a13353e13994ed50ca7d367fa9da9897ce909f86cf0b0ce6151b3c484c8e514b8ac332949c6bbc71001e06e918248a1089f73756bd4840602
@jonasschnelli @PastaPastaPasta
Merge bitcoin#13537: GUI: Peer table: Visualize inbound/outbound stat…
28a62b0 
…e for every row

4132ad3 Show symbol for inbound/outbound in peer table (wodry)

Pull request description:

  Fixes bitcoin#13483

  The address in the network peer table is prefixed with an up-arrow symbolizing an outbound connection, or an down-array symbolizing an inbound connection. See screenshot.

  The user has an easy visual confirmation about the connection direction state. I really like it :)
  Impact to columns sorting is grouping by inbound/outbound first, which in my opinion is an advantage, too.
  ![bildschirmfoto](https://user-images.githubusercontent.com/8447873/41862752-13803eb2-78a5-11e8-9126-a52385f5ec19.png)

Tree-SHA512: d355f679d34c3006743c06750be5f36a083c1a8376da8f5f35045fcd9df964153409946fdde5007734f23bd692c91355962dc42df31122cdcf88e4affce8bc0e
@PastaPastaPasta
scripted-diff: Replace NET_TOR with NET_ONION (bitcoin#13575)
f0e5ef0 
-BEGIN VERIFY SCRIPT-

sed --in-place'' --expression='s/NET_TOR/NET_ONION/g' $(git grep -I --files-with-matches 'NET_TOR')

-END VERIFY SCRIPT-

Signed-off-by: Pasta <pasta@dashboost.org>
@laanwj @PastaPastaPasta
Merge bitcoin#13672: Modified in_addr6 cast in CConman class to work …
02ed3cb 
…with msvc

822a2a3 Modified in_addr6 cast in CConman class to work with msvc. (Aaron Clauson)

Pull request description:

  Fix to allow net.cpp to compile with MSVC. Without this fix the `(in6_addr)IN6ADDR_ANY_INIT` implicit cast generates a compilation error.

Tree-SHA512: f21c5002401dc93564dcf8d49fbafe7c03ad4182df1616d2ee201e2e172f1d696ca7982fb5b42a3b7d6878c8649823044a858401b4172239fb4b0cc2a38db282
@laanwj @PastaPastaPasta
Merge bitcoin#13776: net: Add missing verification of IPv6 address in…
396e9b6 
… CNetAddr::GetIn6Addr(...)

2fb0066 net: Add missing verification of IPv6 address in CNetAddr::GetIn6Addr(...) (practicalswift)

Pull request description:

  Add missing verification of IPv6 address in `CNetAddr::GetIn6Addr(...)`.

Tree-SHA512: 8b0681252eec9cf293a2043e99fdacec520e321b477d8aca1cbd6327b85bf6c5e8cd820fb914f097c62655947e88745ebccd824a78b995a8186e910e6fe520aa
laanwj and others added 6 commits June 8, 2020 20:27
@laanwj @PastaPastaPasta
Merge bitcoin#13907: Introduce a maximum size for locators.
63b1269 
e254ff5 Introduce a maximum size for locators. (Gregory Maxwell)

Pull request description:

  The largest sensible size for a locator is log in the number of blocks.
   But, as noted by Coinr8d on BCT a maximum size message could encode a
   hundred thousand locators.  If height were used to limit the messages
   that could open new attacks where peers on long low diff forks would
   get disconnected and end up stuck.

  Ideally, nodes first first learn to limit the size of locators they
   send before limiting what would be processed, but common implementations
   back off with an exponent of 2 and have an implicit limit of 2^32
   blocks, so they already cannot produce locators over some size.

  Locators are cheap to process so allowing a few more is harmless,
   so this sets the maximum to 64-- which is enough for blockchains
   with 2^64 blocks before the get overhead starts increasing.

Tree-SHA512: da28df9c46c988980da861046c62e6e7f93d0eaab3083d32e408d1062f45c00316d5e1754127e808c1feb424fa8e00e5a91aea2cc3b80326b71c148696f7cdb3
@PastaPastaPasta
Merge bitcoin#14025: p2p: Remove dead code for nVersion=10300
2fa446f 
fa74d3d qa: Remove unused deserialization code in msg_version (MarcoFalke)
fa5099c p2p: Remove dead code for nVersion=10300 (MarcoFalke)

Pull request description:

  This code is undocumented and confusing as well as dead, since peers with a version that old are disconnected immediately.

Tree-SHA512: 58c131a2730b630ffdc191cd65fe736ed1bd57e184902e2af1b1399443c4654617e68774432016df023434055e85d2e8cd32fb03b40c508c3bb8db6d19427434
@PastaPastaPasta
Merge bitcoin#14027: Skip stale tip checking if outbound connections …
490ef35 
…are off or if reindexing.

66b3fc5 Skip stale tip checking if outbound connections are off or if reindexing. (Gregory Maxwell)

Pull request description:

  I got tired of the pointless stale tip notices in reindex and on nodes with connections disabled.

Tree-SHA512: eb07d9c5c787ae6dea02cdd1d67a48a36a30adc5ccc74d6f1c0c7364d404dc8848b35d2b8daf5283f7c8f36f1a3c463aacb190d70a22d1fe796a301bb1f03228
@laanwj @PastaPastaPasta
Merge bitcoin#14140: [Net] Switch nPrevNodeCount to vNodesSize
90ddef5 
83d5305 Switch nPrevNodeCount to vNodesSize. (Patrick Strateman)

Pull request description:

  These both have the same value, but the variable naming is confusing.

Tree-SHA512: 4f645e89efdc69884ff4c8bbcf42e2b35d2733687c0fc6ab3f0797e0141fe23ef9cde8bb6ba422f47a88f554e55a099b1f0b3f47cb9fde12db3d46b9a0041bb0
@laanwj @PastaPastaPasta
Merge bitcoin#14532: Never bind INADDR_ANY by default, and warn when …
f08419f 
…doing so explicitly

27c44ef rpcbind: Warn about exposing RPC to untrusted networks (Luke Dashjr)
d6a1287 CNetAddr: Add IsBindAny method to check for INADDR_ANY (Luke Dashjr)
3615003 net: Always default rpcbind to localhost, never "all interfaces" (Luke Dashjr)

Pull request description:

  A disturbingly large number of listening nodes appear to be also exposing their RPC server to the public internet. To attempt to mitigate this:

  * Only ever bind localhost by default, even if `rpcallowip` is specified. (A warning is given if `rpcallowip` is specified without `rpcbind`, since it doesn't really make sense to do.)
  * Warn about exposing the RPC server to untrusted networks if the user explicitly binds to any INADDR_ANY address.
  * Include a warning about untrusted networks in the `--help` documentation for `rpcbind`.

Tree-SHA512: 755bbca3db416a31393672eccf6675a5ee4d1eb1812cba73ebb4ff8c6b855ecc5df4c692566e9aa7b0f7d4dce6fedb9c0e9f3c265b9663aca36c4a6ba5efdbd4
@laanwj @PastaPastaPasta
Merge bitcoin#14685: fix a deserialization overflow edge case
0f2b3f2 
b08af10 disallow oversized CBlockHeaderAndShortTxIDs (Kaz Wesley)
6bed4b3 fix a deserialization overflow edge case (Kaz Wesley)
051faf7 add a test demonstrating an overflow in a deserialization edge case (Kaz Wesley)

Pull request description:

  A specially-constructed BlockTransactionsRequest can cause `offset` to wrap in deserialization. In the current code, there is not any way this could be dangerous; but disallowing it reduces the potential for future surprises.

Tree-SHA512: 1aaf7636e0801a905ed8807d0d1762132ac8b4421a600c35fb6d5e5033c6bfb587d8668cd9f48c7a08a2ae793a677b7649661e3ae248ab4f8499ab7b6ede483c
@PastaPastaPasta PastaPastaPasta changed the title [0.17] Backport More P2P Backport More P2P Jun 9, 2020
@PastaPastaPasta PastaPastaPasta requested a review from UdjinM6 June 9, 2020 05:25
@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Jun 9, 2020

test failure likely unrelated

@laanwj @PastaPastaPasta
Merge bitcoin#14728: fix uninitialized read when stringifying an addr…
0211eec 
…Local

b7b36de fix uninitialized read when stringifying an addrLocal (Kaz Wesley)
8ebbef0 add test demonstrating addrLocal UB (Kaz Wesley)

Pull request description:

  Reachable from either place where SetIP is used when all of:
  - our best-guess addrLocal for a peer is IPv4
  - the peer tells us it's reaching us at an IPv6 address
  - NET logging is enabled

  In that case, SetIP turns an IPv4 address into an IPv6 address without
  setting the scopeId, which is subsequently read in GetSockAddr during
  CNetAddr::ToStringIP and passed to getnameinfo. Fix by ensuring every
  constructor initializes the scopeId field with something.

Tree-SHA512: 8f0159750995e08b985335ccf60a273ebd09003990bcf2c3838b550ed8dc2659552ac7611650e6dd8e29d786fe52ed57674f5880f2e18dc594a7a863134739e3
UdjinM6
UdjinM6 approved these changes Jun 10, 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.

Looks good 👍

utACK

@PastaPastaPasta PastaPastaPasta mentioned this pull request Jun 10, 2020
Merged
@UdjinM6 UdjinM6 merged commit d750b86 into dashpay:develop Jun 11, 2020
@PastaPastaPasta PastaPastaPasta deleted the backport-13417 branch June 11, 2020 15:49
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

17

Development

Successfully merging this pull request may close these issues.

5 participants

@PastaPastaPasta @UdjinM6 @sipa @laanwj @jonasschnelli