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

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.

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

DrahtBot added the label Refactoring on Feb 5, 2026

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, ajtowns
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:

#34806 (refactor: logging: Various API improvements by ajtowns)
#34741 (refactor: Return std::optional from GetNameProxy/GetProxy by maflcko)
#34729 (Reduce log noise by ajtowns)
#34486 (net: Reduce local network activity when networkactive=0 by willcl-ark)
#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.

maflcko force-pushed on Feb 5, 2026

HowHsu commented at 11:15 am on February 6, 2026: none

ACK fac5685

yuvicc commented at 6:17 am on February 18, 2026: contributor

ACK fac5685e867b4a9ec334ed1ee31aabacd1c3124e

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);

maflcko force-pushed on Feb 26, 2026

DrahtBot added the label CI failed on Feb 26, 2026

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

DrahtBot removed the label CI failed on Feb 26, 2026

in src/rpc/net.cpp:627 in fa4db474a8

624         obj.pushKV("name", GetNetworkName(network));
625         obj.pushKV("limited", !g_reachable_nets.Contains(network));
626         obj.pushKV("reachable", g_reachable_nets.Contains(network));
627-        obj.pushKV("proxy", proxy.IsValid() ? proxy.ToString() : std::string());
628+        obj.pushKV("proxy", is_valid ? proxy.ToString() : std::string{});
629         obj.pushKV("proxy_randomize_credentials", proxy.m_tor_stream_isolation);

ajtowns commented at 8:37 am on March 5, 2026:

Shouldn’t proxy.m_tor_stream_isolation also be conditional on is_valid ?

maflcko commented at 12:36 pm on March 5, 2026:

Yeah, but I guess std::optional could make more sense. Done in #34741 and moved this into draft for now.

in src/script/interpreter.cpp:2161 in fa4db474a8

2157@@ -2158,7 +2158,7 @@ size_t CountWitnessSigOps(const CScript& scriptSig, const CScript& scriptPubKey,
2158         std::vector<unsigned char> data;
2159         while (pc < scriptSig.end()) {
2160             opcodetype opcode;
2161-            scriptSig.GetOp(pc, opcode, data);
2162+            (void)scriptSig.GetOp(pc, opcode, data);

ajtowns commented at 8:44 am on March 5, 2026:

I’m uncomfortable with both declaring [[nodiscard] bool GetOp(...) and then discarding the return value in consensus code (or non-test code in general).

The earlier scriptSig.IsPushOnly() should guarantee that GetOp will never return false, so perhaps Assume(scriptSig.GetOp(pc, opcode, data)) would be better here?

maflcko commented at 2:43 pm on March 13, 2026:

thx, done

in src/script/descriptor.cpp:552 in fa18a0c2d9 outdated

548@@ -549,7 +549,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider
549         CExtKey lh_xprv;
550         // If we have the cache, just get the parent xpub
551         if (cache != nullptr) {
552-            cache->GetCachedLastHardenedExtPubKey(m_expr_index, xpub);
553+            (void)cache->GetCachedLastHardenedExtPubKey(m_expr_index, xpub);

ajtowns commented at 8:48 am on March 5, 2026:

Perhaps:

0if (cache == nullptr || !cache->GetCachedLastHardenedExtPubKey(m_expr_index,xpub) || !xpub.pubkey.IsValid()) {
1    // cache miss, or not cache, or need privkey
2    ...
3}
4assert(xpub.pubkey.IsValid());

maflcko commented at 2:43 pm on March 13, 2026:

Haven’t looked here. This is still in draft and I may take a look on the next rebase.

ajtowns commented at 8:49 am on March 5, 2026: contributor

Concept ACK

maflcko marked this as a draft on Mar 5, 2026

DrahtBot added the label Needs rebase on Mar 13, 2026

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.

6fd7374982

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.

c4c99a6950

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.

47dc58f197

opt proxy 82b349090b

refactor: Add [[nodiscard]] to functions returning bool+mutable ref 597b75f128

maflcko force-pushed on Mar 13, 2026

DrahtBot removed the label Needs rebase on Mar 13, 2026

DrahtBot added the label CI failed on Mar 13, 2026

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

Reviews

Conflicts