💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055986974)
We could definitely test more parts of the framework functionality in another PR. This PR is focused on keeping the error reporting clear for startup failures, to aid troubleshooting.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2824202867)
`2b34857ad5...1c16944a4a`: address suggestions

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055989720)
Done

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055990031)
Done

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055990265)
Done

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055990539)
Done

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055992009)
Done, reworded the comment.

💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055994881)
But this PR introduced the dead code

💬 saikiran57 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2824224700)
Hi @furszy could you please finalize this review and merge this.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2056007024)
https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055969055

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-2824233359)
Hi All @achow101 @furszy @mprenditore @maflcko @w0xlt @davidrobinsonau request to re-review this PR and provide ACK.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2056022026)
Yes, this loose handling makes it possible to do the job with less code. My reasoning is that the worst outcome from the race is acceptable.

I am looking into keeping track of more stuff with respect to the future RPC stats (see at the bottom of https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2042286270) which would also give us more fine grained control here. If it looks reasonable and is not too much code, I will propose the change in https://github.com/bitcoin/bitcoin/pull/29415#
...

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2056025288)
Good catch, thanks. I should have re-checked with my follow-up PR which introduces validation before pushing the change for the fuzz target. Will do that this time.

💬 fanquake commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2824264910)
Re-ran tidy as it looks like 20 will pickup more issues: https://cirrus-ci.com/task/5041587878625280?logs=ci#L1916:
```bash
[09:08:35.465] /ci_container_base/src/rpc/util.h:527:50: error: parameter 'pow_limit' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls,-warnings-as-errors]
[09:08:35.465] 527 | uint256 GetTarget(const CBlockIndex& blockindex, const uint256 pow_limit);
`
...

💬 hodlinator commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2056032514)
> you mean we usually ignore the folder when sorting?

Think it's a case of keeping our headers separate from external dependencies/libraries.

💬 hodlinator commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2056049582)
I think it might make sense to introduce our own version of `clamp` that includes such an `assert` since the standard library has no guarantees if cppreference is to be believed. (Or just start off with an `assert`/`Assume` before the call for now).

📝 nervana21 opened a pull request: "doc: add missing top-level description to pruneblockchain RPC"
(https://github.com/bitcoin/bitcoin/pull/32330)
Previously, the `pruneblockchain` RPC help output included only the method signature and arguments, with no top-level description explaining its purpose or constraints.

This adds a concise description noting that the method deletes block and undo data, requires `-prune` to be enabled at startup, and is irreversible.

✅ nervana21 closed a pull request: "doc: add missing top-level description to pruneblockchain RPC"
(https://github.com/bitcoin/bitcoin/pull/32330)

⚠️ fanquake opened an issue: "cmake: GUI build broken with `-stdlib=libc++`"
(https://github.com/bitcoin/bitcoin/issues/32331)
```bash
master @ 9a4c92eb9ac29204df3d826f5ae526ab16b8ad65
clang version 20.1.2 (Fedora 20.1.2-3.fc42)
cmake -B build -DBUILD_GUI=ON -DAPPEND_CXXFLAGS="-stdlib=libc++" -DAPPEND_LDFLAGS="-stdlib=libc++"
cmake --build build
<snip>
cmake --build build -j17 --target bitcoin-qt
[ 0%] Generating bitcoin-build-info.h
[ 0%] Built target secp256k1_precomputed
[ 0%] Built target crc32c
[ 1%] Built target bitcoin_consensus
[ 3%] Built target bitcoin_cli
[ 4%] Built target univalue
[ 7%] Built target
...

💬 glozow commented on pull request "Add rpcestimateconservativefees":
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824360562)
Concept NACK - even if there are good reasons to use conservative fee estimation, using a config option instead of the RPC parameter is more rigid, requires coordination with the node runner, wouldn't be available for another 6 months, and further complicates the current mess of config options. Passing a 'conservative' arg should be a one-line change to their source code and is already supported by all maintained versions of Core.

> While I do not have specific details on their reasons, the k
...