refactor: Add [[nodiscard]] to functions returning bool+mutable ref by maflcko · Pull Request #34520 · bitcoin/bitcoin

maflcko · 2026-02-05T16:58:07Z

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 · 2026-02-05T16:58:45Z

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  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.

HowHsu · 2026-02-06T11:15:08Z

ACK fac5685

yuvicc · 2026-02-18T06:17:51Z

ACK fac5685

sedited · 2026-02-25T11:16:02Z

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

diff --git a/src/dbwrapper.h b/src/dbwrapper.h
index b2ce67c7c2..c2c2a94abd 100644
--- a/src/dbwrapper.h
+++ b/src/dbwrapper.h
@@ -154 +154 @@ public:
-    template<typename K> bool GetKey(K& key) {
+    template<typename K> [[nodiscard]] bool GetKey(K& key) {
@@ -164 +164 @@ public:
-    template<typename V> bool GetValue(V& value) {
+    template<typename V> [[nodiscard]] bool GetValue(V& value) {
diff --git a/src/qt/addresstablemodel.h b/src/qt/addresstablemodel.h
index 127378ed2f..80a5dc2ac3 100644
--- a/src/qt/addresstablemodel.h
+++ b/src/qt/addresstablemodel.h
@@ -99 +99 @@ private:
-    bool getAddressData(const QString &address, std::string* name, wallet::AddressPurpose* purpose) const;
+    [[nodiscard]] bool getAddressData(const QString &address, std::string* name, wallet::AddressPurpose* purpose) const;
diff --git a/src/txdb.cpp b/src/txdb.cpp
index 27fb76fc4a..a0f55b1cd1 100644
--- a/src/txdb.cpp
+++ b/src/txdb.cpp
@@ -199 +199 @@ std::unique_ptr<CCoinsViewCursor> CCoinsViewDB::Cursor() const
-        i->pcursor->GetKey(entry);
+        (void)i->pcursor->GetKey(entry);

maflcko · 2026-02-26T13:36:42Z

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

ajtowns

Concept ACK

ajtowns · 2026-03-05T08:37:25Z

src/rpc/net.cpp

        obj.pushKV("reachable", g_reachable_nets.Contains(network));
-        obj.pushKV("proxy", proxy.IsValid() ? proxy.ToString() : std::string());
+        obj.pushKV("proxy", is_valid ? proxy.ToString() : std::string{});
        obj.pushKV("proxy_randomize_credentials", proxy.m_tor_stream_isolation);


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

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

ajtowns · 2026-03-05T08:44:48Z

src/script/interpreter.cpp

        while (pc < scriptSig.end()) {
            opcodetype opcode;
-            scriptSig.GetOp(pc, opcode, data);
+            (void)scriptSig.GetOp(pc, opcode, data);


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?

ajtowns · 2026-03-05T08:48:55Z

src/script/descriptor.cpp

        // If we have the cache, just get the parent xpub
        if (cache != nullptr) {
-            cache->GetCachedLastHardenedExtPubKey(m_expr_index, xpub);
+            (void)cache->GetCachedLastHardenedExtPubKey(m_expr_index, xpub);


Perhaps:

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

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

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.

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.

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.

DrahtBot changed the title ~~refactor: Add [[nodiscard]] to functions returning bool+mutable ref~~ refactor: Add [[nodiscard]] to functions returning bool+mutable ref Feb 5, 2026

DrahtBot added the Refactoring label Feb 5, 2026

maflcko force-pushed the 2602-nodiscard branch from facb886 to fac5685 Compare February 5, 2026 18:21

This was referenced Feb 6, 2026

logging: replace -loglevel with -trace, expose trace logging via RPC #34038

Open

Implement BIP 370 PSBTv2 #21283

Open

maflcko force-pushed the 2602-nodiscard branch 2 times, most recently from fadf578 to fa4db47 Compare February 26, 2026 13:33

DrahtBot added the CI failed label Feb 26, 2026

DrahtBot removed the CI failed label Feb 26, 2026

DrahtBot mentioned this pull request Mar 4, 2026

Reduce log noise #34729

Open

ajtowns reviewed Mar 5, 2026

View reviewed changes

maflcko marked this pull request as draft March 5, 2026 12:35

This was referenced Mar 5, 2026

refactor: Return std::optional from GetNameProxy/GetProxy #34741

Open

refactor: replace ArgsManager::cs_args RecursiveMutex with Mutex #34745

Merged

DrahtBot added the Needs rebase label Mar 13, 2026

MarcoFalke added 5 commits March 13, 2026 15:40

opt proxy

82b3490

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

597b75f

maflcko force-pushed the 2602-nodiscard branch from fa4db47 to 597b75f Compare March 13, 2026 14:41

DrahtBot added CI failed and removed Needs rebase labels Mar 13, 2026

This was referenced Mar 14, 2026

refactor: logging: Various API improvements #34806

Open

net: Reduce local network activity when networkactive=0 #34486

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

refactor: Add [[nodiscard]] to functions returning bool+mutable ref#34520
maflcko wants to merge 5 commits intobitcoin:masterfrom
maflcko:2602-nodiscard

maflcko commented Feb 5, 2026

Uh oh!

DrahtBot commented Feb 5, 2026 •

edited

Loading

Uh oh!

HowHsu commented Feb 6, 2026

Uh oh!

yuvicc commented Feb 18, 2026

Uh oh!

sedited commented Feb 25, 2026

Uh oh!

maflcko commented Feb 26, 2026

Uh oh!

ajtowns left a comment

Uh oh!

ajtowns Mar 5, 2026

Uh oh!

maflcko Mar 5, 2026

Uh oh!

ajtowns Mar 5, 2026

Uh oh!

maflcko Mar 13, 2026

Uh oh!

ajtowns Mar 5, 2026

Uh oh!

maflcko Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

maflcko commented Feb 5, 2026

Uh oh!

DrahtBot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

HowHsu commented Feb 6, 2026

Uh oh!

yuvicc commented Feb 18, 2026

Uh oh!

sedited commented Feb 25, 2026

Uh oh!

maflcko commented Feb 26, 2026

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

ajtowns Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

ajtowns Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Feb 5, 2026 •

edited

Loading