Skip to content

doc: fix -netinfo relaytxes help #26328

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.

Sign up for GitHub

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

Jump to bottom
Closed
jonatack wants to merge 1 commit into from
Closed

doc: fix -netinfo relaytxes help #26328

jonatack wants to merge 1 commit into from

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Oct 17, 2022 •
edited
Loading

Addresses #26109 (comment) by Marco Falke (thanks!)

@jonatack jonatack mentioned this pull request Oct 17, 2022
Merged
@DrahtBot DrahtBot added the Docs label Oct 18, 2022
maflcko
maflcko reviewed Oct 18, 2022
View reviewed changes
@jonatack jonatack changed the title doc: fix up -netinfo relaytxes help netinfo: fix relaytxes doc, display 3 relaytxes states Oct 18, 2022
@jonatack jonatack force-pushed the update-netinfo-relaytxes-help branch from e49ecf3 to bb50d89 Compare October 18, 2022 20:09
@jonatack
Copy link
Member Author

jonatack commented Oct 18, 2022

@MarcoFalke @shaavan thanks for taking a look! Addressed your feedback, improved the getpeerinfo#relaytxes help, and added explicitly handling the new (v24.0) relaytxes 3rd state of "not available" in -netinfo.

@jonatack jonatack force-pushed the update-netinfo-relaytxes-help branch from bb50d89 to d51dbbc Compare October 18, 2022 20:20
shaavan
shaavan reviewed Oct 19, 2022
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

relaytxes is set based on the value of m_relay_txes if fStateStats is true; otherwise, relaytxes is unavailable.

net.cpp 203

if (fStateStats) {
            obj.pushKV("relaytxes", statestats.m_relay_txs);
        }

And fStateStats is equal to false when either Node or peer is nullptr:

net_processing.cpp 1555

if (state == nullptr)
            return false;

net_processing.cpp 1566

if (peer == nullptr) return false;

The value of relaytxes depends on the value of statestats.m_relay_txs.

And the value of statestats.m_relay_txs can be false in two cases:

if m_tx_relay→m_relay_txs is false.

net_processing.cpp 1580

if (auto tx_relay = peer->GetTxRelay(); tx_relay != nullptr) {
        stats.m_relay_txs = WITH_LOCK(tx_relay->m_bloom_filter_mutex, return tx_relay->m_relay_txs);

or if m_tx_relay is a nullptr.

net_processing.cpp 1583

else {
        stats.m_relay_txs = false;

And according to the comment on m_tx_relay, it will be a nullptr when we are not relaying transactions to this peer

net_processing.cpp 398

/** Transaction relay data. Will be a nullptr if we're not relaying
     *  transactions with this peer (e.g., if it's a block-relay-only peer or
     *  the peer has sent us fRelay=false with bloom filters disabled). */
    std::unique_ptr<TxRelay> m_tx_relay GUARDED_BY(m_tx_relay_mutex);

So as far as I understood, there are four possible states:

  1. Node asked to relay, and we will relay txs to this node (m_tx_relay ≠ nullptr and m_tx_relay → m_relay_txs = True)
  2. Node asked not to relay (m_tx_relay ≠ nullptr and m_tx_relay→m_relay_txs = False)
  3. Node is not yet setup (peer == nullptr, irrespective of values of m_tx_relay and m_relay_txs)
  4. Node is setup but we can’t relay to it (m_tx_relay = nullptr)

The first three states are properly represented by the PR using "", "*", "." repectively. However, I think there is a need to address the 4th state as well in which we can't relay transactions to the peer.

@jonatack
Copy link
Member Author

jonatack commented Oct 20, 2022 

4. Node is setup but we can’t relay to it (`m_tx_relay` = **nullptr**)

The first three states are properly represented by the PR using "", "*", "." repectively. However, I think there is a need to address the 4th state as well in which we can't relay transactions to the peer.

@shaavan thanks for looking. Running git grep -B4 "stats.m_relay_txs = false;", in the 4th case you describe PeerManagerImpl::GetNodeStateStats() returns m_relay_txs as false (and so getpeerinfo#relaytxes is false). This is one of the 3 states (true, false, not available) returned by that field and handled here.

brunoerg
brunoerg reviewed Oct 26, 2022
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

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

utACK d51dbbc697adeac2563727e27f49b98bcc319914

I was able to verify the three states in the code and the lack of documentation that this PR fixes. Couldn't we cover this case where relaytxes is not available in a functional test? I confess that I could test manually this field by running it, and could check true/false states but it has been a little difficult to test manually and get a n/a.

@luke-jr
Copy link
Member

luke-jr commented Oct 27, 2022

If relaytxes is omitted ONLY if it hasn't been setup yet, we should just include it as false IMO.

(But if it could be during peer destruction, that might not be correct)

@jonatack
Copy link
Member Author

jonatack commented Oct 28, 2022 •
edited
Loading

@luke-jr I tend to agree; nevertheless see #26109 (comment) for the rationale, which was considered compelling enough to make getpeerinfo#relaytxes optional after being always present since the API field was introduced in 08843ed. Edit: I've added the link to the pull description.

Thank you @brunoerg! @MarcoFalke @shaavan mind re-reviewing? @w0xlt @dunxen @kristapsk @1440000bytes @Zero-1729 @0xB10C @jarolrod you have reviewed these kind of changes in the past, if you don't mind having a look.

Zero-1729
Zero-1729 reviewed Oct 30, 2022
View reviewed changes
@jonatack jonatack force-pushed the update-netinfo-relaytxes-help branch from d51dbbc to 7c733bd Compare November 4, 2022 21:46
@jonatack
Copy link
Member Author

jonatack commented Nov 4, 2022

Updated to take feedback by @Zero-1729, ACKs and re-ACKs welcome :)

@jonatack jonatack mentioned this pull request Nov 5, 2022
Merged
Zero-1729
Zero-1729 reviewed Nov 5, 2022
View reviewed changes
Zero-1729
Zero-1729 reviewed Nov 5, 2022
View reviewed changes
Zero-1729
Zero-1729 reviewed Nov 5, 2022
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

tACK 7c733bd25e024a23e75fb1015e1c394df88df2d0

Tested on macOS v13.0 (22A380)

I can confirm this patch splits the displayed relaytxes states into three (true, false, n/a due to pending setup), updates the help docs, and release notes.

Brief digest diff

On master (ae6bb6e)

$ ./src/bitcoin-cli -signet -netinfo 4   
Bitcoin Core client v24.99.0-ae6bb6e71e30-dirty signet - server 70016/Satoshi:24.99.0/

<->   type   net  mping   ping send recv  txn  blk  hb addrp addrl  age id address               version
out   full  ipv4    175    175   15   22         0       172          0  3 85.208.70.229:38333   70016/Satoshi:23.0.0/
out   full  ipv4    301    301   16   16                 123          0  4 202.61.248.219:38333  70016/Satoshi:24.99.0/
out   full  ipv4    517    517   22   22         0  .    184          0  2 72.48.253.168:38333   70016/Satoshi:22.0.0/
out   full  ipv4    570    570   10   10                 230          0  5 147.182.229.68:38333  70016/Satoshi:22.99.0/
out  block  ipv4    753    753   22   22    *    0         .          0  0 18.166.228.180:38333  70016/Satoshi:23.0.0/
out  block  ipv4    879    879   22   22    *    0  .      .          0  1 103.16.128.63:38333   70016/Satoshi:23.0.0/
out   full  ipv4   1247   1247    7    7                 205          0  6 178.128.221.177:38333 70016/Satoshi:22.99.0/
                     ms     ms  sec  sec  min  min                  min

         ipv4    ipv6   total   block
in          0       0       0
out         7       0       7       2
total       7       0       7

Local addresses: n/a
$ ./src/bitcoin-cli -signet getpeerinfo   
[
  {
    "id": 0,
    "addr": "18.166.228.180:38333",
    "addrbind": "192.168.0.161:53506",
    "addrlocal": "102.91.5.10:53506",
    "network": "ipv4",
    "services": "0000000000000409",
    "servicesnames": [
      "NETWORK",
      "WITNESS",
      "NETWORK_LIMITED"
    ],
    "relaytxes": false,
    …
  },
  …
  {
    "id": 2,
    "addr": "72.48.253.168:38333",
    "addrbind": "192.168.0.161:53509",
    "addrlocal": "197.210.70.183:53509",
    "network": "ipv4",
    "services": "000000000000040d",
    "servicesnames": [
      "NETWORK",
      "BLOOM",
      "WITNESS",
      "NETWORK_LIMITED"
    ],
    "relaytxes": true,
    …
  },
  …
]

On Patch (7c733bd25e024a23e75fb1015e1c394df88df2d0)

$ ./src/bitcoin-cli -signet -netinfo 4  
Bitcoin Core client v24.99.0-7c733bd25e02 signet - server 70016/Satoshi:24.99.0/

<->   type   net  mping   ping send recv  txn  blk  hb addrp addrl  age id address               version
out   full  ipv4    197    203   45   45                 242          4  7 49.12.208.214:38333   70016/Satoshi:0.21.1/
out   full  ipv4    198    200    2   53                 172          4  4 85.208.70.229:38333   70016/Satoshi:23.0.0/
out   full  ipv4    200    249   54   53    2            124          4  3 202.61.248.219:38333  70016/Satoshi:24.99.0/
out   full  ipv4    225    225   32   32                 148          4  9 31.14.40.19:38333     70016/Satoshi:22.0.0/
out   full  ipv4    249    284   51   51                 121          4  6 108.161.223.181:38333 70016/Satoshi:22.0.0/
out   full  ipv4    291    291   54   54    1            178          4  2 45.33.47.11:38333     70016/Satoshi:23.0.0/
out   full  ipv4    302    310   52   52                 139          4  5 104.128.228.143:38333 70016/Satoshi:23.0.0/
out   full  ipv4    303    444   44   44    3    2  .    230          4  8 147.182.229.68:38333  70016/Satoshi:22.99.0/
out  block  ipv4    387    405   62   62    .    5  .      .          5  0 18.166.228.180:38333  70016/Satoshi:23.0.0/
out  block  ipv4    442    442   61   61    .              .          5  1 103.16.128.63:38333   70016/Satoshi:23.0.0/
                     ms     ms  sec  sec  min  min                  min

         ipv4    ipv6   total   block
in          0       0       0
out        10       0      10       2
total      10       0      10

Local addresses: n/a 
$ ./src/bitcoin-cli -signet getpeerinfo   
[
  {
    "id": 0,
    "addr": "18.166.228.180:38333",
    "addrbind": "192.168.0.161:59682",
    "addrlocal": "197.210.71.89:64632",
    "network": "ipv4",
    "services": "0000000000000409",
    "servicesnames": [
      "NETWORK",
      "WITNESS",
      "NETWORK_LIMITED"
    ],
    "relaytxes": false,
    "lastsend": 1667626204,
    "lastrecv": 1667626204,
    "last_transaction": 0,
    …
  },
  …
  {
    "id": 4,
    "addr": "85.208.70.229:38333",
    "addrbind": "192.168.0.161:59702",
    "addrlocal": "102.91.5.10:59702",
    "network": "ipv4",
    "services": "0000000000000409",
    "servicesnames": [
      "NETWORK",
      "WITNESS",
      "NETWORK_LIMITED"
    ],
    "relaytxes": true,
    "lastsend": 1667626198,
    "lastrecv": 1667626093,
    "last_transaction": 0,
    …
  },
  …
 {
    "id": 8,
    "addr": "147.182.229.68:38333",
    "addrbind": "192.168.0.161:59717",
    "addrlocal": "197.210.71.89:59717",
    "network": "ipv4",
    "services": "0000000000000409",
    "servicesnames": [
      "NETWORK",
      "WITNESS",
      "NETWORK_LIMITED"
    ],
    "relaytxes": true,
    "lastsend": 1667626197,
    "lastrecv": 1667626102,
    "last_transaction": 1667625918,
    …
  }
  …
]

@jonatack jonatack force-pushed the update-netinfo-relaytxes-help branch from 7c733bd to cfabd15 Compare November 5, 2022 17:53
@jonatack
Copy link
Member Author

jonatack commented Nov 5, 2022

Updated to take feedback by @Zero-1729, ACKs and re-ACKs welcome :)

Zero-1729
Zero-1729 approved these changes Nov 5, 2022
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

crACK cfabd15309eb42a9f20f4c11152c54fd27978fab

mzumsande
mzumsande reviewed Nov 16, 2022
View reviewed changes
src/bitcoin-cli.cpp Outdated
Copy link
Contributor

@mzumsande mzumsande Nov 16, 2022

Choose a reason for hiding this comment

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

See #26457 (comment), I think the third state (N/A) would not be used during connection startup as intended. I confirmed this by adding a sleep at the beginning of ::VERSION processing in ProcessMessage and querying getpeerinfo with this branch - I'd get relaytxes= false but the field is included in the response.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 16, 2022 •
edited
Loading

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
ACK mzumsande
Stale ACK brunoerg, Zero-1729

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

Conflicts

No conflicts as of last run.

@aureleoules aureleoules mentioned this pull request Nov 16, 2022
Merged
@bitcoin bitcoin deleted a comment from DrahtBot Nov 16, 2022
@DrahtBot DrahtBot mentioned this pull request Nov 17, 2022
Closed
@jonatack
Copy link
Member Author

jonatack commented Nov 17, 2022

Closing for now in favor of #26516.

@jonatack jonatack closed this Nov 17, 2022
@jonatack
Copy link
Member Author

jonatack commented Nov 17, 2022

Re-opening after reading #26457 (comment), pending reviewer feedback here and on #26516 (the latter has the benefit of not changing user space).

@jonatack jonatack reopened this Nov 17, 2022
@DrahtBot DrahtBot mentioned this pull request Nov 17, 2022
Closed
@DrahtBot DrahtBot mentioned this pull request Nov 27, 2022
Merged
@fanquake
Copy link
Member

fanquake commented Dec 5, 2022

What are the next steps here?

pending reviewer feedback here and on #26516 (the latter has the benefit of not changing user space).

The reviewer feedback in #26516 seems to be that the change is either incorrect, or maybe needs better justification.

The reviewer feedback in this PR seems to be that the tri-state change is unnecessary, because the third state to display (N/A), will never be used, as it's impossible to have missing values during connection setup, as described in the docs added here.

@maflcko
Copy link
Member

maflcko commented Dec 5, 2022

I think how RPC returns that a value is missing, (with optional, or a magic value, or some extended logic) is orthogonal to cli netinfo changes. However, it would be beneficial to first figure out the RPC interface and then adjust the cli and gui code to that.

@DrahtBot DrahtBot mentioned this pull request Dec 19, 2022
Merged
@jonatack jonatack changed the title netinfo: fix relaytxes doc, display 3 relaytxes states doc: fix -netinfo relaytxes help Jan 10, 2023
@jonatack
doc: fix up -netinfo relaytxes help
0f5fc4f 
Co-authored-by: "MarcoFalke <*~=`'#}+{/-|&$^[email protected]>"
@jonatack jonatack force-pushed the update-netinfo-relaytxes-help branch from cfabd15 to 0f5fc4f Compare January 10, 2023 20:57
@jonatack
Copy link
Member Author

jonatack commented Jan 10, 2023

Simplified to just the fixup to #26109 (comment) found by @MarcoFalke.

mzumsande
mzumsande reviewed Jan 11, 2023
View reviewed changes
Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

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

Code Review ACK 0f5fc4f

@maflcko
Copy link
Member

maflcko commented Jan 11, 2023

I think my misunderstanding came from the fact that it is unclear if version.relaytxs or getpeerinfo.relaytxs was meant, see https://github.com/bitcoin/bitcoin/pull/26109/files#r1067201660

In any case 0f5fc4f lgtm

fanquake added a commit to bitcoin-core/gui that referenced this pull request Jan 11, 2023
@fanquake
Merge bitcoin/bitcoin#26328: doc: fix -netinfo relaytxes help
329d7e3 
0f5fc4f doc: fix up -netinfo relaytxes help (Jon Atack)

Pull request description:

  Addresses bitcoin/bitcoin#26109 (comment) by Marco Falke (thanks!)

ACKs for top commit:
  mzumsande:
    Code Review ACK 0f5fc4f

Tree-SHA512: d7345d1a94b15c4ec1a2bb0be5c04c472411d90cefb4c16ed524933d2bfc36816bb7519c2e109b2e41ff451b039dd2ddaa6d5db917ad54745332f2a1d8b85570
@fanquake
Copy link
Member

fanquake commented Jan 11, 2023

This has been merged.

@fanquake fanquake closed this Jan 11, 2023
@jonatack jonatack deleted the update-netinfo-relaytxes-help branch January 11, 2023 16:43
@jonatack
Copy link
Member Author

jonatack commented Jan 11, 2023

I think my misunderstanding came from the fact that it is unclear if version.relaytxs or getpeerinfo.relaytxs was meant, see #26109 (files)

In any case 0f5fc4f lgtm

I almost added s/relaytxes/getpeerinfo \"relaytxes\" field/ here yesterday. Might sneak it next time I touch the file.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 12, 2023
@fanquake
Merge bitcoin#26328: doc: fix -netinfo relaytxes help
46c295a 
0f5fc4f doc: fix up -netinfo relaytxes help (Jon Atack)

Pull request description:

  Addresses bitcoin#26109 (comment) by Marco Falke (thanks!)

ACKs for top commit:
  mzumsande:
    Code Review ACK 0f5fc4f

Tree-SHA512: d7345d1a94b15c4ec1a2bb0be5c04c472411d90cefb4c16ed524933d2bfc36816bb7519c2e109b2e41ff451b039dd2ddaa6d5db917ad54745332f2a1d8b85570
@bitcoin bitcoin locked and limited conversation to collaborators Jan 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

5 more reviewers

@maflcko maflcko maflcko left review comments

@brunoerg brunoerg brunoerg left review comments

@mzumsande mzumsande mzumsande left review comments

@shaavan shaavan shaavan left review comments

@Zero-1729 Zero-1729 Zero-1729 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jonatack @luke-jr @DrahtBot @fanquake @maflcko @brunoerg @Zero-1729 @mzumsande @shaavan