💬 laanwj commented on pull request "depends: remove `NO_HARDEN` option":
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2720530698)
> Would it make sense to also drop ENABLE_HARDENING? Currently set OFF in the clang-tidy ci job, but I [flipped it ON](https://github.com/davidgumberg/bitcoin/commit/ce79bea14ae1427395b7eddb0a0e1c21fa78516d) and ran the clang-tidy job locally and it seems to work. (But I have no context for why hardening was disabled in clang-tidy cc: @maflcko)

Right, if it's needed for some of our own tooling/testing that might be a valid reason to keep it, but otherwise, nah.

And even then cmake provides
...

💬 l0rinc commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2720533816)
> with a simple "lower is better" comparison

I like the idea, updated the description and the code:

> Plotting the difference between the various proposed commits and the baseline (time_baseline[height] / time_commit_X[height], higher is better) might also be helpful?

Something like this? Conceptually seems useful, but here I don't know how to interpret it, seems too far zoomed in
<img width="10
...

📝 hebasto opened a pull request: "cmake, guix: Skip building tests in subtrees"
(https://github.com/bitcoin/bitcoin/pull/32054)
The `BUILD_TESTS` variable has a broad scope, controlling:
- Building `test_bitcoin`
- Building `test_bitcoin-qt`
- Building tests in subtrees, such as `secp256k1` and `univalue`
- Creating CTest's tests

However, for release builds, only the first is necessary.

To address this, this PR introduces the new `BUILD_TEST_BINARY` variable, which allows building only the `test_bitcoin` binary without enabling other tests.

💬 fanquake commented on pull request "cmake, guix: Skip building tests in subtrees for releases":
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720590640)
Concept NACK on introducing build options to control building single binaries.

💬 hebasto commented on pull request "cmake, guix: Skip building tests in subtrees for releases":
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720603006)
> Concept NACK on introducing build options to control building single binaries.

Every other `BUILD_*` variable does exactly the same.

💬 hebasto commented on pull request "cmake, guix: Skip building tests in subtrees for releases":
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720618613)
Alternatively, an explicit list of targets could be specified here:https://github.com/bitcoin/bitcoin/blob/c20a5ce106be716a503bcf615a1900ba8c635430/contrib/guix/libexec/build.sh#L247-L248

💬 fanquake commented on pull request "cmake, guix: Skip building tests in subtrees for releases":
(https://github.com/bitcoin/bitcoin/pull/32054#issuecomment-2720619271)
Sure, but I think that isn't a great pattern, and this makes it worse. Having a `BUILD_TESTS` & `BUILD_TEST_BINARY` is awkward, and I think the motivation is poor; if you only want to build certain binaries in a Guix build, then use `--target`. Why does it need a new build option?

👍 laanwj approved a pull request: "depends: remove `NO_HARDEN` option"
(https://github.com/bitcoin/bitcoin/pull/32038#pullrequestreview-2681205220)
Code review ACK 5dfef6b9b379f51e69f2a358c05ae3c3e8a26e13

💬 laanwj commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2720673981)
Concept ACK, this has been a source of frustration and misunderstanding in bitcoin's code since the beginning. Hashes are byte blobs, not numbers, and unless there's a specific good reason to interpret them as a number (can't think of more cases than those you already mention) they shouldn't be.

💬 Sjors commented on issue "Add test coverage for getblocktemplate fees":
(https://github.com/bitcoin/bitcoin/issues/32053#issuecomment-2720689733)
I ended up writing one for #31897. Can be salvaged if that PR doesn't make it.

💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1993185874)
Pushed a test.

💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2720701557)
Rebased and switched the order of commits, so it's more clear this PR doesn't change behavior.

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1993202306)
I've updated code block. Could you please check @maflcko and @davidrobinsonau

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2720735149)
The current approach of having the Template Provider communicate via IPC, as described in #31098, indeed does not require Bitcoin Core to change its net code.

Making easy to copy-paste code could still be a motivation for the author, but it would not a good reason to include it.

Additionally, as @theuni pointed out in https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2702063374, it may be the right design / abstraction for a Template Provider either.

💬 janb84 commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720738307)
> @brunoerg @janb84 @Prabhat1308 I expect all of you were running into the issue on macOS? I probably can't fix it myself, but I'd investigate whether the last commit is working:
>
> * Is your result different with or without the last commit?
> * Does `ResetCoverageCounters` work?
> * What is the coverage result for `src/test/util/setup_common.cpp`, especially `g_rng_temp_path_init`? (You can find it in `fuzz_det_cov.show.{run_id}.txt`)

In checking this issue I discovered that due to the
...

💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720745456)
> I discovered that due to the change in [31161](https://github.com/bitcoin/bitcoin/pull/31161) the tooling is broken. (the path has changed from `src/test/` to `/bin` )

Should be trivial to fix up. I am happy to review a tiny pull to fix the silent merge conflict here.

👍 rkrux approved a pull request: "contrib: Fix `gen-bitcoin-conf.sh`"
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2681359199)
tACK a24419f8bed5e1145ce171dbbdad957750585471

I can see it adds the previously excluded options now (starting with `alertnotify`) in the example conf file.

💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2720804983)
> It seems that the first thing to decide is the source of the version information:
>
> * the `CLIENT_VERSION_*` variables set by the build system, or
>
> * a GitHub tag

A related discussion happens in https://github.com/bitcoin-core/bitcoin-maintainer-tools/issues/178.

💬 saikiran57 commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1993261238)
My primary goal is to give fix with minor changes to code, that's why I've updated this `throw std::runtime_error(std::string(__func__) + ": " + error);` to `throw std::invalid_argument(error);` so it does its satisfy the test case. During rpc call if we face issue I cached this exception(invalid_argument) return with existing response message to user if any other exception happened I just left with default implementation.

Regarding your suggestion using `util::Result<void>`. Inside AddWallet
...

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2720833753)
Rebased to resolve a silent merge conflict with merged bitcoin/bitcoin#31161.