refactor: Replace `std::optional<bilingual_str>` with `util::Result` #25977

Replace uses of std::optional<bilingual_str> with util::Result as suggested #25648 (review), #27632 (review), #27632 (review), #24313 (review)

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27300 (build: debug enable addrman consistency checks by willcl-ark)
• #26762 (refactor: Make CCheckQueue RAII-styled by hebasto)
• #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

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.

Updated 9d3918d47daf7b118507ea6db0dcef69cedf7005 -> ebc493e94bbc43b6611b7211b77c12c453171ce9 (pr/bresult-opt.1 -> pr/bresult-opt.2, compare) to fix CI compile error https://cirrus-ci.com/task/6195420723412992?logs=ci#L2419 Rebased ebc493e94bbc43b6611b7211b77c12c453171ce9 -> f1a9c713540b3f00cea3c29ace4958b77390a8ef (pr/bresult-opt.2 -> pr/bresult-opt.3, compare) on top of pr/bresult2.24 to fix conflicts in base PR #25665 Rebased f1a9c713540b3f00cea3c29ace4958b77390a8ef -> fb94363ba45acd0b5ca702a60f41533a7b3fbec4 (pr/bresult-opt.3 -> pr/bresult-opt.4, compare) due to conflict with #25667 Rebased fb94363ba45acd0b5ca702a60f41533a7b3fbec4 -> 080268dc424301198ae93c3d002c13dfbfe6b9c9 (pr/bresult-opt.4 -> pr/bresult-opt.5, compare) due to conflict with #26251 and #26286 Rebased 080268dc424301198ae93c3d002c13dfbfe6b9c9 -> 2804dd4033f9ca636ff7df74453b9ddc048417de (pr/bresult-opt.5 -> pr/bresult-opt.6, compare) Rebased 2804dd4033f9ca636ff7df74453b9ddc048417de -> 4883d30d6a764d9fa551ceecdc807fa3283c891a (pr/bresult-opt.6 -> pr/bresult-opt.7, compare) on top of conflicted pr/bresult2.30, Rebased 4883d30d6a764d9fa551ceecdc807fa3283c891a -> 59c96ae3342d1e396a1df63d0af8457ecf60964a (pr/bresult-opt.7 -> pr/bresult-opt.8, compare) due to conflict with #27254 Rebased 59c96ae3342d1e396a1df63d0af8457ecf60964a -> 74655e5870888e0165c62fec75000fe04f062147 (pr/bresult-opt.8 -> pr/bresult-opt.9, compare) on top of pr/bresult2.33 #27254 Rebased 74655e5870888e0165c62fec75000fe04f062147 -> b2bcba068e2c5542ab157e62c82bb868f678beeb (pr/bresult-opt.9 -> pr/bresult-opt.10, compare) dropping dependency on #27254

Looks like there is a clang bug. Could be fixed by either bumping it again (to clang-13), see #27682, but I am not sure how to do that for macOS. Or by replacing return addrman; with return {std::move(addrman)};, or something like that (temporarily).

godbolt for clang-12 (broken): https://godbolt.org/z/1hraPjxrf

Thanks, added std::move as suggested.

Updated b2bcba068e2c5542ab157e62c82bb868f678beeb -> 8aa8f73adce72e1ae855b43413c1f65504423cb7 (pr/bresult-opt.10 -> pr/bresult-opt.11, compare) with workaround for CI errors https://cirrus-ci.com/task/6456067106799616?logs=ci#L1528 https://cirrus-ci.com/task/5048692223246336?logs=ci#L1240 https://cirrus-ci.com/task/6174592130088960?logs=ci#L1328

addrdb.cpp:213:12: error: call to implicitly-deleted copy constructor of 'util::Result<std::__1::unique_ptr<AddrMan, std::__1::default_delete<AddrMan> > >::T' (aka 'std::__1::unique_ptr<AddrMan, std::__1::default_delete<AddrMan> >')
    return addrman;
           ^~~~~~~
/usr/lib/llvm-10/bin/../include/c++/v1/memory:2513:3: note: copy constructor is implicitly deleted because 'unique_ptr<AddrMan, std::__1::default_delete<AddrMan> >' has a user-declared move constructor
  unique_ptr(unique_ptr&& __u) _NOEXCEPT
  ^
./util/result.h:47:14: note: passing argument to parameter 'obj' here
    Result(T obj) : m_variant{std::in_place_index_t<1>{}, std::move(obj)} {}
             ^
1 error generated.

very nice ACK 8aa8f73adce72e1ae855b43413c1f65504423cb 🏏

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: very nice ACK 8aa8f73adce72e1ae855b43413c1f65504423cb 🏏
FlnWlOVGKZVAPgirGB1Me+n1IHxeaKXXl2LHKPIpsnt+N2jG0ILNCy/guKB9V+K/JzCbDLy9h/UJBgDsXbspAw==

</details>

ACK 8aa8f73adce72e1ae855b43413c1f65504423cb7

I think [[nodiscard] qualifiers for all the util::Result return types would be nice.

ACK 8aa8f73adce72e1ae855b43413c1f65504423cb7, I have reviewed the code and it looks OK.

Looks like there is a clang bug. Could be fixed by either bumping it again, see #27682, but I am not sure how to do that for macOS. Or by replacing return addrman; with return {std::move(addrman)};, or something like that (temporarily).

Although GCC complains:

$ ./configure CONFIG_SITE=$PWD/depends/arm-linux-gnueabihf/share/config.site --enable-suppress-external-warnings CXXFLAGS="-Wextra -Wno-psabi"
$ make > /dev/null 
addrdb.cpp: In function ‘util::Result<std::unique_ptr<AddrMan> > LoadAddrman(const NetGroupManager&, const ArgsManager&)’:
addrdb.cpp:213:21: warning: redundant move in return statement [-Wredundant-move]
  213 |     return std::move(addrman); // std::move should be unneccessary but is temporarily needed to work around clang bug (https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1561270092)
      |            ~~~~~~~~~^~~~~~~~~
addrdb.cpp:213:21: note: remove ‘std::move’ call

Yeah, that's why I suggested {std::move()}, not std::move(), see #25977 (comment)