Fix -netinfo backward compat with getpeerinfo pre-v26 #29212

1. 
   jonatack commented at 9:01 pm on January 9, 2024: contributor

   Commit fb5bfed26a564014b83ccfc96ff00b630930fc61 in #29058 will cause -netinfo to break when calling it on a node that is running pre-v26 bitcoind, as getpeerinfo doesn’t yet return a “transport_protocol_type” field.

   Fix this by adding an IsNull() check, as already done for other recent getpeerinfo fields, and also in the same commit:

   a) avoid checking for the full string “detecting”, and instead do the cheaper check for the most frequent case of the string starting with “v”

   b) drop displaying the “v” prefix in all the rows, as it doesn’t add useful information, and instead use “v” for the column header

   c) display nothing when a value isn’t determined yet, like for the -netinfo mping and ping columns (as * already has a separate meaning in this dashboard, and ? might look like there is a bug)

2. 
   DrahtBot commented at 9:01 pm on January 9, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	kristapsk, mzumsande, achow101
   Concept ACK	sipa

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

3. Fix -netinfo backward compat with getpeerinfo pre-v26

   CLI -netinfo will currently break when calling it on a node that is running
   pre-v26 bitcoind, as `getpeerinfo` doesn't yet return a transport_protocol_type
   field.
   
   Fix this by adding an `IsNull()` check as already done for other fields, and also:
   
   - avoid checking for the full string "detecting", and instead do the cheaper
     check for the most frequent case of the string starting with "v"
   
   - drop displaying the "v" prefix in all the rows, as it doesn't add useful
     information, and instead use "v" for the column header
   
   - display nothing during peer setup, like for the -netinfo mping and ping columns

   5fa74609b8
4. in src/bitcoin-cli.cpp:521 in d221f2a333 outdated

   517@@ -518,7 +518,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
   518                 const std::string addr{peer["addr"].get_str()};
   519                 const std::string age{conn_time == 0 ? "" : ToString((time_now - conn_time) / 60)};
   520                 const std::string sub_version{peer["subver"].get_str()};
   521-                const std::string transport{peer["transport_protocol_type"].get_str()};
   522+                const std::string transport{peer["transport_protocol_type"].isNull() ? "" : peer["transport_protocol_type"].get_str()};
   

   ---

   ---

   

   kristapsk commented at 9:20 pm on January 9, 2024:

   Can’t we just default to "v1" instead of "" in case of null? Pre-v26 doesn’t support v2 transport, it will always be v1.

   ---

   

   jonatack commented at 9:26 pm on January 9, 2024:

   Can’t we just default to "v1" instead of "" in case of null? Pre-v26 doesn’t support v2 transport, it will always be v1.

   Done

5. jonatack force-pushed on Jan 9, 2024
6. 
   jonatack commented at 9:29 pm on January 9, 2024: contributor

   Re-pushed per @kristapsk’s feedback (thanks!)
7. 
   sipa commented at 9:30 pm on January 9, 2024: member

   Concept ACK
8. kristapsk approved
9. 
   kristapsk commented at 9:54 pm on January 9, 2024: contributor

   ACK 5fa74609b833643334dfb5519f2023119984267b
10. DrahtBot requested review from sipa on Jan 9, 2024
11. in src/bitcoin-cli.cpp:541 in 5fa74609b8

    537@@ -538,7 +538,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
    538         // Report detailed peer connections list sorted by direction and minimum ping time.
    539         if (DetailsRequested() && !m_peers.empty()) {
    540             std::sort(m_peers.begin(), m_peers.end());
    541-            result += strprintf("<->   type   net tp  mping   ping send recv  txn  blk  hb %*s%*s%*s ",
    542+            result += strprintf("<->   type   net  v  mping   ping send recv  txn  blk  hb %*s%*s%*s ",
    

    ---

    ---

    

    mzumsande commented at 10:18 pm on January 9, 2024:

    I think that if we don’t write the “v”, we can make the width of the added column 1 instead of 2.

    ---

    

    jonatack commented at 10:27 pm on January 9, 2024:

    Yes, I began with that, but it looked better to my eye when I stayed with the width you set. Otherwise, it seemed to crowd close to the net column. Can update if people really prefer that way.

    ---

    

    kristapsk commented at 10:37 pm on January 9, 2024:

    I like that extra space as is, IMHO it visually looks better.

    

    ---

    

    mzumsande commented at 10:38 pm on January 10, 2024:

    ok, if it’s on purpose it’s fine.
12. 
    mzumsande commented at 10:24 pm on January 9, 2024: contributor

    Concept ACK
13. 
    mzumsande commented at 10:38 pm on January 10, 2024: contributor

    Code Review ACK 5fa74609b833643334dfb5519f2023119984267b
14. 
    achow101 commented at 5:35 pm on January 11, 2024: member

    ACK 5fa74609b833643334dfb5519f2023119984267b
15. achow101 merged this on Jan 11, 2024
16. achow101 closed this on Jan 11, 2024

    ---

17. jonatack deleted the branch on Jan 11, 2024

Contributors
jonatack DrahtBot kristapsk sipa mzumsande achow101

Review Requested
sipa