💬 fanquake commented on issue "Release Schedule for 29.0":
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2720068615)
`rc1` has been tagged: https://github.com/bitcoin/bitcoin/releases/tag/v29.0rc1.

💬 ajtowns commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2720097064)
> Plotting the performance of the blocks from the produced `debug.log` files shows (from the last run, can differ slightly from the normalized average shown below) the effect of each commit:

Wouldn't these plots be easier to read with block height on the x-axis and time on the y-axis, giving a consistent domain (each case goes from height 0 to 880k or so) with a simple "lower is better" comparison? (rather than "further left is better")

Plotting the difference between the various proposed
...

💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720322464)
@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`)

⚠️ Sjors opened an issue: "Add test coverage for getblocktemplate fees"
(https://github.com/bitcoin/bitcoin/issues/32053)
Suggested in https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1992245751

There's probably other aspects of `getblocktemplate` and the underlying template construction methods that coverage.

Related:
- #31981 adds coverage for the `proposal` mode

💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1992988592)
Tracking in #32053.

💬 maflcko commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1992994451)
It has been permitted previously and this commit is silently breaking it. If you want to use -1, it seems better to properly sanitize the user input first, or use a different approach.

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2720368885)
Rebased after #31283.

💬 maflcko commented on pull request "build: align debugging flags to `-O0`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2720373606)
no opinion about the build changes here, but about the CI changes I slightly tend toward NACK, as per my previous comment https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2685140630

💬 fanquake commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2720395222)
> it should be the intersection of what's needed for ... Sv2
> Maybe also worth considering is to have a common library for Sv2

As far as I'm aware, nothing SV2 related is going into Core (aside from the mining interface), and we aren't planning on refactoring P2P code so it could be copy-pasted out/reused in some other SV2 utility, so, SV2 shouldn't be a motivating usecase here? It looks like #30694 could also be updated to remove any reference to SV2 as motivation (i.e "Doing Stratum V2")
...

💬 maflcko commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1993037907)
Why change to `Debug` when there is no reason? The docs literally say in the first line:

> This document explains how to use clang’s source-based code coverage feature. It’s called “source-based” because it operates on AST and preprocessor information directly. This allows it to generate very precise coverage data.

https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#introduction

If it doesn't work, at a minimum, you should provide exact steps to reproduce, not add workarounds for b
...

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