refactor: Add [[nodiscard]] to functions returning bool+mutable ref #34520

1. 
   maflcko commented at 4:58 pm on February 5, 2026: member

   Legacy code often used a function signature of the form bool Get...(const& in, mut& in_out); to implement a getter that optionally returns a value.

   In modern code, this can be replaced by std::optional<_> Get...(const& in);.

   However, some legacy code remains. To ensure that the “imaginary optional” is not unwrapped when the getter returns false, add a C++17 [[nodiscard]] attribute to all those legacy functions.

   There were only a few places that ignored the return value. I’ve fixed them in separate commits.

   This should be easy to review via --word-diff-regex=. and then confirming that the attribute was added to such a getter.

2. refactor: Add [[nodiscard]] to GetOp/GetScriptOp

   Also, use the return value in the test. Otherwise, compilation warns:
   
   src/test/transaction_tests.cpp:906:9: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
     906 |         t.vin[0].scriptSig.GetOp(pc, opcode); // advance to next op
         |         ^~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~
   1 warning generated.

   fa3a40a4a8
3. refactor: Add [[nodiscard]] to GetCachedLastHardenedExtPubKey

   Also, mark the return value as unused in one place, which is checked in
   the line after. This avoids a compile warning:
   
   src/script/descriptor.cpp:552:13: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
     552 |             cache->GetCachedLastHardenedExtPubKey(m_expr_index, xpub);
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~
   1 warning generated.

   fa18a0c2d9
4. DrahtBot renamed this:
   refactor: Add [[nodiscard]] to functions returning bool+mutable ref
   refactor: Add [[nodiscard]] to functions returning bool+mutable ref
   on Feb 5, 2026
5. DrahtBot added the label Refactoring on Feb 5, 2026
6. 
   DrahtBot commented at 4:58 pm on February 5, 2026: contributor

   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
   Concept ACK	sedited
   Stale ACK	HowHsu, yuvicc

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #34038 (logging: replace -loglevel with -trace, various API improvements by ajtowns)
   • #21283 (Implement BIP 370 PSBTv2 by achow101)

   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.

7. refactor: Add [[nodiscard]] to GetProxy

   Also, refactor the rpc code to check the return value (which is
   identical to proxy.IsValid()). Otherwise, the compiler would warn:
   
   src/rpc/net.cpp:622:9: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
     622 |         GetProxy(network, proxy);
         |         ^~~~~~~~ ~~~~~~~~~~~~~~
   1 warning generated.

   fa17ce6465
8. maflcko force-pushed on Feb 5, 2026
9. 
   HowHsu commented at 11:15 am on February 6, 2026: none

   ACK fac5685
10. 
    yuvicc commented at 6:17 am on February 18, 2026: contributor

    ACK fac5685e867b4a9ec334ed1ee31aabacd1c3124e
11. 
    sedited commented at 11:16 am on February 25, 2026: contributor

    Concept ACK

    This looks like a good thing to add, but I wonder if handling these cases incrementally is the correct approach. Outside of these “getter” functions there are so many cases where adding [[nodiscard]] make sense too. There is modernize-use-nodiscard, but that is too noisy. An alternative might be to roll our own clang tidy plugin that checks for bool return + mutable ref or pointer argument.

    Do you also want to add these couple of cases (also added one that takes a mutable pointer):

     0diff --git a/src/dbwrapper.h b/src/dbwrapper.h
     1index b2ce67c7c2..c2c2a94abd 100644
     2--- a/src/dbwrapper.h
     3+++ b/src/dbwrapper.h
     4@@ -154 +154 @@ public:
     5-    template<typename K> bool GetKey(K& key) {
     6+    template<typename K> [[nodiscard]] bool GetKey(K& key) {
     7@@ -164 +164 @@ public:
     8-    template<typename V> bool GetValue(V& value) {
     9+    template<typename V> [[nodiscard]] bool GetValue(V& value) {
    10diff --git a/src/qt/addresstablemodel.h b/src/qt/addresstablemodel.h
    11index 127378ed2f..80a5dc2ac3 100644
    12--- a/src/qt/addresstablemodel.h
    13+++ b/src/qt/addresstablemodel.h
    14@@ -99 +99 @@ private:
    15-    bool getAddressData(const QString &address, std::string* name, wallet::AddressPurpose* purpose) const;
    16+    [[nodiscard]] bool getAddressData(const QString &address, std::string* name, wallet::AddressPurpose* purpose) const;
    17diff --git a/src/txdb.cpp b/src/txdb.cpp
    18index 27fb76fc4a..a0f55b1cd1 100644
    19--- a/src/txdb.cpp
    20+++ b/src/txdb.cpp
    21@@ -199 +199 @@ std::unique_ptr<CCoinsViewCursor> CCoinsViewDB::Cursor() const
    22-        i->pcursor->GetKey(entry);
    23+        (void)i->pcursor->GetKey(entry);
    

12. maflcko force-pushed on Feb 26, 2026
13. refactor: Add [[nodiscard]] to functions returning bool+mutable ref fa4db474a8
14. maflcko force-pushed on Feb 26, 2026
15. DrahtBot added the label CI failed on Feb 26, 2026
16. 
    maflcko commented at 1:36 pm on February 26, 2026: member

    This looks like a good thing to add, but I wonder if handling these cases incrementally is the correct approach.

    I wondered that, too. The patch here was written in 2018, but fell off the table. While I rebased it, some places where transformed to std::optional (which should be used in a few more places instead of nodiscard).

    Outside of these “getter” functions there are so many cases where adding [[nodiscard]] make sense too.

    Are there? I think the attribute should only be used where it really makes sense.

    modernize-use-nodiscard

    I think this will do the exact opposite of what we want: It ignores functions that have a mutable ref param. It only adds it to functions that don’t really need it in the first place.

    Happy to take a look at writing a clang-tidy plugin, maybe as a follow-up?

    dbwrapper.h

    I’ve also seen this one, but left it for a follow-up, because it is not trivial to see if and why (void) would be the correct fix here. I wanted to keep this pull focussed on mostly mechanical changes for now.

    getAddressData

    Thx, done

17. DrahtBot removed the label CI failed on Feb 26, 2026

Contributors
maflcko DrahtBot HowHsu yuvicc sedited

Labels
Refactoring