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

vijaydasmp · 2023-12-17T14:13:45Z

Bitcoin Backports

vijaydasmp · 2023-12-19T01:22:59Z

@UdjinM6 ci not triggering

vijaydasmp · 2023-12-21T10:50:43Z

pipeline: https://gitlab.com/dashpay/dash/-/pipelines/1115319634

vijaydasmp · 2023-12-21T12:33:36Z

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

knst

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

knst · 2023-12-26T09:16:16Z

src/net.cpp

@@ -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()) {


missing changes above:

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

src/net.cpp

knst · 2023-12-26T09:31:27Z

src/rpc/net.cpp

@@ -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


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

knst · 2023-12-26T10:25:02Z

src/net.cpp

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

+std::string CNode::ConnectionTypeAsString() const


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?

knst · 2023-12-26T10:30:00Z

src/net_processing.cpp

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


missing comment:

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

knst · 2023-12-26T10:32:52Z

src/net.h

@@ -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};


(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

vijaydasmp · 2023-12-29T11:41:45Z

Hello @knst @UdjinM6 @PastaPastaPasta , requesting review
CI: https://gitlab.com/dashpay/dash/-/pipelines/1122077152

knst

utACK

UdjinM6

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

src/net.cpp

src/test/fuzz/util.h

src/net.cpp

src/net_processing.cpp

knst

re-utACK

UdjinM6

utACK with one nit

test/functional/rpc_net.py

UdjinM6

re-utACK

vijaydasmp · 2024-01-06T17:00:43Z

Hello @PastaPastaPasta , requesting review

PastaPastaPasta · 2024-01-07T01:38:19Z

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

knst · 2024-01-07T06:10:38Z

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

20188 is already done full in the past; this PR has only some follow-up fixes due to new code in 19316 that has been missing here backport: bitcoin#18736, #19379, #19296, #18775, #20188, #19558, #18445, #18744, #19067, #19065 (fuzzing harness backports: part 4) #4987
19725 is RPC for 19316, but we can't make it full due to breaking changes (deprecating old rpc), so, partial merge which introduce new functionality.

vijaydasmp · 2024-01-08T05:27:45Z

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 · 2024-01-08T05:49:42Z

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 · 2024-01-08T16:51:47Z

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

PastaPastaPasta

utACK for merging via merge commit

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

…uzzing harness for CConnman

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.

vijaydasmp force-pushed the bp22_10_1 branch from 30cbf3b to 2ed22ae Compare December 17, 2023 15:14

vijaydasmp changed the title ~~backport: Merge bitcoin#19316~~ backport: Merge bitcoin#19316, 19725 Dec 17, 2023

vijaydasmp changed the title ~~backport: Merge bitcoin#19316, 19725~~ backport: Merge bitcoin#19316, 19725, 19724 Dec 18, 2023

vijaydasmp force-pushed the bp22_10_1 branch from 23d6c4f to 5aa2b1e Compare December 18, 2023 17:12

vijaydasmp force-pushed the bp22_10_1 branch 3 times, most recently from 0eee8f8 to d6a4d4d Compare December 20, 2023 09:59

thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Dec 20, 2023

vijaydasmp force-pushed the bp22_10_1 branch 2 times, most recently from 5b6e6ee to e0d2088 Compare December 21, 2023 05:23

vijaydasmp changed the title ~~backport: Merge bitcoin#19316, 19725, 19724~~ backport: Merge bitcoin#19316, (partial) 19725, 19724 Dec 21, 2023

vijaydasmp force-pushed the bp22_10_1 branch 3 times, most recently from 2fc3ab4 to fdf5372 Compare December 21, 2023 08:30

vijaydasmp marked this pull request as ready for review December 21, 2023 10:49

vijaydasmp marked this pull request as draft December 21, 2023 10:50

vijaydasmp force-pushed the bp22_10_1 branch from fdf5372 to 9b579af Compare December 21, 2023 11:42

vijaydasmp marked this pull request as ready for review December 21, 2023 12:33

knst requested changes Dec 26, 2023

View reviewed changes

vijaydasmp force-pushed the bp22_10_1 branch 2 times, most recently from cdbb1ed to c4b9eea Compare December 29, 2023 05:38

vijaydasmp changed the title ~~backport: Merge bitcoin#19316, (partial) 19725, 19724~~ backport: Merge bitcoin#19316, 9725, 19724 Dec 29, 2023

vijaydasmp requested a review from knst December 29, 2023 09:12

knst changed the title ~~backport: Merge bitcoin#19316, 9725, 19724~~ backport: Merge bitcoin#19316, 19725, 19724 Dec 29, 2023

knst previously approved these changes Dec 29, 2023

View reviewed changes

UdjinM6 requested changes Dec 29, 2023

View reviewed changes

vijaydasmp dismissed knst’s stale review via 0553c85 January 6, 2024 08:01

vijaydasmp force-pushed the bp22_10_1 branch from 22aa478 to 0553c85 Compare January 6, 2024 08:01

knst previously approved these changes Jan 6, 2024

View reviewed changes

vijaydasmp requested a review from UdjinM6 January 6, 2024 08:34

UdjinM6 previously approved these changes Jan 6, 2024

View reviewed changes

test/functional/rpc_net.py Outdated Show resolved Hide resolved

vijaydasmp dismissed stale reviews from UdjinM6 and knst via 5c747bf January 6, 2024 16:28

vijaydasmp force-pushed the bp22_10_1 branch from 0553c85 to 5c747bf Compare January 6, 2024 16:28

vijaydasmp requested review from knst and UdjinM6 January 6, 2024 16:31

UdjinM6 approved these changes Jan 6, 2024

View reviewed changes

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

vijaydasmp force-pushed the bp22_10_1 branch from 5c747bf to 3d803b6 Compare January 8, 2024 16:48

PastaPastaPasta approved these changes Jan 9, 2024

View reviewed changes

fanquake and others added 4 commits January 9, 2024 08:15

fix: follow-up missing changes from Merge bitcoin#20188: tests: Add f…

aa74d0b

…uzzing harness for CConnman

PastaPastaPasta force-pushed the bp22_10_1 branch from 3d803b6 to aa74d0b Compare January 9, 2024 14:15

PastaPastaPasta merged commit 3c7c283 into dashpay:develop Jan 9, 2024

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

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

Uh oh!

Conversation

vijaydasmp commented Dec 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vijaydasmp commented Dec 19, 2023

Uh oh!

vijaydasmp commented Dec 21, 2023

Uh oh!

vijaydasmp commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knst Dec 26, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vijaydasmp commented Dec 29, 2023

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

vijaydasmp commented Jan 6, 2024

Uh oh!

PastaPastaPasta commented Jan 7, 2024

Uh oh!

knst commented Jan 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vijaydasmp commented Jan 8, 2024

Uh oh!

knst commented Jan 8, 2024

Uh oh!

vijaydasmp commented Jan 8, 2024

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vijaydasmp commented Dec 17, 2023 •

edited

Loading

vijaydasmp commented Dec 21, 2023 •

edited

Loading

knst Dec 26, 2023 •

edited

Loading

knst commented Jan 7, 2024 •

edited

Loading