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

💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055802001)
> Capped at 256 MiB, as gains are barely measurable for bigger batches (see PR 31645)

I don't mind adding, but a simple blame would immediately reveal that

> unneeded braces line 18

They may be implied, but I want to emphasize that mathematically this isn't associative, i.e. not the same as
```C++
dbcache_bytes / (DEFAULT_KERNEL_CACHE * DEFAULT_DB_CACHE_BATCH)
```

📝 juanmigdr opened a pull request: "Add rpcestimateconservativefees"
(https://github.com/bitcoin/bitcoin/pull/32329)
### Summary

This PR introduces a new configuration option: `-rpcestimateconservativefees`. When enabled, it forces the default behavior of the `estimatesmartfee` RPC to use **conservative mode** fee estimation.

### Motivation

Bitcoin Core v28.0 changed the default fee estimation mode for `estimatesmartfee` from **conservative** to **economical** ([[#30275](https://github.com/bitcoin/bitcoin/pull/30275)](https://github.com/bitcoin/bitcoin/pull/30275)). While this change reduces fee over-
...