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.

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

backport: Merge bitcoin#19316, (Partial) 19725, 19724 #5771

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

vijaydasmp
Copy link

@vijaydasmp vijaydasmp commented Dec 17, 2023

Bitcoin Backports

@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#19316 backport: Merge bitcoin#19316, 19725 Dec 17, 2023
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#19316, 19725 backport: Merge bitcoin#19316, 19725, 19724 Dec 18, 2023
@vijaydasmp
Copy link
Author

@UdjinM6 ci not triggering

@vijaydasmp vijaydasmp force-pushed the bp22_10_1 branch 3 times, most recently from 0eee8f8 to d6a4d4d Compare December 20, 2023 09:59
@thephez thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Dec 20, 2023
@vijaydasmp vijaydasmp force-pushed the bp22_10_1 branch 2 times, most recently from 5b6e6ee to e0d2088 Compare December 21, 2023 05:23
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#19316, 19725, 19724 backport: Merge bitcoin#19316, (partial) 19725, 19724 Dec 21, 2023
@vijaydasmp vijaydasmp force-pushed the bp22_10_1 branch 3 times, most recently from 2fc3ab4 to fdf5372 Compare December 21, 2023 08:30
@vijaydasmp vijaydasmp marked this pull request as ready for review December 21, 2023 10:49
@vijaydasmp
Copy link
Author

@vijaydasmp vijaydasmp marked this pull request as draft December 21, 2023 10:50
@vijaydasmp vijaydasmp marked this pull request as ready for review December 21, 2023 12:33
@vijaydasmp
Copy link
Author

vijaydasmp commented Dec 21, 2023

Hello @UdjinM6, @PastaPastaPasta, @knst, please review

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

I found several missing changes, please check them and let's do bitcoin#19725 not-partial.

@@ -3617,7 +3621,7 @@ bool CConnman::IsMasternodeQuorumNode(const CNode* pnode)
// Let's see if this is an outgoing connection to an address that is known to be a masternode
// We however only need to know this if the node did not authenticate itself as a MN yet
uint256 assumedProTxHash;
if (pnode->GetVerifiedProRegTxHash().IsNull() && !pnode->fInbound) {
if (pnode->GetVerifiedProRegTxHash().IsNull() && !pnode->IsInboundConn()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing changes above:

+// Initiate outbound connections from -addnode
-// Initiate manual connections

Copy link
Author

Choose a reason for hiding this comment

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

ok

src/net.cpp Outdated Show resolved Hide resolved
src/rpc/net.cpp Outdated
@@ -203,7 +205,10 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
// their ver message.
obj.pushKV("subver", stats.cleanSubVer);
obj.pushKV("inbound", stats.fInbound);
obj.pushKV("addnode", stats.m_manual_connection);
if (IsDeprecatedRPCEnabled("getpeerinfo_addnode")) {
// addnode is deprecated in v0.21 for removal in v0.22
Copy link
Collaborator

@knst knst Dec 26, 2023

Choose a reason for hiding this comment

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

-// addnode is deprecated in v0.21 for removal in v0.22
+// addnode is deprecated in v20.1/v21 for removal in v21/v22

Copy link
Author

Choose a reason for hiding this comment

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

got it

@@ -580,6 +580,26 @@ bool CNode::IsBlockRelayOnly() const {
return (ignores_incoming_txs && !HasPermission(PF_RELAY)) || !IsAddrRelayPeer();
}

std::string CNode::ConnectionTypeAsString() const
Copy link
Collaborator

Choose a reason for hiding this comment

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

for bitcoin#19725 please create a new file doc/release-notes-19725.md
and put missing changes there.

In this case backport 19725 is not partial anymore, can remove this mark, is it?

@@ -2988,6 +3003,7 @@ void PeerManagerImpl::ProcessMessage(
m_connman.PushMessage(&pfrom, CNetMsgMaker(nSendVersion).Make(NetMsgType::GETADDR));
pfrom.fGetAddr = true;
m_addrman.Good(pfrom.addr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing comment:


            // Moves address from New to Tried table in Addrman, resolves
            // tried-table collisions, etc.

Copy link
Author

Choose a reason for hiding this comment

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

got it

@@ -1215,7 +1263,7 @@ class CNode

// flood relay
std::vector<CAddress> vAddrToSend;
std::unique_ptr<CRollingBloomFilter> m_addr_known = nullptr;
std::unique_ptr<CRollingBloomFilter> m_addr_known{nullptr};
Copy link
Collaborator

Choose a reason for hiding this comment

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

(see net.h:1274)
we still have IsAddrRelay usages over code base.
Please, drop remaining usages and this method; update related comment also:

-// in dash: m_tx_relay should never be nullptr, use `IsAddrRelayPeer() == false` instead
+// in dash: m_tx_relay should never be nullptr, use `RelayAddrsWithConn() == false` instead

Copy link
Author

Choose a reason for hiding this comment

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

got it

@vijaydasmp vijaydasmp force-pushed the bp22_10_1 branch 2 times, most recently from cdbb1ed to c4b9eea Compare December 29, 2023 05:38
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#19316, (partial) 19725, 19724 backport: Merge bitcoin#19316, 9725, 19724 Dec 29, 2023
@vijaydasmp vijaydasmp requested a review from knst December 29, 2023 09:12
@vijaydasmp
Copy link
Author

@knst knst changed the title backport: Merge bitcoin#19316, 9725, 19724 backport: Merge bitcoin#19316, 19725, 19724 Dec 29, 2023
knst
knst previously approved these changes Dec 29, 2023
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK

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.

pls see below and also 19725 (rpc) is a breaking change, so this should not be merged yet

src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/test/fuzz/util.h Outdated Show resolved Hide resolved
src/net.cpp Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
knst
knst previously approved these changes Jan 6, 2024
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

re-utACK

UdjinM6
UdjinM6 previously approved these changes Jan 6, 2024
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 with one nit

test/functional/rpc_net.py Outdated Show resolved Hide resolved
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.

re-utACK

@vijaydasmp
Copy link
Author

Hello @PastaPastaPasta , requesting review

@PastaPastaPasta
Copy link
Member

Why can 19725 and 20188 not be done in full? Why are they valuable to do partially?

@knst
Copy link
Collaborator

knst commented Jan 7, 2024

Why can 19725 and 20188 not be done in full? Why are they valuable to do partially?

@vijaydasmp
Copy link
Author

In this PR I have marked 20188 as partial as this pr has only follow up changes for it, Shall I remove the partial tag from it, requesting suggestion

@knst
Copy link
Collaborator

knst commented Jan 8, 2024

I usually write something like this commit e2fe1a2

fix: follow-up missing changes from Merge bitcoin#20413: build: Require C++17 compiler

Or da212ad

fix: follow-up Merge bitcoin#17381: LegacyScriptPubKeyMan code cleanups - now it's possible to remove workaround

Sorry for my English if grammar in mentioned examples is incorrect

@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#19316, (Partial) 19725, 19724, (partial) 20188 backport: Merge bitcoin#19316, (Partial) 19725, 19724 Jan 8, 2024
@vijaydasmp
Copy link
Author

Updated commit message for [fix: follow-up missing changes from bitcoin#20188],
requesting review @knst @PastaPastaPasta

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for merging via merge commit

fanquake and others added 4 commits January 9, 2024 08:15
01e2830 [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar)
bc5d65b [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar)
2f2e13b [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar)
7f7b83d [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar)
35839e9 [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar)
4972c21 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar)
60156f5 [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar)
7b322df [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar)
1492342 [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar)
49efac5 [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar)
d3698b5 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar)
46578c0 [doc] Describe different connection types (Amiti Uttarwar)
442abae [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar)
af59feb [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar)
e1bc298 [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar)
0e52a65 [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar)
1521c47 [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar)
26304b4 [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar)
3f1b714 scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar)

Pull request description:

  **This is part 1 of bitcoin#19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality.

  **This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions.

  This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types.
  (some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.)

  Overview of this PR:
  * rename `oneshot` to `addrfetch`
  * introduce `ConnectionType` enum
  * one by one, add different connection types to the enum
  * expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts)
  * fix the bug in counting different type of connections
  * some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive.

ACKs for top commit:
  jnewbery:
    utACK 01e2830
  laanwj:
    Code review ACK 01e2830, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall
  vasild:
    ACK 01e2830
  sdaftuar:
    ACK 01e2830.
  fanquake:
    ACK 01e2830 - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now.
  jb55:
    wow this code was messy before... ACK 01e2830

Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
…fo, improve logs

a512925 [doc] Release notes (Amiti Uttarwar)
50f94b3 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9 [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9 [log] Add connection type to log statement (Amiti Uttarwar)

Pull request description:

  After bitcoin#19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.

  This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.

  Suggested by sdaftuar- bitcoin#19316 (comment) & bitcoin#19316 (comment)

ACKs for top commit:
  jnewbery:
    Code review ACK a512925.
  sipa:
    utACK a512925
  guggero:
    Tested and code review ACK a512925.
  MarcoFalke:
    cr ACK a512925 🌇
  promag:
    Code review ACK a512925.

Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
eb1c5d0 [doc] Follow developer notes, add comment about missing default. (Amiti Uttarwar)
d5a57ce [doc] Describe connection types in more depth. (Amiti Uttarwar)
4829b6f [refactor] Simplify connection type logic in ThreadOpenConnections (Amiti Uttarwar)
1e563ae [refactor] Simplify check for block-relay-only connection. (Amiti Uttarwar)
da3a0be [test] Add explicit tests that connection types get set correctly (Amiti Uttarwar)
1d74fc7 [trivial] Small style updates (Amiti Uttarwar)
ff6b908 [doc] Explain address handling logic in process messages (Amiti Uttarwar)
dff16b1 [refactor] Restructure logic to check for addr relay. (Amiti Uttarwar)
a6ab1e8 [net] Remove unnecessary default args on OpenNetworkConnection (Amiti Uttarwar)
8d6ff46 scripted-diff: Rename `OUTBOUND` ConnectionType to `OUTBOUND_FULL_RELAY` (Amiti Uttarwar)

Pull request description:

  This PR addresses outstanding review comments from bitcoin#19316. It further simplifies `net.cpp` complexity and adds documentation about design goals about different connection types.

ACKs for top commit:
  naumenkogs:
    ACK eb1c5d0
  laanwj:
    Code review ACK eb1c5d0

Tree-SHA512: 2fe14e428404c95661e5518c8c90db07ab5b9ebb1bac921b3bdf82b181f444fae379f8fc0a2b619e6b4693f01c55bd246fbd8c8eedadd96849a30de3161afee5
@PastaPastaPasta PastaPastaPasta merged commit 3c7c283 into dashpay:develop Jan 9, 2024
4 of 5 checks passed
kwvg added a commit to kwvg/dash that referenced this pull request Mar 25, 2024
comment was moved to net.h in 678df63 (dashpay#4888) and removed entirely in
796353a (dashpay#5771). the comment is being restored back to where it is
upstream, in CNode::RelayAddrsWithConn.
kwvg added a commit to kwvg/dash that referenced this pull request Mar 27, 2024
comment was moved to net.h in 678df63 (dashpay#4888) and removed entirely in
796353a (dashpay#5771). the comment is being restored back to where it is
upstream, in CNode::RelayAddrsWithConn.
kwvg added a commit to kwvg/dash that referenced this pull request Apr 2, 2024
comment was moved to net.h in 678df63 (dashpay#4888) and removed entirely in
796353a (dashpay#5771). the comment is being restored back to where it is
upstream, in CNode::RelayAddrsWithConn.
kwvg added a commit to kwvg/dash that referenced this pull request Apr 3, 2024
comment was moved to net.h in 678df63 (dashpay#4888) and removed entirely in
796353a (dashpay#5771). the comment is being restored back to where it is
upstream, in CNode::RelayAddrsWithConn.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants