💬 fanquake commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2823725523)
> I wonder if this is relevant, given how long the functional tests take.

My assumption would be that developers are running the units tests regularly, far more often than they are running the entire functional test suite.

💬 Sjors commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2823736638)
Tested 86c7c65e2fe7a33f47e08d7c344dbee44ea1e379 on Windows 11 against HWI 3.1.0 on testnet4.

First time it crashes immediately after creating a new wallet from a Trezor model T. The wallet ended up blank with no descriptors. The debug log shows nothing useful, except that it didn't import descriptors. cc @achow101

```
2025-04-23T09:50:08Z Using SQLite Version 3.46.1
2025-04-23T09:50:08Z Using wallet C:\Users\sjors\AppData\Roaming\Bitcoin\testnet4\wallets\TrezorT
2025-04-23T09:50:08Z in
...

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055688076)
My intent is to keep as much of the code separate from error handling as possible. By narrowing the scope of the try it makes clear to the reader that there aren't exceptions hiding in every line of the happy path.

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2055693705)
Fixed. @TheCharlatan why was this set to `Yes` in the original?

💬 maflcko commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2055699796)
depedency → dependency

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2823758637)
An alternative approach could be to make Cap'n Proto optional and have `-DENABLE_IPC` default to `ON` only if its present. Though I vaguely recall that with CMake we wanted to move away from enabling config options depending on whether a library is found.

💬 l0rinc commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2823759748)
ACK fa19d8bee350fef088f5fabca38f09610fa8d20d

The rebase had to fix a `Proxy` merge conflict but otherwise is a clean cherry-pick of the previous `readability-avoid-const-params-in-decls` commit.

Q: Any reason for not including these `const` removals in the last commit?
* `interfaces::BlockTemplate::waitNext`
* `node::GetMinimumTime`
* `blockToJSON`
* ` blockheaderToJSON`
* `GetTarget`
* `wallet::CreateDescriptor`
* `wallet::CWallet::ListAddrBookFunc`
* `DeriveTarget
...

💬 maflcko commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2055703942)
Specficially → Specifically

Also, `bitcoin*d*-wallet` seems odd.

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055708651)
I think the opposite is true, this way we're forcing the reader to consider the error case when trying to understand the happy path (i.e. separation of concerns). But I'll leave it, it's not *that* important.

💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055699711)
In c475135c07e588f656090664c7e48c8a73f2305c: Is this going to be upstreamed?

💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055701332)
In ae3ddd322ca61f6bc7c4f5d6c372057069e6c5ea: is this going to be upstreamed / aligned with the upstream code (https://github.com/arun11299/cpp-subprocess/pull/109) ?

💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055708434)
In ce9d47987be2b6a72fc87cc42a1c31a6f1766d52: This is adding suppressions for warnings but doesn't mention what they are, or explain why we aren't fixing the code?

💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055703963)
In ce950ec20fa0d246d2fd44f562a864d3ff83d399 "Fix quote issue on Windows ": This isn't in a Windows block, so I'm guessing it doesn't have any effect on other OS's? Is this alos going to be upstreamed?

💬 0xB10C commented on issue "ci: failure in interface_usdt_coinselection.py":
(https://github.com/bitcoin/bitcoin/issues/32322#issuecomment-2823781979)
Seems like a one-off event for now and not much that can be done about it?

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055750894)
I must be missing something fundamental here, I don't understand why these lines are never triggered while running the test.
Are these triggered in some special circumstance that has happened before but isn't happening anymore - and since it's already testing other tests, there's no point in covering it with actual tests?

I find it a bit confusing, wanted to understand why e.g. the `nocleanup` branch was added here - but disabling it still passes, so that doesn't help...

```python
if se
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055753450)
I think 1. and 2. are correct, but in 3. it would happen differently - it would remove the transaction because both txid and wtxid match. `stale_tx` came from the private broadcast storage, is the original transaction, not the malleated one.

💬 l0rinc commented on pull request "ci: switch to LLVM 20 in tidy job":
(https://github.com/bitcoin/bitcoin/pull/32226#issuecomment-2823865942)
ACK 08aa7fe2326391e6d633c2da50959754e3e7b8d6

Since last review `modernize-type-traits` lint rule was removed, and `RemovePrefixView` was also updated to use `starts_with`.

<details>
<summary>Diff since last review</summary>

```patch
diff --git a/src/.clang-tidy b/src/.clang-tidy
index aa171dc0ac..1cf270833a 100644
--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -9,7 +9,6 @@ bugprone-lambda-function-name,
bugprone-unhandled-self-assignment,
misc-unused-using-decls,
misc-no-rec
...

💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055779247)
https://en.cppreference.com/w/cpp/algorithm/clamp states that
> If lo is greater than hi, the behavior is undefined.

(cc: @hodlinator)
Should we add an explicit `assert` somewhere that this isn't the case? Though it's implied currently from the tests...

💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055801774)
Will do if I repush.

💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055801848)
you mean we usually ignore the folder when sorting? I found examples for all sorts of includes (pun, intended) - I'll adjust if I have to push again.