p2p, rpc: address relay fixups #22616

Following review of new changes merged today, move a use of statestats in getpeerinfo to within the section guarded by if (fStateStats), e.g. PeerManagerImpl::GetNodeStateStats true, and pass an in-param by reference to const.

Could use std::optional to avoid this stuff in the future?

Could use std::optional to avoid this stuff in the future?

Sure, will have a look. Might be better as a separate refactoring patch?

good catch on the statestats conditional @jonatack! thanks for fixing :) ACK 5e33f762d4

RE:

Could use std::optional to avoid this stuff in the future?

Sure, will have a look

good idea @MarcoFalke! FYI @jonatack I'm also going to take a look. having compiler catch this error seems like a good move because clearly I missed it 😛

@amitiuttarwar sgtm, go for it, happy to review!

took a look, unfortunately I don't think having GetNodeStateStats return an std::optional<CNodeStateStats> increases the compiler guarantee. we still have to manually check for presence.

<details><summary>example of accessing fields of a std::nullopt </summary>

struct NewStruct {
    int a;
    int b;
};

std::optional<NewStruct> GetNodeStateStats() {
    return std::nullopt;
}

int main()
{
    std::optional<NewStruct> fStateStats = GetNodeStateStats();
    std::cout << fStateStats->a << std::endl;
    std::cout << fStateStats->b << std::endl;

    return 0;
}

... compiles fine =\

</details>

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22604 (p2p, rpc, test: address rate-limiting follow-ups by jonatack)
• #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.

yes...did https://github.com/jonatack/bitcoin/commit/397c33c399685b2ca4fe98de6e975f29f2f3d040 as a warmup, it builds and tests pass, and it seems to be a bit nicer as an interface but doesn't yet do what we're looking for, was thinking to then convert some of the CNodeStateStats struct members to std::optional and then maybe use them with value_or or something.

Can you explain why this is something we don't want? Surely compilers are happy to compile UB, but that doesn't mean it won't be caught.

Previously (returning a pair of bool + some type T), the only way to catch it is with code review (and maybe tests).

Returning a std::optional<T> instead will allow any of the following to catch it:

• code review (it is easier because there is a * or -> without corresponding check)
• compiler warning
• valgrind [1]
• static analysis
• tests (maybe also with valgrind)
• something I missed?

[1]: For example:

#include <optional>

struct NewStruct {
  int a;
  int b;
};

std::optional<NewStruct> GetNodeStateStats() { return std::nullopt; }

int main() { return GetNodeStateStats()->a; }

==991573== Memcheck, a memory error detector
==991573== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==991573== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
==991573== Command: ./exe
==991573== 
==991573== Syscall param exit_group(status) contains uninitialised byte(s)
==991573==    at 0x4CB2021: _Exit (in /usr/lib64/libc-2.33.so)
==991573==    by 0x4C24C01: __run_exit_handlers (in /usr/lib64/libc-2.33.so)
==991573==    by 0x4C24C9F: exit (in /usr/lib64/libc-2.33.so)
==991573==    by 0x4C0CB7B: (below main) (in /usr/lib64/libc-2.33.so)

Note that on master it is impossible for valgrind to catch this because the memory is always filled:

struct CNodeStateStats {
    int nSyncHeight = -1;
    int nCommonHeight = -1;
    int m_starting_height = -1;
    std::chrono::microseconds m_ping_wait;
    std::vector<int> vHeightInFlight;
    uint64_t m_addr_processed = 0;
    uint64_t m_addr_rate_limited = 0;
    bool m_addr_relay_enabled{false};
};

If it wasn't filled, valgrind detecting it would depend on whether {} is used to initialize or not. I.e

  NewStruct ret{};
  GetNodeStateStats(ret);

vs

  NewStruct ret;
  GetNodeStateStats(ret);

For my part, I wasn't sure if the direction started with in https://github.com/jonatack/bitcoin/commit/397c33c399685b2ca4fe98de6e975f29f2f3d040 was what you had in mind, versus for instance also or starting with making the struct member(s) std::optional themselves, or something else.

It seems to be a separate refactoring from this small fix-up, but good to clarify the direction.

I think one way to improve this would be to combine CNodeStats and CNodeStateStats into a single struct, and change the individual fields that are populated by net_processing to be std::optional<>s. The rpc code could then populate the univalue object as follows:

        // ...
        obj.pushKV("inbound", stats.fInbound);
        obj.pushKV("bip152_hb_to", stats.m_bip152_highbandwidth_to);
        obj.pushKV("bip152_hb_from", stats.m_bip152_highbandwidth_from);
        if (stats->m_starting_height) obj.pushKV("startingheight", stats->m_starting_height.value());
        if (stats->nSyncHeight) obj.pushKV("synced_headers", stats->nSyncHeight.value());
        // etc ...

That requires quite a bit of moving code around, since both net and net_processing would have to include the header that defines the combined stats struct. I made a start on that work, but it's very out of date: https://github.com/jnewbery/bitcoin/tree/2021-01-stats

ACK 5e33f762d44557a1e3f0ff3c280d8a3ab98e3867

<!--cf906140f33d8803c4a75a2196329ecb-->

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}