Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jan 20, 2026

std::bind has many issues:

  • It is verbosely listing all placeholders, but in a meaningless way, because it doesn't name the args or their types.
  • It silently ignores args passed to it, when one arg is overridden. For example [1] compiles fine on current master.
  • Accidentally duplicated placeholders compile fine as well.
  • Usually the placeholders aren't even needed.
  • This makes it hard to review, understand, and maintain.

Fix all issues by using std::bind_front from C++20, which allows to drop the brittle _1, _2, ... placeholders. The replacement should be correct, if the trailing placeholders are ordered.

Introducing the same silent bug on top of this pull request [2] will now lead to a compile failure.


[1]

diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 694fb535b5..7661dd361e 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals()
     m_handler_status_changed = m_wallet->handleStatusChanged(std::bind(&NotifyKeyStoreStatusChanged, this));
-    m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5));
+    m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, CTxDestination{}, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5));
     m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind(NotifyTransactionChanged, this, std::placeholders::_1, std::placeholders::_2)); 

[2]

diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 578713c0ab..84cced741c 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals()
     m_handler_status_changed = m_wallet->handleStatusChanged(std::bind_front(&NotifyKeyStoreStatusChanged, this));
-    m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this));
+    m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this, CTxDestination{}));
     m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind_front(NotifyTransactionChanged, this));

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 20, 2026

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

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK hebasto, fjahr

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:

  • #34158 (torcontrol: Remove libevent usage by fjahr)
  • #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)

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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

  • std::bind_front(&microTask, std::ref(s), std::ref(mutex), std::ref(counter), -delta + 1, noTime) in src/test/scheduler_tests.cpp

2026-01-20

@hebasto
Copy link
Member

hebasto commented Jan 20, 2026

Concept ACK.

@fjahr
Copy link
Contributor

fjahr commented Jan 20, 2026

Concept ACK, just ran into bind issues in a PR recently and then replaced it with bind_front.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants