refactor: Continue moving application data from CNode to Peer #26621

dergoegge commented at 5:26 PM on December 1, 2022: member

This PR picks up a subset of changes from #24970 and additionally moves m_bip152_highbandwith{to,from}, nTimeOffset, nVersion, m_greates_common_version.

DrahtBot commented at 5:26 PM on December 1, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	fanquake
Concept ACK	theStack, mzumsande, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/754 (Add BIP324 "Transport" label to peer details by hebasto)
#bitcoin-core/gui/677 (Bugfix: Address broken things around Peers detail view by luke-jr)
#28464 (net: improve max-connection limits code by mzumsande)
#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
#28423 (kernel: Remove protocol.h/netaddress.h/compat.h from kernel headers by TheCharlatan)
#28331 (BIP324 integration by sipa)
#28248 (p2p: peer connection bug fixes, logic and logging improvements by jonatack)
#28170 (p2p: adaptive connections services flags by furszy)
#28078 (net, refactor: remove unneeded exports, use helpers over low-level code, use optional by jonatack)
#28031 (Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module by glozow)
#27837 (net: introduce block tracker to retry to download blocks after failure by furszy)
#27581 (net: Continuous ASMap health check by fjahr)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
#26326 (net: don't lock cs_main while reading blocks in net processing by andrewtoth)
#26283 (p2p: Fill reconciliation sets and request reconciliation (Erlay) by naumenkogs)
#21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Refactoring on Dec 1, 2022

dergoegge force-pushed on Dec 1, 2022

DrahtBot added the label Needs rebase on Dec 2, 2022

dergoegge force-pushed on Dec 5, 2022

DrahtBot removed the label Needs rebase on Dec 5, 2022

in src/rpc/net.cpp:125 in f5498810dc outdated

 121 | @@ -122,15 +122,15 @@ static RPCHelpMan getpeerinfo()
 122 |                      {RPCResult::Type::NUM, "bytessent", "The total bytes sent"},
 123 |                      {RPCResult::Type::NUM, "bytesrecv", "The total bytes received"},
 124 |                      {RPCResult::Type::NUM_TIME, "conntime", "The " + UNIX_EPOCH_TIME + " of the connection"},
 125 | -                    {RPCResult::Type::NUM, "timeoffset", "The time offset in seconds"},
 126 | +                    {RPCResult::Type::NUM, "timeoffset", /*optional=*/true, "The time offset in seconds"},

maflcko commented at 3:42 PM on December 5, 2022:

Not sure if changing the type to optional is a refactor. There are already several conflicting pulls out there trying to change the types here from optional back or forth, so I think it would be better to not have another one. For now it might be best to make this a "real" refactor, keep the default value, and let other pulls decide if it is wrong or not.

dergoegge commented at 3:11 PM on December 19, 2022:

Rebased this on the now merged #26515, so the changing to optional was no longer needed.

dergoegge force-pushed on Dec 5, 2022

luke-jr commented at 8:53 AM on December 7, 2022: member

What's the goal of this refactor? Seems like the "refactor moratorium" should apply?

dergoegge commented at 11:10 AM on December 7, 2022: member

What's the goal of this refactor?

This is continuing the work outlined here: https://github.com/bitcoin/bitcoin/issues/19398

DrahtBot added the label Needs rebase on Dec 19, 2022

dergoegge force-pushed on Dec 19, 2022

DrahtBot removed the label Needs rebase on Dec 19, 2022

dergoegge force-pushed on Jan 17, 2023

DrahtBot added the label Needs rebase on Jan 18, 2023

fanquake commented at 12:15 PM on January 18, 2023: member

Concept ACK

dergoegge force-pushed on Jan 18, 2023

dergoegge commented at 12:23 PM on January 18, 2023: member

Rebased.

DrahtBot removed the label Needs rebase on Jan 18, 2023

fanquake commented at 4:48 PM on January 18, 2023: member

@mzumsande @ajtowns @theStack Any Concept ACK/NACK or thoughts here?

ajtowns commented at 9:57 PM on January 18, 2023: contributor

@fanquake: drahtbot is misinterpreting your question :)

I think moving these is a good idea. The amount of code touched and the number of conflicting PRs is annoying though.

Might be better to move the elements individually when they're useful for some patch that makes an observable change, though, rather than in a dedicated refactoring pr with no immediate benefit?

theStack commented at 6:48 AM on January 19, 2023: contributor

Concept ACK

in test/functional/p2p_compactblocks_hb.py:20 in 52e5ce2762 outdated

  16 | @@ -17,9 +17,9 @@ def set_test_params(self):
  17 |  
  18 |      def peer_info(self, from_node, to_node):
  19 |          """Query from_node for its getpeerinfo about to_node."""
  20 | -        for peerinfo in self.nodes[from_node].getpeerinfo():
  21 | -            if "(testnode%i)" % to_node in peerinfo['subver']:
  22 | -                return peerinfo
  23 | +        for peer in self.nodes[from_node].getpeerinfo():

fanquake commented at 2:11 PM on January 19, 2023:

In 52e5ce2762cb025a8d7d6b61e1121f9a09367689: The changes to the tests to tolerate a missing subver should no-longer be needed now that these fields are non-optional? Commit message also mentions marking the field optional, but that no-longer happens (and doesn't need to).

dergoegge commented at 11:48 AM on February 2, 2023:

Right, done.

in src/net_processing.cpp:188 in c25f0f1719 outdated

 183 | @@ -184,6 +184,8 @@ static constexpr double MAX_ADDR_RATE_PER_SECOND{0.1};
 184 |  static constexpr size_t MAX_ADDR_PROCESSING_TOKEN_BUCKET{MAX_ADDR_TO_SEND};
 185 |  /** The compactblocks version we support. See BIP 152. */
 186 |  static constexpr uint64_t CMPCTBLOCKS_VERSION{2};
 187 | +/** Seed for the randomizer for the nonce field in version messages. SHA256("localhostnonce")[0:8] */
 188 | +static constexpr uint64_t RANDOMIZER_ID_LOCALHOSTNONCE = 0xd93e69e2bbfa5735ULL;

fanquake commented at 2:13 PM on January 19, 2023:

In c25f0f1719fbda24a9dbfeb8e1c9d253d55807c2 could:

static constexpr uint64_t RANDOMIZER_ID_LOCALHOSTNONCE{0xd93e69e2bbfa5735UL};

in src/net_processing.cpp:408 in 9873aa48a9 outdated

 404 | @@ -405,6 +405,10 @@ struct Peer {
 405 |      /** Protocol version indicated by the peer in its initial version message. */
 406 |      std::atomic<int> m_version{0};
 407 |  
 408 | +    /** Greates common protocol version that both we and the peer support. Set

fanquake commented at 2:18 PM on January 19, 2023:

In 9873aa48a92ccf526762bde8d24911c3e81adb32: Greates and in the commit message m_greates_common_version

dergoegge commented at 3:49 PM on January 23, 2023: member

@ajtowns Thank you for having a look!

I think moving these is a good idea. The amount of code touched and the number of conflicting PRs is annoying though.

I generally do not agree with our "we hate rebasing" culture because there is no way to do collaborative development without rebasing. There are ~370 open PRs and avoiding conflicts is basically impossible. Merging something should boil down to whether or not the actual change is worthwhile and shouldn't depend on whether or not other people can be bothered to rebase their PRs as a result.

The majority of the conflicts here are either drafts, need more work, mine or are failing CI and the rest (afaict) only conflict in a very minor way. But again even if this weren't the case, I think it should just boil down to whether or not my changes are worthwhile.

Might be better to move the elements individually when they're useful for some patch that makes an observable change, though, rather than in a dedicated refactoring pr with no immediate benefit?

This would take ten times longer, not reduce the number of conflicts and would likely just end up with someone requesting that I split out the "pure refactoring" commits into their own PR anyway. So we might as well just rip the band-aid of in one go.

How can you say that there is no immediate benefit, when this PR is continuing an ongoing project #19398, that we (as a project) had seemingly agreed on being a worthwhile thing? Basically all PRs related to #19398 have been of this nature. Did they all have no benefit?

ajtowns commented at 10:17 PM on January 23, 2023: contributor

How can you say that there is no immediate benefit,

The benefit of refactoring is that it makes future patches easier to write/test/review/etc. An immediate benefit would be that it makes a PR we have right now easier to write/test/review/etc. AFAIK there isn't an open PR in the repo that this refactoring is making easier to write/test/review/etc, so that's why I say there's no immediate benefit.

Basically all PRs related to #19398 have been of this nature. Did they all have no benefit?

I think most of them also had no immediate benefit, yes, which is different to no benefit at all. At least one introduced a bug (#21160 vs #26569 / #26616), which is the opposite of an immediate benefit.

Like I said, I think this is a good idea. But even good ideas need to be prioritised; and for me refactorings default to coming very low on the list -- they easily introduce both immediate costs (review of the refactoring, rebasing and re-review of conflicting PRs) and long term costs (when they introduce bugs despite the review), and the benefits are often only theoretical ("someday we might write a patch that this helps") or a matter of opinion ("this makes the code look better to me").

This would take ten times longer, not reduce the number of conflicts and would likely just end up with someone requesting that I split out the "pure refactoring" commits into their own PR anyway.

This PR's been open for ~50 days; if no PR is going to benefit from this change for 500 days, then I've got no objection to leaving it unmerged for that long, or closing it and redoing it later. If we do manage a refactoring moratorium, then that will reduce the amount of rebasing of non-refactor PRs. And whether the refactoring is split out into a separate PR doesn't seem like a big deal either way; the cost is time spent reviewing and rebasing, not in making a PR or not...

in src/net_processing.cpp:3612 in c25f0f1719 outdated

3456 | @@ -3431,6 +3457,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3457 |                         tx_relay->m_next_inv_send_time == 0s));
3458 |          }
3459 |  
3460 | +        WITH_LOCK(m_version_nonces_mutex, m_version_nonces.erase(pfrom.GetId()));

mzumsande commented at 10:16 PM on January 30, 2023:

As far as I can see, this is an optimization because entries in m_version_nonces are only used to prevent self-connects, and at this point we can be sure we didn't connect to ourselves, so the entry is no longer needed.

A comment here (or somewhere else about this) would be good.

dergoegge commented at 11:48 AM on February 2, 2023:

Added a comment

in src/net_processing.cpp:402 in 8fcbab5bbe outdated

 397 | @@ -398,6 +398,10 @@ struct Peer {
 398 |      /** Peer selected us as (compact blocks) high-bandwidth peer (BIP152). */
 399 |      std::atomic<bool> m_bip152_highbandwidth_from{false};
 400 |  
 401 | +    /** Time offset computed during the version handshake based on the
 402 | +     * timestamp the peer send in the initial version message. */

mzumsande commented at 11:05 PM on January 30, 2023:

nit: sent; also maybe drop "initial", it's not like they send multiple version msgs.

mzumsande commented at 11:49 PM on January 30, 2023: contributor

Concept ACK

I do think that several of the conflicting PRs should be higher prio than this, but I also think that the rebases this will cause will be rather trivial in most of the cases.

dergoegge force-pushed on Feb 2, 2023

DrahtBot added the label Needs rebase on Feb 17, 2023

dergoegge force-pushed on Feb 27, 2023

DrahtBot removed the label Needs rebase on Feb 27, 2023

dergoegge closed this on Mar 1, 2023

dergoegge reopened this on Mar 1, 2023

maflcko commented at 11:56 AM on March 1, 2023: member

Cirrus CI won't pick up close/open, IIRC. If there was no review, your best option is to (force) push to trigger CI, if the GitHub trigger event was lost on the first try.

dergoegge force-pushed on Mar 1, 2023

DrahtBot added the label Needs rebase on Mar 28, 2023

dergoegge force-pushed on Mar 28, 2023

DrahtBot removed the label Needs rebase on Mar 28, 2023

hebasto commented at 2:09 PM on March 30, 2023: member

Concept ACK.

DrahtBot added the label Needs rebase on Mar 30, 2023

dergoegge force-pushed on Apr 3, 2023

dergoegge commented at 9:26 AM on April 3, 2023: member

Rebased

DrahtBot removed the label Needs rebase on Apr 3, 2023

DrahtBot added the label Needs rebase on Apr 20, 2023

dergoegge force-pushed on Apr 21, 2023

DrahtBot removed the label Needs rebase on Apr 21, 2023

DrahtBot added the label CI failed on Apr 21, 2023

DrahtBot removed the label CI failed on Apr 22, 2023

DrahtBot added the label Needs rebase on May 24, 2023

dergoegge force-pushed on May 30, 2023

DrahtBot added the label CI failed on May 30, 2023

DrahtBot removed the label Needs rebase on May 30, 2023

DrahtBot removed the label CI failed on May 31, 2023

DrahtBot added the label CI failed on Jun 25, 2023

dergoegge force-pushed on Jun 28, 2023

dergoegge commented at 12:52 PM on June 28, 2023: member

Needs rebase if still relevant

Rebased

DrahtBot removed the label CI failed on Jun 28, 2023

DrahtBot added the label CI failed on Jul 13, 2023

maflcko commented at 5:54 AM on July 14, 2023: member

Needs rebase if still relevant

dergoegge force-pushed on Jul 14, 2023

dergoegge commented at 9:37 AM on July 14, 2023: member

Needs rebase if still relevant

Thanks, rebased

DrahtBot removed the label CI failed on Jul 14, 2023

DrahtBot added the label Needs rebase on Jul 25, 2023

dergoegge force-pushed on Jul 26, 2023

DrahtBot removed the label Needs rebase on Jul 26, 2023

DrahtBot added the label Needs rebase on Sep 7, 2023

scripted-diff: rename CNodeStateStats

Most of the stats in CNodeStateStats are now taken from the Peer object.
Rename the struct to PeerStats.

-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren CNodeStateStats          PeerStats
ren GetNodeStateStats        GetPeerStats
ren nodeStateStats           m_peer_stats
ren statestats               peer_stats
ren fNodeStateStatsAvailable m_peer_stats_available
ren fStateStats              peer_stats_available

-END VERIFY SCRIPT-

211b84afbc

[net processing] Move cleanSubVer to net_processing

Also rename to m_clean_subversion.

Since the subversion is now contained in the Peer object, it may not be
available in `getpeerinfo` while the peer connection is being torn down.
Update the rpc help text to mark the field optional and update the tests
to tolerate the field being absent.

fe7c799bec

[net processing] Remove CNode::nLocalHostNonce

Handle version nonces entirely within net_processing.

1a4369a7f0

[net processing] Move m_bip152_highbandwidth_{to,from} to net_processing ab884b5783

[net processing] Move nTimeOffset to net_processing d690fe1c19

[net processing] Move nVersion to net_processing 8c08e747c5

[net processing] Move m_greatest_common_version to net_processing a840290438

dergoegge force-pushed on Sep 11, 2023

DrahtBot removed the label Needs rebase on Sep 11, 2023

DrahtBot added the label Needs rebase on Sep 14, 2023

DrahtBot commented at 10:44 AM on September 14, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

achow101 marked this as a draft on Sep 20, 2023

dergoegge closed this on Sep 28, 2023

bitcoin locked this on Sep 27, 2024