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

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 <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35011 (ci, iwyu: Fix warnings in src/script and treat them as errors by BrandonOdiwuor)
#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: contributor

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

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

edit: now merged

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:

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

edit: thx, done

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

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

maflcko force-pushed on Mar 26, 2026

DrahtBot removed the label CI failed on Mar 26, 2026

maflcko force-pushed on Mar 31, 2026

DrahtBot added the label Needs rebase on Apr 3, 2026

maflcko force-pushed on Apr 3, 2026

DrahtBot removed the label Needs rebase on Apr 3, 2026

maflcko marked this as ready for review on Apr 3, 2026

maflcko commented at 2:08 PM on April 3, 2026: member

Rebased, and taken out of draft, now that all feedback is addressed.

in src/script/interpreter.cpp:2158 in fa9e0570a6

2154 | @@ -2154,7 +2155,7 @@ size_t CountWitnessSigOps(const CScript& scriptSig, const CScript& scriptPubKey,
2155 |          std::vector<unsigned char> data;
2156 |          while (pc < scriptSig.end()) {
2157 |              opcodetype opcode;
2158 | -            scriptSig.GetOp(pc, opcode, data);
2159 | +            Assume(scriptSig.GetOp(pc, opcode, data));

ryanofsky commented at 2:40 PM on April 7, 2026:

In commit "refactor: Add [[nodiscard]] to GetOp/GetScriptOp" (fa9e0570a6e1da609d052e6d2bcdc9744b719c42)

I don't think it makes sense to use Assume here. Logically CountWitnessSigOps could either be intended to tolerate malformed scripts, in which case it should ignore the GetOp return value, or intended not to tolerate them, in which case it should abort or raise an exception. I don't see how it could realistically be intended to tolerate malformed scripts in release builds but not debug builds.

maflcko commented at 7:13 PM on April 7, 2026:

This used to be (void), but then I changed it to Assume after a review comment (https://github.com/bitcoin/bitcoin/pull/34520#discussion_r2888547507). Now I changed to Assert, but I guess tidy uses a release build an now contrib/devtools/check-deps.sh fails with:


Error: interpreter.cpp.o depends on check.cpp.o symbol 'assertion_fail(std::source_location const&, std::basic_string_view<char, std::char_traits<char> >)', can suppress with:
    SUPPRESS["interpreter.cpp.o check.cpp.o _Z14assertion_failRKSt15source_locationSt17basic_string_viewIcSt11char_traitsIcEE"]=1
Error: Unexpected dependencies were detected. Check previous output.

Not sure how to fix this. I guess I'd have to move util/check.h to crypto? According to:

doc/design/libraries.md:98:- *libbitcoin_consensus* should only depend on *libbitcoin_crypto*, and all other libraries besides *libbitcoin_crypto* should be allowed to depend on it.

ryanofsky commented at 7:58 PM on April 7, 2026:

re: #34520 (review)

Not sure how to fix this.

Would suggest a plain assert:

bool got_op = scriptSig.GetOp(pc, opcode, data);;
assert(got_op); // Guaranteed by IsPushOnly()

Or maybe it is fine for consensus to depend on util though now it is no longer distributed as a separate library. Moving check to crypto could also be ok. Plain assert seems best to me though.

maflcko commented at 9:01 PM on April 7, 2026:

Ok, I may use assert for now, but it seems better to answer the library design question. Otherwise, it will come up again in the future. There is also kernel_util from #28690, so possibly there is no need consensus to depend on all of util, but it may be fine to depend on kernel_util.

ryanofsky commented at 12:57 PM on April 8, 2026:

Ok, I may use assert for now, but it seems better to answer the library design question.

Yes, it looks like the underlying problem is that util/check.h depends on some globals like g_detail_test_only_CheckFailuresAreExceptionsNotAborts and g_enable_dynamic_fuzz_determinism which need to live in some library. Otherwise check.h could be a header-only dependency.

I don't think it particularly matters where those globals live. The easiest thing to do which preserves existing dependencies and would not be a bad solution IMO would be to move these globals to the crypto library like hex_base was moved there in #29015, since everything can depend on crypto. Or maybe introduce a small core or globals library to hold these and maybe other things like G_TRANSLATION_FUN. The kernel_util idea from #28690 also seems ok (with some different naming since probably wallet and consensus libraries should not depend on a iibrary with "kernel" in the name)

Another option would to just let the consensus library depend on util. I don't think there would be harm from that if consensus is not distributed separately.

But I don't think it would be good to make library dependency changes in this PR if assert fits well here, which it seems like it does.

in src/script/descriptor.cpp:553 in 4444e4abc1 outdated

 549 | @@ -550,10 +550,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider
 550 |          CExtPubKey xpub;
 551 |          CExtKey lh_xprv;
 552 |          // If we have the cache, just get the parent xpub
 553 | -        if (cache != nullptr) {
 554 | -            cache->GetCachedLastHardenedExtPubKey(m_expr_index, xpub);
 555 | -        }
 556 | -        if (!xpub.pubkey.IsValid()) {
 557 | +        if (!cache || !cache->GetCachedLastHardenedExtPubKey(m_expr_index, xpub) || !xpub.pubkey.IsValid()) {

ryanofsky commented at 2:48 PM on April 7, 2026:

In commit "refactor: Add [[nodiscard]] to GetCachedLastHardenedExtPubKey" (4444e4abc181ccaadeb8788eb4ab90f0a63ab485)

Looks like commit message should be updated. It says "mark the return value as unused in one place" which is not what this doing (it is using the return value as an optimization)

ryanofsky approved

ryanofsky commented at 2:53 PM on April 7, 2026: contributor

Code review fa97a912e836943b33d21e83807fb8784e7abceb. FIrst commit does not seem right to me but otherwise these are nice changes that could prevent bugs.

DrahtBot requested review from sedited on Apr 7, 2026

DrahtBot requested review from ajtowns on Apr 7, 2026

maflcko force-pushed on Apr 7, 2026

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

Also, assert the return value in CountWitnessSigOps, since it can never
be false due to the prior guard on IsPushOnly().

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.

faf261d711

refactor: Add [[nodiscard]] to GetCachedLastHardenedExtPubKey

Also, use the return value in one place. 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.

fae767ff7a

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

maflcko force-pushed on Apr 7, 2026

DrahtBot added the label CI failed on Apr 7, 2026

maflcko marked this as a draft on Apr 7, 2026