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

ReviewabilityReviewsAgreementSize
#34520 maflcko refactor: Add [[nodiscard]] to functions returning bool+mutable refCI failing0 (+2)Strong74 + 1 tests
  • Prevent callers from accidentally using invalid or uninitialized output when getter functions fail
  • Ensure callers check return values by enforcing compiler warnings on legacy retrieval functions
  • CI is failing, so check whether the failure is caused by this PR before doing a deep review
  • Strong support with no open objections (sedited, ryanofsky)
  • Support because preventing ignored returns could prevent bugs (ryanofsky)
  • Concept support but questioned whether an incremental approach is best (sedited)
  • Addressed earlier concerns on proxy isolation and descriptor cache handling (ajtowns)
  • 74 lines added or modified outside tests
  • 1 lines added or modified in tests
  • 77 lines removed in total
  • 22 files, 3 commits
Wallet#81 of 92
P4 · cleanup
  • P4 because it is internal defensive hygiene adding compiler warnings for ignored return values
  • It fixes no active wallet bug or user-visible problem
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Annotate legacy getter and signing functions that return a boolean success flag alongside mutable out-parameters with the C++17 `[[nodiscard]]` attribute across the codebase. Address callers that previously ignored the return value by asserting success in witness sigop counting, checking descriptor cache return status, and wrapping test iteration in `BOOST_REQUIRE`.

Problem: Legacy getters using the `bool Get...(..., mut& out)` idiom allow callers to ignore return values and inadvertently use uninitialized or invalid out-parameters when retrieval fails.

Evidence of importance:

  • PR description: "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."
  • Commit 90a82662a9 notes that ignoring the return value of `GetOp` triggers compiler warnings once annotated: `warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]`

Scope: Touches diverse subsystems including script interpretation, descriptor caching, wallet interfaces, and networking to add compiler attributes.

Risk of a mistake: medium (consensus, wallet, p2p). It introduces an Assert in CountWitnessSigOps within consensus-critical interpreter code; if the invariant on IsPushOnly fails under any condition, nodes would crash.

Changed since the previous assessment: No substantive changes; commits and approach remain unchanged from the previous version.

Reviewability

Ready · CI failing

CI is red; check whether the failure is the PR's before a deep review.

Agreement

Strong

substantive support, no open objection (sedited, ryanofsky)

Thread notes: Initial assessment. Multiple concerns from ajtowns were addressed by pushing fixes or spinning off PR #34741. ryanofsky raised an objection regarding Assume/Assert in libbitcoin_consensus (CountWitnessSigOps), which caused check-deps failures in CI; the author agreed to use assert for now, but a fix has not yet been pushed, leaving the claim open.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
ajtowns (member)objection, correctnessproxy.m_tor_stream_isolation was not conditional on is_validfixed · fix pushed · v1, not yet re-checkedansweredyes2026-03-05: 'Shouldn't `proxy.m_tor_stream_isolation` also be conditional on `is_valid` ?'
Status from: 2026-03-05: maflcko: 'Yeah, but I guess `std::optional` could make more sense. Done in https://github.com/bitcoin/bitcoin/pull/34741 and moved this into draft for now. edit: now merged'
ajtowns (member)objection, approachdeclaring nodiscard but casting to void in consensus code discards safety guaranteesfixed · fix pushed · v1, not yet re-checkedansweredyes2026-03-05: 'I'm uncomfortable with both declaring `[[nodiscard] bool GetOp(...)` and then discarding the return value in consensus code (or non-test code in general).'
Status from: 2026-03-13: maflcko: 'thx, done'
ajtowns (member)objection, correctnessdescriptor cache misses or invalid cached pubkeys are not handled properlyfixed · fix pushed · v1, not yet re-checkedansweredyes2026-03-05: 'if (cache == nullptr || !cache->GetCachedLastHardenedExtPubKey(m_expr_index,xpub) || !xpub.pubkey.IsValid())'
Status from: 2026-03-13: maflcko: 'edit: thx, done'
ryanofsky (member)objection, approachusing Assume or Assert introduces unwanted library dependencies into libbitcoin_consensus or inconsistent release/debug behavioranswered · reply on record; standing not assessed yet · v1, not yet re-checkedansweredyes2026-04-07: '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.'

Support:

  • HowHsu (contributor) (ACK, 2026-02-06): ACK fac5685 [not substantive]
  • yuvicc (contributor) (ACK, 2026-02-18): ACK fac5685e867b4a9ec334ed1ee31aabacd1c3124e [not substantive]
  • sedited (member) (Concept ACK, 2026-02-25): This looks like a good thing to add, but I wonder if handling these cases incrementally is the correct approach.
  • ajtowns (member) (Concept ACK, 2026-03-05): Concept ACK [not substantive]
  • ryanofsky (member) (ACK, 2026-04-07): FIrst commit does not seem right to me but otherwise these are nice changes that could prevent bugs.

Participants:

  • HowHsu (contributor, support, 1 statements 2026-02-06 to 2026-02-06): ACKed commit fac5685
  • yuvicc (contributor, support, 1 statements 2026-02-18 to 2026-02-18): ACKed commit fac5685
  • sedited (member, support, 1 statements 2026-02-25 to 2026-02-25): Concept ACKed, suggested considering clang-tidy tooling and noted extra getter candidates
  • ajtowns (member, support, 4 statements 2026-03-05 to 2026-03-05): Concept ACKed, raised concerns about proxy isolation, consensus GetOp return handling, and descriptor cache handling
  • ryanofsky (member, support, 5 statements 2026-04-07 to 2026-04-08): Approved with comments; objected to Assume/Assert in consensus CountWitnessSigOps triggering library dependency issues

Files

2 lines under test/bench/ci.

  • src/script/signingprovider.h +30/-30
  • src/wallet/scriptpubkeyman.h +10/-10
  • src/script/descriptor.cpp +3/-6
  • src/interfaces/wallet.h +4/-4
  • src/netaddress.h +3/-3
  • src/script/descriptor.h +3/-3
  • src/script/script.h +3/-3
  • src/script/sign.cpp +2/-2
  • src/txdb.cpp +2/-2
  • src/wallet/wallet.h +2/-2
  • src/script/interpreter.cpp +2/-1
  • src/common/args.h +1/-1
  • src/common/config.cpp +1/-1
  • src/external_signer.h +1/-1
  • src/interfaces/node.h +1/-1
  • src/musig.cpp +1/-1
  • src/net_processing.cpp +1/-1
  • src/net_processing.h +1/-1
  • src/psbt.h +1/-1
  • src/qt/addresstablemodel.h +1/-1
  • src/test/transaction_tests.cpp +1/-1
  • src/wallet/feebumper.h +1/-1

Card

The short text the ranking pass compares across PRs.

This PR adds [[nodiscard]] attributes across the codebase to legacy getter functions that return a boolean success flag alongside mutable out-parameters. It prevents callers from inadvertently ignoring failure status and using invalid out-parameters. The PR also fixes existing call sites where return values were previously ignored, including adding an assertion inside script witness sigop counting and checking cache retrieval results during descriptor evaluation.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-25T01:14, confidence high, input hash b62f101145fba866
thread read through 2026-04-08 (19 statements), head 1bd568e2c7
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Refactoring, CI failed · draft

Categories

Wallet

P4 · cleanup

Adds `[[nodiscard]]` to key and script retrieval interfaces in `SigningProvider` and `ScriptPubKeyMan` to ensure callers do not ignore return values. While useful defensive hygiene to prevent using uninitialized or invalid out-parameters, it is an internal compiler-attribute refactor with no active wallet bug fix.

Membership: Annotates legacy key and script retrieval interfaces across src/script/signingprovider.h, src/wallet/scriptpubkeyman.h, and src/wallet/wallet.h with [[nodiscard]].

Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 0