refactor: Avoid copies by using const references or by move-construction #31650

maflcko · 2025-01-13T20:41:34Z

Top level const in declarations is problematic for many reasons:

It is often a typo, where one wanted to denote a const reference. For example bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, ... is missing the &. This will create a redundant copy of the value.
In constructors it prevents move construction.
It can incorrectly imply some data is const, like in an imaginary example std::span<int> Shuffle(const std::span<int>);, where the ints are not const.
The compiler ignores the const from the declaration in the implementation.
It isn't used consistently anyway, not even on the same line.

Fix some issues by:

Using a const reference to avoid a copy, where read-only of the value is intended. This is only done for values that may be expensive to copy.
Using move-construction to avoid a copy
Applying readability-avoid-const-params-in-decls via clang-tidy

DrahtBot · 2025-01-13T20:41:38Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31650.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, l0rinc
Concept ACK	fanquake
Stale ACK	rkrux, ryanofsky, pablomartin4btc, sedited

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:

#34296 (refactor: [move-only] Merge core_io module, remove from libkernel by maflcko)
#34181 (refactor: [p2p] Make ProcessMessage private again, Use references when non-null by maflcko)
#33663 (net: Filter addrman during address selection via AddrPolicy to avoid underfill by waketraindev)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#33008 (wallet: support bip388 policy with external signer by Sjors)
#32958 (wallet/refactor: change PSBTError to PSBTResult and remove std::optionalcommon::PSBTResult and return common::PSBTResult by kevkevinpal)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt 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.

hebasto · 2025-01-14T09:01:46Z

Concept ACK.

fanquake · 2025-02-20T20:51:19Z

Concept ACK - has enforcement, few conflicts, added to 30.x.

ryanofsky

Code review ACK fa8e57951abc5c737ab68694b8f6aa77551faa6b. Agree it would be nice to enforce this check to reduce noise and prevent potential bugs

ryanofsky · 2025-03-27T18:53:09Z

src/external_signer.h

In commit "refactor: Avoid copies by using const references or by move-construction" (fafe09b0161c8c8d22a1e2e1bad0428b51fca813)

This change seems reasonable but would point out that sometimes short string copies like this can be intentional:
#24306 (comment). I'd probably go with this change, but I don't actually know if copying might be better.

I'd be surprised if anything made a difference here in this function and I think anything is fine here (std::string copy, const std::string& reference, or even a span or std::string_view).

ryanofsky · 2025-03-27T18:58:33Z

src/crypto/hex_base.h

In commit "refactor: Enforce readability-avoid-const-params-in-decls" (fa8e57951abc5c737ab68694b8f6aa77551faa6b)

Since these are definitions not declarations, I don't think these actually need to change to fix the warnings. Does seem good to change, though.

l0rinc

Concept ACK - this should clear up a few const usages.

After rebasing I found a few other similar ones, but not sure if they were ignored on purpose or not.
Please consider the ones that are related to this change - and just skip the ones that you've ignored on purpose.

Details

diff --git a/src/cuckoocache.h b/src/cuckoocache.h
index 8370179395..9dcceec739 100644
--- a/src/cuckoocache.h
+++ b/src/cuckoocache.h
@@ -471,7 +471,7 @@ public:
      * flag is set
      * @returns true if the element is found, false otherwise
      */
-    inline bool contains(const Element& e, const bool erase) const
+    inline bool contains(const Element& e, bool erase) const
     {
         std::array<uint32_t, 8> locs = compute_hashes(e);
         for (const uint32_t loc : locs)
diff --git a/src/deploymentinfo.cpp b/src/deploymentinfo.cpp
index 185a7dcb54..237de65820 100644
--- a/src/deploymentinfo.cpp
+++ b/src/deploymentinfo.cpp
@@ -37,7 +37,7 @@ std::string DeploymentName(Consensus::BuriedDeployment dep)
     return "";
 }
 
-std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(const std::string_view name)
+std::optional<Consensus::BuriedDeployment> GetBuriedDeployment(std::string_view name)
 {
     if (name == "segwit") {
         return Consensus::BuriedDeployment::DEPLOYMENT_SEGWIT;
diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index 2adeaea652..4d2fa5013b 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -207,7 +207,7 @@ struct TransactionInfo {
     /* The block height the transaction entered the mempool */
     const unsigned int txHeight;
 
-    TransactionInfo(const CTransactionRef& tx, const CAmount& fee, const int64_t vsize, const unsigned int height)
+    TransactionInfo(const CTransactionRef& tx, const CAmount& fee, int64_t vsize, unsigned int height)
         : m_tx{tx},
           m_fee{fee},
           m_virtual_transaction_size{vsize},
@@ -238,10 +238,10 @@ struct NewMempoolTransactionInfo {
     const bool m_has_no_mempool_parents;
 
     explicit NewMempoolTransactionInfo(const CTransactionRef& tx, const CAmount& fee,
-                                       const int64_t vsize, const unsigned int height,
-                                       const bool mempool_limit_bypassed, const bool submitted_in_package,
-                                       const bool chainstate_is_current,
-                                       const bool has_no_mempool_parents)
+                                       const int64_t vsize, unsigned int height,
+                                       bool mempool_limit_bypassed, bool submitted_in_package,
+                                       bool chainstate_is_current,
+                                       bool has_no_mempool_parents)
         : info{tx, fee, vsize, height},
           m_mempool_limit_bypassed{mempool_limit_bypassed},
           m_submitted_in_package{submitted_in_package},
diff --git a/src/logging.h b/src/logging.h
index fdc12c79b3..de0e1cab7e 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -239,7 +239,7 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
 bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
 
 template <typename... Args>
-inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
+inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags flag, BCLog::Level level, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
 {
     if (LogInstance().Enabled()) {
         std::string log_msg;
diff --git a/src/netbase.h b/src/netbase.h
index d7c6e1c215..02b61a29de 100644
--- a/src/netbase.h
+++ b/src/netbase.h
@@ -60,7 +60,7 @@ class Proxy
 public:
     Proxy() : m_is_unix_socket(false), m_randomize_credentials(false) {}
     explicit Proxy(const CService& _proxy, bool _randomize_credentials = false) : proxy(_proxy), m_is_unix_socket(false), m_randomize_credentials(_randomize_credentials) {}
-    explicit Proxy(const std::string path, bool _randomize_credentials = false) : m_unix_socket_path(path), m_is_unix_socket(true), m_randomize_credentials(_randomize_credentials) {}
+    explicit Proxy(std::string path, bool _randomize_credentials = false) : m_unix_socket_path(std::move(path)), m_is_unix_socket(true), m_randomize_credentials(_randomize_credentials) {}
 
     CService proxy;
     std::string m_unix_socket_path;
diff --git a/src/node/miner.h b/src/node/miner.h
index c09e9eb5d1..2940df5435 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -220,7 +220,7 @@ private:
  * accounts for the BIP94 timewarp rule, so does not necessarily reflect the
  * consensus limit.
  */
-int64_t GetMinimumTime(const CBlockIndex* pindexPrev, const int64_t difficulty_adjustment_interval);
+int64_t GetMinimumTime(const CBlockIndex* pindexPrev, int64_t difficulty_adjustment_interval);
 
 int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
 
diff --git a/src/qt/addresstablemodel.h b/src/qt/addresstablemodel.h
index 877b01d882..595b815afd 100644
--- a/src/qt/addresstablemodel.h
+++ b/src/qt/addresstablemodel.h
@@ -90,7 +90,7 @@ public:
     QString GetWalletDisplayName() const;
 
 private:
-    WalletModel* const walletModel;
+    WalletModel* walletModel;
     AddressTablePriv *priv = nullptr;
     QStringList columns;
     EditStatus editStatus = OK;
diff --git a/src/qt/walletview.h b/src/qt/walletview.h
index 475085044d..687f22234a 100644
--- a/src/qt/walletview.h
+++ b/src/qt/walletview.h
@@ -56,7 +56,7 @@ private:
     //! The wallet model represents a bitcoin wallet, and offers access to
     //! the list of transactions, address book and sending functionality.
     //!
-    WalletModel* const walletModel;
+    WalletModel* walletModel;
 
     OverviewPage *overviewPage;
     QWidget *transactionsPage;
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 041186cae0..feb650bcf2 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -1773,7 +1773,7 @@ std::optional<std::pair<std::vector<unsigned char>, int>> ParseHexStrEnd(std::sp
 
 /** BuildBack pops the last two elements off `constructed` and wraps them in the specified Fragment */
 template<typename Key>
-void BuildBack(const MiniscriptContext script_ctx, Fragment nt, std::vector<NodeRef<Key>>& constructed, const bool reverse = false)
+void BuildBack(const MiniscriptContext script_ctx, Fragment nt, std::vector<NodeRef<Key>>& constructed, bool reverse = false)
 {
     NodeRef<Key> child = std::move(constructed.back());
     constructed.pop_back();
@@ -1814,7 +1814,7 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
     to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1);
 
     // Parses a multi() or multi_a() from its string representation. Returns false on parsing error.
-    const auto parse_multi_exp = [&](std::span<const char>& in, const bool is_multi_a) -> bool {
+    const auto parse_multi_exp = [&](std::span<const char>& in, bool is_multi_a) -> bool {
         const auto max_keys{is_multi_a ? MAX_PUBKEYS_PER_MULTI_A : MAX_PUBKEYS_PER_MULTISIG};
         const auto required_ctx{is_multi_a ? MiniscriptContext::TAPSCRIPT : MiniscriptContext::P2WSH};
         if (ctx.MsContext() != required_ctx) return false;
diff --git a/src/streams.h b/src/streams.h
index 20bdaf2c06..f6bc0478a3 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -547,7 +547,7 @@ public:
 
     //! Move the read position ahead in the stream to the given position.
     //! Use SetPos() to back up in the stream, not SkipTo().
-    void SkipTo(const uint64_t file_pos)
+    void SkipTo(uint64_t file_pos)
     {
         assert(file_pos >= m_read_pos);
         while (m_read_pos < file_pos) AdvanceStream(file_pos - m_read_pos);
diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
index 297595a9cf..92ffacdfdc 100644
--- a/src/test/argsman_tests.cpp
+++ b/src/test/argsman_tests.cpp
@@ -52,7 +52,7 @@ BOOST_AUTO_TEST_CASE(util_datadir)
 struct TestArgsManager : public ArgsManager
 {
     TestArgsManager() { m_network_only_args.clear(); }
-    void ReadConfigString(const std::string str_config)
+    void ReadConfigString(const std::string& str_config)
     {
         std::istringstream streamConfig(str_config);
         {
@@ -63,7 +63,7 @@ struct TestArgsManager : public ArgsManager
         std::string error;
         BOOST_REQUIRE(ReadConfigStream(streamConfig, "", error));
     }
-    void SetNetworkOnlyArg(const std::string arg)
+    void SetNetworkOnlyArg(const std::string& arg)
     {
         LOCK(cs_args);
         m_network_only_args.insert(arg);
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 52d7bd0efc..3bea74d9f7 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -258,7 +258,7 @@ struct TestChain100Setup : public TestingSetup {
  * be used in "hot loops", for example fuzzing or benchmarking.
  */
 template <class T = const BasicTestingSetup>
-std::unique_ptr<T> MakeNoLogFileContext(const ChainType chain_type = ChainType::REGTEST, TestOpts opts = {})
+std::unique_ptr<T> MakeNoLogFileContext(ChainType chain_type = ChainType::REGTEST, TestOpts opts = {})
 {
     opts.extra_args = Cat(
         {
diff --git a/src/uint256.cpp b/src/uint256.cpp
index 2756a7f5cd..f9fe8187a6 100644
--- a/src/uint256.cpp
+++ b/src/uint256.cpp
@@ -18,7 +18,7 @@ std::string base_blob<BITS>::GetHex() const
 }
 
 template <unsigned int BITS>
-void base_blob<BITS>::SetHexDeprecated(const std::string_view str)
+void base_blob<BITS>::SetHexDeprecated(std::string_view str)
 {
     std::fill(m_data.begin(), m_data.end(), 0);
 
diff --git a/src/util/strencodings.h b/src/util/strencodings.h
index fd713a555c..1875810ab8 100644
--- a/src/util/strencodings.h
+++ b/src/util/strencodings.h
@@ -369,7 +369,7 @@ std::optional<uint64_t> ParseByteUnits(std::string_view str, ByteUnit default_mu
 
 namespace util {
 /** consteval version of HexDigit() without the lookup table. */
-consteval uint8_t ConstevalHexDigit(const char c)
+consteval uint8_t ConstevalHexDigit(char c)
 {
     if (c >= '0' && c <= '9') return c - '0';
     if (c >= 'a' && c <= 'f') return c - 'a' + 0xa;
diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h
index e029f39430..c5e4526412 100644
--- a/src/wallet/coinselection.h
+++ b/src/wallet/coinselection.h
@@ -93,7 +93,7 @@ public:
         }
     }
 
-    COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, const CAmount fees)
+    COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, CAmount fees)
         : COutput(outpoint, txout, depth, input_bytes, spendable, solvable, safe, time, from_me)
     {
         // if input_bytes is unknown, then fees should be 0, if input_bytes is known, then the fees should be a positive integer or 0 (input_bytes known and fees = 0 only happens in the tests)
@@ -355,7 +355,7 @@ private:
     }
 
 public:
-    explicit SelectionResult(const CAmount target, SelectionAlgorithm algo)
+    explicit SelectionResult(CAmount target, SelectionAlgorithm algo)
         : m_target(target), m_algo(algo) {}
 
     SelectionResult() = delete;
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 91dfd35479..322770e197 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -179,14 +179,14 @@ protected:
 public:
     explicit ScriptPubKeyMan(WalletStorage& storage) : m_storage(storage) {}
     virtual ~ScriptPubKeyMan() = default;
-    virtual util::Result<CTxDestination> GetNewDestination(const OutputType type) { return util::Error{Untranslated("Not supported")}; }
+    virtual util::Result<CTxDestination> GetNewDestination(OutputType type) { return util::Error{Untranslated("Not supported")}; }
     virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; }
 
     //! Check that the given decryption key is valid for this ScriptPubKeyMan, i.e. it decrypts all of the keys handled by it.
     virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key) { return false; }
     virtual bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { return false; }
 
-    virtual util::Result<CTxDestination> GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return util::Error{Untranslated("Not supported")}; }
+    virtual util::Result<CTxDestination> GetReservedDestination(OutputType type, bool internal, int64_t& index, CKeyPool& keypool) { return util::Error{Untranslated("Not supported")}; }
     virtual void KeepDestination(int64_t index, const OutputType& type) {}
     virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {}
 
diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h
index 3acaba0202..e584cd10e5 100644
--- a/src/wallet/test/util.h
+++ b/src/wallet/test/util.h
@@ -128,7 +128,7 @@ public:
 std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(MockableData records = {});
 MockableDatabase& GetMockableDatabase(CWallet& wallet);
 
-ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success);
+ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, bool success);
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_TEST_UTIL_H
diff --git a/src/zmq/zmqabstractnotifier.h b/src/zmq/zmqabstractnotifier.h
index 17fa7bbaa9..1cf54b8a78 100644
--- a/src/zmq/zmqabstractnotifier.h
+++ b/src/zmq/zmqabstractnotifier.h
@@ -35,7 +35,7 @@ public:
     std::string GetAddress() const { return address; }
     void SetAddress(const std::string &a) { address = a; }
     int GetOutboundMessageHighWaterMark() const { return outbound_message_high_water_mark; }
-    void SetOutboundMessageHighWaterMark(const int sndhwm) {
+    void SetOutboundMessageHighWaterMark(int sndhwm) {
         if (sndhwm >= 0) {
             outbound_message_high_water_mark = sndhwm;
         }

l0rinc · 2025-03-28T14:15:45Z

src/deploymentinfo.h

Was the const std::string_view kept in the .cpp file on purpose?
I've noticed we usually avoid consting a view...

I see we even have const std::string_view& - would it make sense to unify those usages here as well?

The goal of this pull is to avoid const params in declarations, not to avoid const params in definitions.

I can understand the confusion, so I am happy to do any of the following:

Close this pull

Just take the first commit and drop the second one

Add a commit to remove const in definitions as well, though, this at this point we may enter the territory where it isn't worth it to spend review time on this refactor

I can understand the confusion

I saw both were changed here, I think it's a good change, I like consistency and though a few other similar ones could be added here.

Not sure why you'd want to close the PR because of my comments, if they're not useful, you can explain it to me or just wait for other reviewers.

Feel free to resolve my comments.

I think it's a good change, I like consistency and though a few other similar ones could be added here.

I don't think it is possible to achieve "consistency" here. There is no automated check to catch those in definitions. The goals here are:

Performance improvements (by the first commit). However, there are no benchmarks, so this is questionable.

Readability and style related improvements. However, they are not enforced consistently, so this is questionable as well.

So I'll drop the second commit for now.

re: #31650 (comment)

Was the const std::string_view kept in the .cpp file on purpose?

IMO top-level consts in method declarations are always harmful because they are confusing, misleading, unchecked by compilers, and noisy.

But top-level consts in method definitions can be good or harmful or neither or both depending on the context. They can be good if they prevent bugs from mutating parameters, or make it clearer that variables won't change. They can be bad if they prevent move optimizations, or make code more complicated by requiring declarations of similar extra variables.

This point of this commit fa8e57951abc5c737ab68694b8f6aa77551faa6b is to enable the readability-avoid-const-params-in-decls check, so it makes sense to me if it only updates declarations and leaves most definitions alone.

Yes, agree in general - this example seemed different, that's why I asked.
I hope we can reenable the clang-tidy in a next PR, I also think it was a useful change.

Ok, restored initial version of this pull request. Should be easy to re-review.

src/wallet/wallet.cpp

src/external_signer.cpp

maflcko · 2026-01-08T21:05:29Z

Rebased. Looks like there have only been a few conflicts and it should be not too hard to re-review with a range-diff.

l0rinc · 2026-01-13T11:32:44Z

Rebased the changed locally for my last ack, soft reset it against the PR.

Besides a conflict with https://github.com/bitcoin/bitcoin/blame/3c8d389a84d29c7e5532548320228f3e8107969d/src/test/headers_sync_chainwork_tests.cpp#L101-L103 (where the consts were already removed) a few new consts in headers were also removed since:

Details

diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index 0ee41adcc5..53a760cd2f 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -760,7 +760,7 @@ BITCOINKERNEL_API void btck_logging_disable();
  *
  * @param[in] options Sets formatting options of the log messages.
  */
-BITCOINKERNEL_API void btck_logging_set_options(const btck_LoggingOptions options);
+BITCOINKERNEL_API void btck_logging_set_options(btck_LoggingOptions options);
 
 /**
  * @brief Set the log level of the global internal logger. This does not
@@ -835,7 +835,7 @@ BITCOINKERNEL_API void btck_logging_connection_destroy(btck_LoggingConnection* l
  * @return               An allocated chain parameters opaque struct.
  */
 BITCOINKERNEL_API btck_ChainParameters* BITCOINKERNEL_WARN_UNUSED_RESULT btck_chain_parameters_create(
-    const btck_ChainType chain_type);
+    btck_ChainType chain_type);
 
 /**
  * Copy the chain parameters.
diff --git a/src/rpc/rawtransaction_util.h b/src/rpc/rawtransaction_util.h
index 65e65c3789..7bd051220a 100644
--- a/src/rpc/rawtransaction_util.h
+++ b/src/rpc/rawtransaction_util.h
@@ -54,7 +54,7 @@ std::vector<std::pair<CTxDestination, CAmount>> ParseOutputs(const UniValue& out
 void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in);
 
 /** Create a transaction from univalue parameters */
-CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional<bool> rbf, const uint32_t version);
+CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional<bool> rbf, uint32_t version);
 
 /** Explain the UniValue "decoded" transaction object, may include extra fields if processed by wallet **/
 std::vector<RPCResult> DecodeTxDoc(const std::string& txid_field_doc, bool wallet);

ACK fa7a981

src/psbt.cpp

src/external_signer.cpp

The previous `assert` used `>=`, allowing `input_index == psbt.inputs.size()` and out-of-bounds access in `psbt.inputs[input_index]`. Found during review: bitcoin#31650 (comment)

sedited

ACK fa7a981

2f5b1c5 psbt: Fix `PSBTInputSignedAndVerified` bounds `assert` (Lőrinc) Pull request description: This PR fixes an off-by-one in a debug assertion in `PSBTInputSignedAndVerified`. The function indexes `psbt.inputs[input_index]`, so the assertion must not allow indexing at `psbt.inputs.size()`. Found during review: #31650 (comment) ACKs for top commit: optout21: utACK 2f5b1c5 maflcko: lgtm ACK 2f5b1c5 achow101: ACK 2f5b1c5 Tree-SHA512: cec613a9a38358d5caa243197d746baa129aebfd7fe697689f28e652f94c4683873c4676d5eb2eb909ea19de5e5f6e54ecc5f3162384a48f6f38a59273667689

The previous `assert` used `>=`, allowing `input_index == psbt.inputs.size()` and out-of-bounds access in `psbt.inputs[input_index]`. Found during review: bitcoin#31650 (comment) Github-Pull: bitcoin#34272 Rebased-From: 2f5b1c5

maflcko · 2026-01-14T22:05:54Z

rebased to drop hunks which have already been merged. Should be trivial to re-ACK

hebasto

ACK fa64d84, I have reviewed the code and it looks OK.

l0rinc · 2026-01-16T15:55:39Z

diff reACK fa64d84

Redid the rebase locally, soft reset against PR is empty.

DrahtBot changed the title ~~refactor: Enforce readability-avoid-const-params-in-decls~~ refactor: Enforce readability-avoid-const-params-in-decls Jan 13, 2025

DrahtBot added the Refactoring label Jan 13, 2025

DrahtBot mentioned this pull request Jan 13, 2025

Decouple WalletModel from RPCExecutor bitcoin-core/gui#841

Merged

maflcko force-pushed the 2501-less-wrong-const branch from fa6954f to fa31c25 Compare January 14, 2025 09:40

This was referenced Jan 14, 2025

refactor: Use std::span over Span #31519

Merged

kernel: Move kernel-related cache constants to kernel cache #31483

Merged

DrahtBot added the Needs rebase label Jan 16, 2025

This was referenced Jan 17, 2025

wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict #29680

Closed

Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling #28031

Closed

maflcko force-pushed the 2501-less-wrong-const branch from fa31c25 to fa89b4b Compare January 20, 2025 08:55

DrahtBot removed the Needs rebase label Jan 20, 2025

This was referenced Jan 21, 2025

Remove the legacy wallet and BDB dependency #28710

Merged

Cluster mempool implementation #28676

Closed

fanquake added this to the 30.0 milestone Feb 20, 2025

maflcko force-pushed the 2501-less-wrong-const branch from fa89b4b to fa02e12 Compare February 20, 2025 21:10

This was referenced Mar 10, 2025

[IBD] specialize block serialization #31868

Draft

refactor: Use util::Result class for wallet loading #25722

Draft

DrahtBot added the Needs rebase label Mar 20, 2025

maflcko force-pushed the 2501-less-wrong-const branch from fa02e12 to fa8e579 Compare March 20, 2025 13:34

DrahtBot removed the Needs rebase label Mar 20, 2025

ryanofsky approved these changes Mar 27, 2025

View reviewed changes

DrahtBot requested review from fanquake and hebasto March 27, 2025 19:01

l0rinc reviewed Mar 28, 2025

View reviewed changes

maflcko removed this from the 30.0 milestone Mar 31, 2025

maflcko changed the title ~~refactor: Enforce readability-avoid-const-params-in-decls~~ refactor: Avoid copies by using const references or by move-construction Mar 31, 2025

DrahtBot changed the title ~~refactor: Avoid copies by using const references or by move-construction~~ refactor: Avoid copies by using const references or by move-construction Mar 31, 2025

DrahtBot removed Needs rebase CI failed labels Jan 8, 2026

l0rinc approved these changes Jan 13, 2026

View reviewed changes

src/psbt.cpp Outdated Show resolved Hide resolved

src/external_signer.cpp Show resolved Hide resolved

DrahtBot requested review from pablomartin4btc, rkrux and ryanofsky January 13, 2026 11:33

l0rinc mentioned this pull request Jan 13, 2026

psbt: Fix PSBTInputSignedAndVerified bounds assert #34272

Merged

sedited approved these changes Jan 13, 2026

View reviewed changes

DrahtBot added the Needs rebase label Jan 14, 2026

MarcoFalke added 2 commits January 14, 2026 23:03

refactor: Avoid copies by using const references or by move-construction

faf0c2d

refactor: Enforce readability-avoid-const-params-in-decls

fa64d84

maflcko force-pushed the 2501-less-wrong-const branch from fa7a981 to fa64d84 Compare January 14, 2026 22:04

DrahtBot removed the Needs rebase label Jan 14, 2026

DrahtBot mentioned this pull request Jan 15, 2026

refactor: [move-only] Merge core_io module, remove from libkernel #34296

Open

hebasto approved these changes Jan 16, 2026

View reviewed changes

DrahtBot requested review from l0rinc and sedited January 16, 2026 15:31

refactor: Avoid copies by using const references or by move-construction #31650

Are you sure you want to change the base?

refactor: Avoid copies by using const references or by move-construction #31650

Uh oh!

Conversation

maflcko commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

hebasto commented Jan 14, 2025

Uh oh!

fanquake commented Feb 20, 2025

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jan 8, 2026

Uh oh!

l0rinc commented Jan 13, 2026

Uh oh!

Uh oh!

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 14, 2026

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc commented Jan 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

maflcko commented Jan 13, 2025 •

edited

Loading

DrahtBot commented Jan 13, 2025 •

edited

Loading

l0rinc Mar 30, 2025 •

edited

Loading

ryanofsky Mar 31, 2025 •

edited

Loading