net, rpc: expose high bandwidth mode state via getpeerinfo

theStack commented at 2:08 pm on August 21, 2020: member

Fixes #19676, “For every peer expose through getpeerinfo RPC whether or not we selected them as HB peers, and whether or not they selected us as HB peers.” See BIP152, in particular the protocol flow diagram. The newly introduced states are changed on the following places in the code:

on reception of a SENDCMPCT message with valid version, the field m_highbandwidth_from is changed depending on the first integer parameter in the message (1=high bandwidth, 0=low bandwidth), i.e. it just mirrors the field CNodeState.fPreferHeaderAndIDs.
after adding a SENDCMPCT message to the send queue, the field m_highbandwidth_to is changed depending on how the first integer parameter is set (same as above)

Note that after receiving VERACK, the node also sends SENDCMPCT, but that is only to announce the preferred version and never selects high-bandwidth mode, hence there is no need to change the state variables there, which are initialized to false anyways.

DrahtBot added the label P2P on Aug 21, 2020

DrahtBot added the label RPC/REST/ZMQ on Aug 21, 2020

jonatack commented at 2:17 pm on August 21, 2020: member

Concept/approach ACK, could add test coverage and release note (see #19731 for an example).

theStack commented at 2:56 pm on August 21, 2020: member

Thanks for reviewing @jonatack! Added a release note, will add a functional test for this within the next days.

DrahtBot commented at 4:02 pm on August 21, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20599 (net processing: Tolerate sendheaders and sendcmpct messages before verack by jnewbery)

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.

theStack commented at 11:54 pm on August 22, 2020: member

Added a functional test to p2p_compactblocks.py that covers all 4 possible combinations of bandwidth_{from,to}.

practicalswift commented at 5:38 am on August 23, 2020: contributor

Concept ACK

Would be nice to have bitcoin-cli -netinfo t expose this info in the future :)

jonatack commented at 6:11 am on August 24, 2020: member

Would be nice to have bitcoin-cli -netinfo t expose this info in the future :)

Yes. Along with other new ones like last_transaction, last_block, conn_type.

in doc/release-notes-19776.md:9 in edb4ad4c9c outdated

0@@ -0,0 +1,9 @@
1+Updated RPCs
2+------------
3+
4+- The `getpeerinfo` RPC now has additional `highbandwidth_from` and
5+  `highbandwidth_to` fields that indicate whether a peer selected us as compact
6+  blocks high-bandwidth peer (`..._from`) or whether we selected a peer to be in
7+  compact blocks high-bandwidth mode (`..._to`), respectively. High-bandwidth
8+  peers are supposed to send new block announcements via a `cmptblock` message,
9+  rather than the usual inv/headers announcements. See BIP 152 for more details.

jonatack commented at 7:14 am on August 24, 2020:

8e9e5517 suggestion

0- The `getpeerinfo` RPC returns two new boolean fields, `highbandwidth_to` and
1  `highbandwidth_from`, that respectively indicate whether we selected a peer to
2  be in compact blocks high-bandwidth mode or whether a peer selected us as a
3  compact blocks high-bandwidth peer. High-bandwidth peers send new block
4  announcements via a `cmptblock` message rather than the usual inv/headers
5  announcements. See BIP152 for more details. (#19776)

theStack commented at 10:37 pm on August 27, 2020:

Thanks, I like that much better! Adapted it, removed the indentation (as other PR review notes currently in master also don’t have it) and added you as co-author.

in src/rpc/net.cpp:114 in edb4ad4c9c outdated

109@@ -110,6 +110,8 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
110                             {RPCResult::Type::NUM, "version", "The peer version, such as 70001"},
111                             {RPCResult::Type::STR, "subver", "The string version"},
112                             {RPCResult::Type::BOOL, "inbound", "Inbound (true) or Outbound (false)"},
113+                            {RPCResult::Type::BOOL, "highbandwidth_from", "Whether peer selected us as (compact blocks) high-bandwidth peer"},
114+                            {RPCResult::Type::BOOL, "highbandwidth_to", "Whether we selected peer as (compact blocks) high-bandwidth peer"},

jonatack commented at 7:21 am on August 24, 2020:

d778c4a / f9cc66bc / edb4ad4 here and in net.h/cpp and p2p_compactblocks.py, consider inverting the order to be to/from, per consistency with the existing lastsend/lastrecv order in the codebase and the language idioms of to/from and send/receive.

theStack commented at 10:40 pm on August 27, 2020:

Good point, done.

in test/functional/p2p_compactblocks.py:799 in edb4ad4c9c outdated

794+        def assert_highbandwidth_states(node, hb_from, hb_to):
795+            peerinfo = node.getpeerinfo()[-1]
796+            assert_equal(peerinfo['highbandwidth_from'], hb_from)
797+            assert_equal(peerinfo['highbandwidth_to'], hb_to)
798+
799+        # initially, both nodes haven't selected the other peer as high-bandwidth yet

jonatack commented at 7:27 am on August 24, 2020:

edb4ad4 nit, s/both nodes haven’t/neither node/

theStack commented at 10:48 pm on August 27, 2020:

Thanks, fixed.

in src/net_processing.cpp:2636 in edb4ad4c9c outdated

2630@@ -2629,8 +2631,10 @@ void ProcessMessage(
2631                 State(pfrom.GetId())->fProvidesHeaderAndIDs = true;
2632                 State(pfrom.GetId())->fWantsCmpctWitness = nCMPCTBLOCKVersion == 2;
2633             }
2634-            if (State(pfrom.GetId())->fWantsCmpctWitness == (nCMPCTBLOCKVersion == 2)) // ignore later version announces
2635+            if (State(pfrom.GetId())->fWantsCmpctWitness == (nCMPCTBLOCKVersion == 2)) { // ignore later version announces
2636                 State(pfrom.GetId())->fPreferHeaderAndIDs = fAnnounceUsingCMPCTBLOCK;
2637+                pfrom.m_highbandwidth_from = fAnnounceUsingCMPCTBLOCK;

jonatack commented at 8:01 am on August 24, 2020:

f9cc66bc An explanatory comment here may be nice; I found this the least clear line of this PR changeset.

theStack commented at 10:39 pm on August 27, 2020:

Done.

in test/functional/p2p_compactblocks.py:794 in edb4ad4c9c outdated

789+        # create new p2p connection for a fresh state w/o any prior sendcmpct messages sent
790+        hb_test_node = self.nodes[0].add_p2p_connection(TestP2PConn(cmpct_version=2))
791+
792+        # Asserts that the RPC getpeerinfo boolean result fields "highbandwidth_{from,to}"
793+        # for the last peer of a given node match the given parameters
794+        def assert_highbandwidth_states(node, hb_from, hb_to):

jonatack commented at 8:09 am on August 24, 2020:

edb4ad4 suggestions

0-        # Asserts that the RPC getpeerinfo boolean result fields "highbandwidth_{from,to}"
1-        # for the last peer of a given node match the given parameters
2-        def assert_highbandwidth_states(node, hb_from, hb_to):
3+        # Assert the getpeerinfo `highbandwidth_to/highbandwith_from` boolean
4+        # fields for the last peer of a given node match the given parameters.
5+        def assert_highbandwidth_states_of_last_peer(node, hb_to, hb_from):

theStack commented at 10:47 pm on August 27, 2020:

Shortened the comment a bit and also reordered the parameters. Didn’t want to change the function name to be even longer though (considering it’s so close to where its used, possibly even closer after #19781 would be merged) everything should be clear.

jonatack commented at 8:17 am on August 24, 2020: member

Tested and globally looks good. A few suggestions follow.

sipa commented at 8:27 am on August 24, 2020: member

Concept ACK.

I’d suggest putting “bip152” somewhere in the name of the field, as the concept of high bandwidth mode is very specific to that BIP (and really doesn’t dramatically increase actual bandwidth, as an unqualified name may make seem).

theStack commented at 11:15 am on August 24, 2020: member

@sipa: Agreed that referring to the BIP in the name would make sense here. @jonatack: Thanks for the detailled review! I plan to address all of your suggestions.

Before I take any further changes, I’d like to discuss the naming. Some suggestions:

bip152_highbandwidth_{from,to}
bip152_hb_{from,to} (shorter, but not obvious)
bip152_cmpctblk_announce_{from,to} (avoids the “high-bandwidth” terminology)
…

Does anyone have preferrences here or any other idea? My favourite would simply be bip152_highbandwidth_{from,to} (though being quite long).

jonatack commented at 1:11 pm on August 25, 2020: member

My favourite would simply be bip152_highbandwidth_{from,to}

I agree with you. Might be good to have input from @TheBlueMatt or @gmaxwell.

instagibbs commented at 3:19 pm on August 25, 2020: member

bip152_highbandwidth_{from,to} bip152_hb_{from,to} (shorter, but not obvious)

These seems fine, hb is shorter so I prefer. Anyone who is using this stuff should be familiar with the BIP and if not read it anyways.

naumenkogs commented at 12:53 pm on August 26, 2020: member

Concept ACK.

theStack commented at 3:32 pm on August 26, 2020: member

// EDIT: Before there was a “rebased” comment there that was not accurate, I confused with another PR 🤦‍♂️ Will still wait a bit for more naming opinions before tackling review suggestions.

Currently bip152_bandwidth_{from,to} (preferred by 2, jonatack and my humble self) is favoured over bip152_bh_{from,to} (preferred by 1, instagibbs).

theStack force-pushed on Aug 27, 2020

theStack commented at 10:52 pm on August 27, 2020: member

Decided for the name bip152_highbandwidth_{to,from} for now (both for the exposed RPC result fields and the internal variable names) and force-pushed with most of the suggestions from jonatack.

gmaxwell commented at 0:48 am on August 28, 2020: contributor

The patch I carry locally for this calls it

```
       obj.pushKV("bip152_hb", 
```

but I don’t think it matter other than it should mention BIP152.

in src/rpc/net.cpp:114 in a286f89a9c outdated

109@@ -110,6 +110,8 @@ static UniValue getpeerinfo(const JSONRPCRequest& request)
110                             {RPCResult::Type::NUM, "version", "The peer version, such as 70001"},
111                             {RPCResult::Type::STR, "subver", "The string version"},
112                             {RPCResult::Type::BOOL, "inbound", "Inbound (true) or Outbound (false)"},
113+                            {RPCResult::Type::BOOL, "bip152_highbandwidth_to", "Whether we selected peer as (compact blocks) high-bandwidth peer"},
114+                            {RPCResult::Type::BOOL, "bip152_highbandwidth_from", "Whether peer selected us as (compact blocks) high-bandwidth peer"},

jonatack commented at 11:06 am on September 1, 2020:

The names have indeed become very long, and they should arguably be written as bip152_high_bandwidth_{to/from}, so I’d now vote for bip152_hb_{to/from}.

0    "version" : n,                               (numeric) The peer version, such as 70001
1    "subver" : "str",                            (string) The string version
2    "inbound" : true|false,                      (boolean) Inbound (true) or Outbound (false)
3    "bip152_highbandwidth_to" : true|false,      (boolean) Whether we selected peer as (compact blocks) high-bandwidth peer
4    "bip152_highbandwidth_from" : true|false,    (boolean) Whether peer selected us as (compact blocks) high-bandwidth peer
5    "addnode" : true|false,                      (boolean) Whether connection was due to addnode/-connect or if it was an automatic/inbound connection
6    "startingheight" : n,                        (numeric) The starting height (block) of the peer

theStack commented at 8:36 am on September 3, 2020:

Done, see comment below.

in doc/release-notes-19776.md:4 in a286f89a9c outdated

0@@ -0,0 +1,9 @@
1+Updated RPCs
2+------------
3+
4+The `getpeerinfo` RPC returns two new boolean fields, `bip152_highbandwidth_to`

jonatack commented at 11:06 am on September 1, 2020:

nit if you retouch

0- The `getpeerinfo` RPC returns two new boolean fields, `bip152_highbandwidth_to`

theStack commented at 8:36 am on September 3, 2020:

Done.

jonatack commented at 11:12 am on September 1, 2020: member

ACK a286f89a9c0

Happy to re-review if you opt for the shorter naming.

theStack force-pushed on Sep 3, 2020

theStack commented at 8:38 am on September 3, 2020: member

Now that three people voted for the short version (instagibbs, jonatack, and indirectly also gmaxwell), I changed the RPC result fields to bip152_hb_{to,from}. The internal state variable names in CNode{Stats} are still kept in the long version though.

jonatack commented at 8:59 am on September 3, 2020: member

Code review ACK bff461c per git diff a286f89 bff461c

MarcoFalke referenced this in commit b99a1633b2 on Sep 20, 2020

theStack force-pushed on Sep 20, 2020

theStack commented at 9:56 am on September 20, 2020: member

Rebased on master and shortened the test by using the parameterized ctor for msg_sendcmpct, now that #19781 has been merged.

jonatack commented at 10:36 am on September 20, 2020: member

Code review re-ACK bc8cc02 per git range-diff 831b0ec bff461c bc8cc02

sidhujag referenced this in commit 0753f966dc on Sep 21, 2020

naumenkogs commented at 9:37 am on September 21, 2020: member

Code review ACK bc8cc02

DrahtBot added the label Needs rebase on Sep 21, 2020

theStack force-pushed on Sep 21, 2020

theStack commented at 11:37 pm on September 21, 2020: member

Rebased on master (was needed after the recent merge of #17785). Thanks a lot for reviewing, jonatack and naumenkogs!

DrahtBot removed the label Needs rebase on Sep 22, 2020

naumenkogs commented at 6:15 am on September 22, 2020: member

reACK 4df1d1244cd334fdb946de698fe39eba1b11147b

jonatack commented at 7:29 am on September 22, 2020: member

Code review re-ACK 4df1d1244cd334fdb946de698fe39eba1b11147b per git range-diff 7737603 bc8cc02 4df1d12

DrahtBot added the label Needs rebase on Sep 26, 2020

net: save high-bandwidth mode states in CNodeStats 30bc8fab68

rpc: expose high-bandwidth mode states via getpeerinfo a7ed00f8bb

doc: release note for new getpeerinfo fields "bip152_hb_{from,to}"

Co-authored-by: Jon Atack <jon@atack.com>

dab6583307

test: add test for high-bandwidth mode states in getpeerinfo 343dc4760f

in src/net.h:940 in d51fc2538d outdated

935@@ -934,6 +936,10 @@ class CNode
936 public:
937     uint256 hashContinue;
938     std::atomic<int> nStartingHeight{-1};
939+    // We selected peer as (compact blocks) high-bandwidth peer (BIP152)
940+    bool m_bip152_highbandwidth_to{false};

sipa commented at 6:31 pm on September 26, 2020:

As these are mutable fields, add a GUARDED_BY annotation?

theStack commented at 5:20 pm on September 27, 2020:

Good point, I’m not sure though on what to put into the annotation, i.e. guarded by what? I see three possibilities here to protecting concurrent access to the boolean fields:

introducing a new Mutex (seems overkill)
guard with cs_main, as that Mutex is already guaranteed to be held when the flags are written (seems overkill as well, considering this is not validation-critical at all and just about some statistics; locking cs_main in CNode::copyStats() just feels wrong)
simply using an atomic bool

Any thoughts about this?

sipa commented at 11:01 pm on September 27, 2020:

Atomic bools seem easiest.

theStack force-pushed on Sep 28, 2020

theStack commented at 11:06 pm on September 28, 2020: member

Rebased on master, changed the data type of the introduced high-bandwidth flags CNode::m_bip152_highbandwidth_{to,from} to atomic bools (see discussion #19776 (review)) and fixed typos in the comments (s/sendcmpt/sendcmpct/).

DrahtBot removed the label Needs rebase on Sep 28, 2020

naumenkogs commented at 6:00 am on September 29, 2020: member

reACK 343dc4760fd2407895fc8b3417a504b194429156

jonatack commented at 6:51 am on September 29, 2020: member

re-ACK 343dc4760fd2407895fc8b3417a504b194429156 per git range-diff 7ea6499 4df1d12 343dc47

theStack commented at 1:21 pm on October 21, 2020: member

Since there hasn’t been any activity for more than three weeks: is there anything else I can do for this PR? It has two ACKs (jonatack, naumenkogs) and two Concept ACKs (sipa, practicalswift), I think it should be ready for merge.

jonatack commented at 1:33 pm on October 21, 2020: member

I reckon it’s not a reflection on this PR (which seems good AFAICS), just that attention has been focused on reviewing key features before the feature freeze October 15 and now on fixing bugs before branch-off. After branch-off, I’m guessing that features in waiting like this one will see renewed attention (if not before).

MarcoFalke added this to the milestone 0.22.0 on Oct 21, 2020

MarcoFalke commented at 2:32 pm on October 21, 2020: member

Looks like this has missed the feature freeze. Can be merged after Nov 1st

MarcoFalke merged this on Dec 10, 2020

MarcoFalke closed this on Dec 10, 2020

sidhujag referenced this in commit f4ab4efc6d on Dec 10, 2020

Fabcien referenced this in commit 5020c1764d on Feb 3, 2022

DrahtBot locked this on Feb 15, 2022

net, rpc: expose high bandwidth mode state via getpeerinfo #19776

Conflicts