💬 hodlinator commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1763327444)
nit: Line grows from 209 to 253 chars, please consider inserting line break(s).
```suggestion
inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line,
const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<sizeof...(Params)> fmt, const Params&... params)
```

💬 hodlinator commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1763307360)
nit: The `Params` name was already used in *scriptpubkeyman.h* and *wallet.h* before the PR but `Args` is used in *util/string.h*, *index/base.h/cpp*, *netbase.cpp* and used to be used here. Why switch and touch 2 additional lines?

The old way feels more consistent with the C "varargs". But maybe there's a convincing argument for "Params".

Did the change locally and it compiles without causing any non-obvious issue.

👋 hebasto's pull request is ready for review: "ci: Use `ninja` to build in macOS native CI job"
(https://github.com/bitcoin/bitcoin/pull/30915)

💬 hebasto commented on pull request "ci: Use `ninja` to build in macOS native CI job":
(https://github.com/bitcoin/bitcoin/pull/30915#issuecomment-2356083087)
cc @maflcko

💬 hebasto commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2356089545)
@theStack

Could rebase once more please to refresh the CI?

💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2356103199)
> Is this so that re-runs (of the same PR) always hit the cache? If that's it I'm not _totally_ sure it's worth it. My intuition says that if we had multiple servers, even if a re-run took place on a different machine, we'd likely get a _pretty decent_ cache hit from another run or a previous master build

Right. It is unclear whether this is worth it. I think when it comes to builds on master, the speed doesn't matter at all, because no one is waiting for the result to appear immediately. So
...

💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2356181083)
> CPU-bound (and not IO-bound)

Actually, the long-running fuzz test may be IO-bound so using a ramdisk could speed them up. So this would be another idea to look at.

💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2356184027)
> @theStack
>
> Could rebase once more please to refresh the CI?

Yes, done.

👍 hebasto approved a pull request: "[28.x] Further backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/30827#pullrequestreview-2310065055)
ACK 06a7df70df30879e0b691d1a252636f703b8cdfb, I've backported the listed PRS locally. The only merge conflict I faced was in https://github.com/bitcoin/bitcoin/pull/30807. It was trivial to resolve.

💬 maflcko commented on pull request "build: speed up by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2356197180)
> It's clear to see that the header generation is self-contained, and afaik that's the only place that would matter for build performance.

Could rebase on current master, now that the header generation is 30x faster, after 2a0949f0977db2dbb7ac452e8d58d413b9526756 ? This should also change the benchmark in the pull description.

💬 maflcko commented on pull request "ci: Use `ninja` to build in macOS native CI job":
(https://github.com/bitcoin/bitcoin/pull/30915#issuecomment-2356214347)
review ACK d01b85bfecbcf16ea16f90e2ade7537bf582269f

Can't hurt to test this in one task

👍 ryanofsky approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2310085862)
Code review ACK 7942951e3fcc58f7db0059546d03be9ea243f1db. Just rebased since last review

👍 TheCharlatan approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2310089065)
Re-ACK 7942951e3fcc58f7db0059546d03be9ea243f1db

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#issuecomment-2356233258)
Rebased after merge of #30286.

💬 0xBEEFCAF3 commented on pull request "CAT in Tapscript (BIP-347)":
(https://github.com/bitcoin/bitcoin/pull/29247#discussion_r1763450844)
Great suggestion! Added some additional unit tests to script_tests.json testing different combinations of scripts with and without (SCRIPT_VERIFY_DISCOURAGE_OP_CAT, SCRIPT_VERIFY_OP_CAT and SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS)

💬 jaybny commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2356240428)
ok. is C++20 a requirement? why? it's not fully supported by clang or gcc yet. can get 90% there with libstdc++ , but I guess it's only a matter of time.

💬 hebasto commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2356249815)
> is C++20 a requirement?

Yes. Please refer to https://github.com/bitcoin/bitcoin/pull/28349.

💬 mzumsande commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763467219)
I thought about it a bit and don't see a great non-invasive solution to this: We could add a call to `NotifyHeaderTip` in the calling functions after `cs_main` is released, which would be in `ActivateBestChain()` (plus in the `invalidateblock` / `reconsiderblock` rpcs which seem less important), and I'm unsure if we want to refactor this function to always call `NotifyBestHeader()` just for the special case where a connected block turns out to be invalid.
In any case, since `m_best_header` is
...

💬 mzumsande commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763468418)
will change to "/" so it mirrors the "invalidation / reconsideration" in the line before.

💬 mzumsande commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763474241)
change to `::cs_main`