🚀 fanquake merged a pull request: "depends: add -g to DEBUG=1 flags"
(https://github.com/bitcoin/bitcoin/pull/29527)

💬 fanquake commented on pull request "depends: add -g to DEBUG=1 flags":
(https://github.com/bitcoin/bitcoin/pull/29527#issuecomment-2034083503)
Going to followup with an optimization flags alingment RFC.

💬 glozow commented on pull request "doc: update release-process.md":
(https://github.com/bitcoin/bitcoin/pull/29645#issuecomment-2034094652)
(Yay v26.1) I have a couple more things for the last section, and then will take out of draft.

💬 fanquake commented on pull request "depends: add new LLVM debug macro":
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2034145974)
> IIRC usage was turned into a compile error (maybe only intermittently, will check again).

I think I was thinking of something else here. Have changed this to just adding the new macro.

👋 fanquake's pull request is ready for review: "depends: add new LLVM debug macro"
(https://github.com/bitcoin/bitcoin/pull/29781)

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-2034186584)
> Dealing with a read-only tmp file is probably outside the scope of this PR, but maybe we should reconsider if +rw might actually be the correct mode to use.

@luke-jr that seems like an unlikely situation (unless bitcoind is killed in the moment between writing the temporary cookie file and renaming it?), nonetheless, it could be addressed by changing where we apply the new more restrictive `400` permissions, from the temp file to the renamed `.cookie`.

I don't think the temporary cookie
...

💬 tdb3 commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2034218395)
Rebased and pushed. `skipunit` now removed.

💬 vostrnad commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034241283)
Tested self-built MSVC binaries for 26.0 and 27.0rc1, definitely no regression.

26.0:

27.0rc1:

Note that I made corrections to my previous graphs because of a bug that caused some runs to leak from one graph to the other. The regression is even more prominent now.

💬 maflcko commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034250821)
> Tested self-built MSVC binaries for 26.0 and 27.0rc1, definitely no regression.

I forgot to mention that optimization was enabled in commit 41e378a0a1f0729b423034f47c28a4e83287a1e8. So for a fair comparison, one would have to backport 41e378a0a1f0729b423034f47c28a4e83287a1e8 to 26.0.

I presume that benchmarking IBD is expensive. So maybe comparing the micro-benchmarks can provide a hint at which code is slower?

💬 fanquake commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034264315)
> Tested pre-built binaries on Debian 12 WSL on the same system, measured 45 runs of each. No performance regression,
> Tested self-built MSVC binaries for 26.0 and 27.0rc1, definitely no regression.

It's not clear to me if there is still an issue here or not. Or is there still a regression when comparing our 26.x and 27.x Windows release binaries?

If the only issue is in relation to self-compiled MSVC binaries, then this isn't a blocker for 27.x.

💬 hebasto commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034267106)
Speaking of MSVC builds, it's worth noting that they still don't have a hardware-accelerated SHA256 implementation (see https://github.com/bitcoin/bitcoin/pull/24773).

💬 naiyoma commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#issuecomment-2034278715)
CONCEPT ACK , Using `calculate_input_weight` is a better approach than manual estimations, also increases readability.

💬 maflcko commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#discussion_r1549504485)
Could also print the result on success? IIUC, stdout will be hidden by the test runner, and when running this test independently, having some feedback is better than no feedback?

💬 vostrnad commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034285249)
@maflcko

> I forgot to mention that optimization was enabled in commit https://github.com/bitcoin/bitcoin/commit/41e378a0a1f0729b423034f47c28a4e83287a1e8. So for a fair comparison, one would have to backport https://github.com/bitcoin/bitcoin/commit/41e378a0a1f0729b423034f47c28a4e83287a1e8 to 26.0.

The original regression is also in variance which doesn't appear with the MSVC binary, so I don't think it's necessary to test the backport.

@fanquake

> It's not clear to me if there is st
...

📝 fanquake opened a pull request: "[RFC] Switch and/or align debugging flags (back) to `-Og`"
(https://github.com/bitcoin/bitcoin/pull/29796)
We currently have two issues in relation to debugging optimisation flags:

* A depends build with `DEBUG=1` and using `./configure --enable-debug` do not align, which is inconsistent/confusing. Depends currently uses `-O1`, while `--enable-debug` will set `-O0`.
* `-O0` is unusable in various ways:
* Compiling certain assembly under `-O0` doesn't currently work: https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-2027843446 (probably an upstream issue?).
* Windows cross-compi
...

💬 dergoegge commented on pull request "refactor: Use typesafe Wtxid in compact block encodings":
(https://github.com/bitcoin/bitcoin/pull/29752#discussion_r1549507036)
> as I understand it, this was sort-of a bug in the previous implementation but didn't cause a crash, just 'potentially bad' behaviour?

It would actually be a crash. Just to recap the bug that exists on master:

* If the ring buffer is not fully filled, then it contains default initialized `uint256`s (as the first pair element)
* We loop over the **entire** ring buffer in `PartiallyDownloadedBlock::InitData`, [compute the short id](https://github.com/bitcoin/bitcoin/blob/0d509bab45d292caea
...

📝 hebasto opened a pull request: "guix: Remove another leftover from #29648"
(https://github.com/bitcoin/bitcoin/pull/29797)
It was overlooked in bitcoin/bitcoin#29787.

💬 hebasto commented on pull request "[RFC] Switch and/or align debugging flags (back) to `-Og`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2034313126)
> * A depends build with `DEBUG=1` and using `./configure --enable-debug` do not align, which is inconsistent/confusing.

Concept ACK on fixing that.

💬 maflcko commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034314366)
> I presume that benchmarking IBD is expensive. So maybe comparing the micro-benchmarks can provide a hint at which code is slower?

I forgot to mention that benchmarks are disabled in guix. So something like `sed -i -e 's/--disable-bench //g' $( git grep -l disable-bench ./contrib/guix/ )` may be needed before a guix build.

🤔 sr-gi requested changes to a pull request: "Simplify network-adjusted time warning logic"
(https://github.com/bitcoin/bitcoin/pull/29623#pullrequestreview-1973887290)
ACK https://github.com/bitcoin/bitcoin/commit/2ef71c73582be554e565ada3f8a6ca77c2cd79f1

Left some comments inline, mostly nits/things that may need renaming.

Also, `qt/guiutils.h` still refers to CNodeCombinedStats.nTimeOffset after 5084ad48462b3169c99a81aeb21a339c3a9a4728, in a couple of places, but it may be better to change that for `time_offset` given `nTimeOffset` is not a thing anymore.