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

theStack · 2020-08-21T14:08:17Z

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.

jonatack · 2020-08-21T14:17:59Z

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

theStack · 2020-08-21T14:56:30Z

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

DrahtBot · 2020-08-21T16:02:30Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net processing: Tolerate sendheaders and sendcmpct messages before verack #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 · 2020-08-22T23:54:08Z

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

practicalswift · 2020-08-23T05:38:58Z

Concept ACK

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

jonatack · 2020-08-24T06:11:46Z

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.

jonatack

Tested and globally looks good. A few suggestions follow.

doc/release-notes-19776.md

src/rpc/net.cpp

test/functional/p2p_compactblocks.py

src/net_processing.cpp

test/functional/p2p_compactblocks.py

sipa · 2020-08-24T08:27:12Z

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 · 2020-08-24T11:15:35Z

@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 · 2020-08-25T13:11:03Z

My favourite would simply be bip152_highbandwidth_{from,to}

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

instagibbs · 2020-08-25T15:19:44Z

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 · 2020-08-26T12:53:56Z

Concept ACK.

theStack · 2020-08-26T15:32:01Z

// 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 · 2020-08-27T22:52:13Z

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 · 2020-08-28T00:48:03Z

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.

jonatack

ACK a286f89

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

src/rpc/net.cpp

doc/release-notes-19776.md

theStack · 2020-09-03T08:38:56Z

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

Code review ACK bff461c per git diff a286f89 bff461c

6384419 test: add parameterized constructor for msg_sendcmpct() (Sebastian Falbesoner) Pull request description: While working on the test for #19776 I noticed that creating a `sendcmpct` message is quite cumbersome -- due to the lack of a parameterized constructor, one needs to create an empty (that is, initialized with default values) object and then set the two fields one by one. This PR replaces the default constructor with a parameterized constructor and uses it in the test `p2p_compactblocks.py`, reducing LOC. No need to pollute the namespace with temporary throw-away message objects anymore. ACKs for top commit: guggero: Code review ACK 6384419. epson121: Code review ACK 6384419 Tree-SHA512: 3b58d276d714b73abc6cc98d1d52dec5f6026b33f03faaeb7dcbc5d83ac377555179f98b159b2b9ecc8957999c35a1dc082e3c69299c5fde4e35f1bd0587ce9d

theStack · 2020-09-20T09:56:51Z

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

…cmpct() 6384419 test: add parameterized constructor for msg_sendcmpct() (Sebastian Falbesoner) Pull request description: While working on the test for bitcoin#19776 I noticed that creating a `sendcmpct` message is quite cumbersome -- due to the lack of a parameterized constructor, one needs to create an empty (that is, initialized with default values) object and then set the two fields one by one. This PR replaces the default constructor with a parameterized constructor and uses it in the test `p2p_compactblocks.py`, reducing LOC. No need to pollute the namespace with temporary throw-away message objects anymore. ACKs for top commit: guggero: Code review ACK 6384419. epson121: Code review ACK 6384419 Tree-SHA512: 3b58d276d714b73abc6cc98d1d52dec5f6026b33f03faaeb7dcbc5d83ac377555179f98b159b2b9ecc8957999c35a1dc082e3c69299c5fde4e35f1bd0587ce9d

naumenkogs · 2020-09-21T09:37:41Z

Code review ACK bc8cc02

theStack · 2020-09-21T23:37:48Z

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

naumenkogs · 2020-09-22T06:15:22Z

reACK 4df1d12

jonatack · 2020-09-22T07:29:54Z

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

src/net.h

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

theStack · 2020-09-28T23:06:17Z

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 (comment)) and fixed typos in the comments (s/sendcmpt/sendcmpct/).

naumenkogs · 2020-09-29T06:00:17Z

reACK 343dc47

jonatack · 2020-09-29T06:51:04Z

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

theStack · 2020-10-21T13:21:44Z

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 · 2020-10-21T13:33:14Z

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).

maflcko · 2020-10-21T14:32:09Z

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

…etpeerinfo 343dc47 test: add test for high-bandwidth mode states in getpeerinfo (Sebastian Falbesoner) dab6583 doc: release note for new getpeerinfo fields "bip152_hb_{from,to}" (Sebastian Falbesoner) a7ed00f rpc: expose high-bandwidth mode states via getpeerinfo (Sebastian Falbesoner) 30bc8fa net: save high-bandwidth mode states in CNodeStats (Sebastian Falbesoner) Pull request description: Fixes bitcoin#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](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki), in particular the [protocol flow diagram](https://github.com/bitcoin/bips/raw/master/bip-0152/protocol-flow.png). 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. ACKs for top commit: naumenkogs: reACK 343dc47 jonatack: re-ACK 343dc47 per `git range-diff 7ea6499 4df1d12 343dc47` Tree-SHA512: f4999e6a935266812c2259a9b5dc459710037d3c9e938006d282557cc225e56128f72965faffb207fc60c6531fab1206db976dd8729a69e8ca29d4835317b99f

Summary: > net: save high-bandwidth mode states in CNodeStats > rpc: expose high-bandwidth mode states via getpeerinfo > doc: release note for new getpeerinfo fields "bip152_hb_{from,to}" > test: add test for high-bandwidth mode states in getpeerinfo This is a backport of [[bitcoin/bitcoin#19776 | core#19776]] Depends on D10961 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10962

DrahtBot added P2P RPC/REST/ZMQ labels Aug 21, 2020

This was referenced Aug 21, 2020

[RPC] Add connection type to getpeerinfo, improve logs #19725

Merged

p2p: Unify Send and Receive protocol versions #17785

Merged

theStack mentioned this pull request Aug 23, 2020

test: add parameterized constructor for msg_sendcmpct() #19781

Merged

jonatack reviewed Aug 24, 2020

View reviewed changes

theStack force-pushed the 20200821-rpc-expose-high-handwidth-mode-state-via-getpeerinfo branch from edb4ad4 to a286f89 Compare August 27, 2020 22:35

DrahtBot mentioned this pull request Aug 28, 2020

net processing: Move block inventory state to net_processing #19829

Merged

jonatack reviewed Sep 1, 2020

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

doc/release-notes-19776.md Outdated Show resolved Hide resolved

theStack force-pushed the 20200821-rpc-expose-high-handwidth-mode-state-via-getpeerinfo branch from a286f89 to bff461c Compare September 3, 2020 08:32

jonatack reviewed Sep 3, 2020

View reviewed changes

DrahtBot mentioned this pull request Sep 19, 2020

net, rpc: expose connection type in getpeerinfo #19883

Closed

theStack force-pushed the 20200821-rpc-expose-high-handwidth-mode-state-via-getpeerinfo branch from bff461c to bc8cc02 Compare September 20, 2020 09:51

DrahtBot added the Needs rebase label Sep 21, 2020

theStack force-pushed the 20200821-rpc-expose-high-handwidth-mode-state-via-getpeerinfo branch from bc8cc02 to 4df1d12 Compare September 21, 2020 23:28

DrahtBot removed the Needs rebase label Sep 22, 2020

DrahtBot mentioned this pull request Sep 23, 2020

net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo #20002

Merged

DrahtBot added the Needs rebase label Sep 26, 2020

sipa reviewed Sep 26, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

theStack and others added 4 commits September 29, 2020 00:42

net: save high-bandwidth mode states in CNodeStats

30bc8fa

rpc: expose high-bandwidth mode states via getpeerinfo

a7ed00f

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

dab6583

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

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

343dc47

theStack force-pushed the 20200821-rpc-expose-high-handwidth-mode-state-via-getpeerinfo branch from 4df1d12 to 343dc47 Compare September 28, 2020 22:55

DrahtBot removed the Needs rebase label Sep 28, 2020

maflcko added this to the 0.22.0 milestone Oct 21, 2020

DrahtBot mentioned this pull request Dec 8, 2020

net processing: Tolerate sendheaders and sendcmpct messages before verack #20599

Merged

maflcko merged commit 22f13c1 into bitcoin:master Dec 10, 2020

jonatack mentioned this pull request Jan 2, 2021

doc: add -netinfo help #20829

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theStack commented Aug 21, 2020 •

edited

jonatack commented Aug 21, 2020 •

edited

theStack commented Aug 21, 2020

DrahtBot commented Aug 21, 2020 •

edited

theStack commented Aug 22, 2020

practicalswift commented Aug 23, 2020

jonatack commented Aug 24, 2020

jonatack left a comment

sipa commented Aug 24, 2020

theStack commented Aug 24, 2020

jonatack commented Aug 25, 2020

instagibbs commented Aug 25, 2020 •

edited

naumenkogs commented Aug 26, 2020

theStack commented Aug 26, 2020 •

edited

theStack commented Aug 27, 2020

gmaxwell commented Aug 28, 2020

jonatack left a comment

theStack commented Sep 3, 2020 •

edited

jonatack left a comment

theStack commented Sep 20, 2020

naumenkogs commented Sep 21, 2020

theStack commented Sep 21, 2020

naumenkogs commented Sep 22, 2020

jonatack commented Sep 22, 2020

theStack commented Sep 28, 2020

naumenkogs commented Sep 29, 2020

jonatack commented Sep 29, 2020

theStack commented Oct 21, 2020

jonatack commented Oct 21, 2020 •

edited

maflcko commented Oct 21, 2020

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

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

Conversation

theStack commented Aug 21, 2020 • edited

jonatack commented Aug 21, 2020 • edited

theStack commented Aug 21, 2020

DrahtBot commented Aug 21, 2020 • edited

Conflicts

theStack commented Aug 22, 2020

practicalswift commented Aug 23, 2020

jonatack commented Aug 24, 2020

jonatack left a comment

Choose a reason for hiding this comment

sipa commented Aug 24, 2020

theStack commented Aug 24, 2020

jonatack commented Aug 25, 2020

instagibbs commented Aug 25, 2020 • edited

naumenkogs commented Aug 26, 2020

theStack commented Aug 26, 2020 • edited

theStack commented Aug 27, 2020

gmaxwell commented Aug 28, 2020

jonatack left a comment

Choose a reason for hiding this comment

theStack commented Sep 3, 2020 • edited

jonatack left a comment

Choose a reason for hiding this comment

theStack commented Sep 20, 2020

naumenkogs commented Sep 21, 2020

theStack commented Sep 21, 2020

naumenkogs commented Sep 22, 2020

jonatack commented Sep 22, 2020

theStack commented Sep 28, 2020

naumenkogs commented Sep 29, 2020

jonatack commented Sep 29, 2020

theStack commented Oct 21, 2020

jonatack commented Oct 21, 2020 • edited

maflcko commented Oct 21, 2020

theStack commented Aug 21, 2020 •

edited

jonatack commented Aug 21, 2020 •

edited

DrahtBot commented Aug 21, 2020 •

edited

instagibbs commented Aug 25, 2020 •

edited

theStack commented Aug 26, 2020 •

edited

theStack commented Sep 3, 2020 •

edited

jonatack commented Oct 21, 2020 •

edited