💬 furszy commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1755789250)
q: if the `block` is only used to obtain the undo data, shouldn't this be a `!chainman.m_blockman.ReadBlockFromDisk(block, *blockindex))`? (negated)

📝 kevkevinpal opened a pull request: "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes"
(https://github.com/bitcoin/bitcoin/pull/30875)
A bit of a followup from https://github.com/bitcoin/bitcoin/pull/30840

There a few more places in the developer notes that we're mentioning `--enable-debug` which we should replace with `-DCMAKE_BUILD_TYPE=Debug` due to the autotools to cmake upgrade

💬 mzumsande commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1755950399)
We need both block and undo data below if we don't skip with the non-detailed response here. So there is a parenthesis after the first negation, so the logic is `not(a and b)` which is the same as `not(a) or not(b).`

💬 jaybny commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2345200662)
have you tested this? https://github.com/bitcoin-core/gui-qml

💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1756050527)
Would it be interesting to also special case:

```python
spk = bytes.fromhex(spkhex)
...
elif 2 <= len(spk) <= 76 and spk[0] + 1 == len(spk):
return True
```

ie, a single fixed push of 1-75 bytes is also treated as trivial, so you can make your challenge be a push of sha256(your-email-address) to get a unique PoW-only signet.

🤔 ajtowns reviewed a pull request: "signet: fixing mining for OP_TRUE challenge"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2299146330)
ACK ecee62d2ab63685455b428db4e832827b6ce36f1 with nits

💬 ajtowns commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r1756114962)
Should keep the `, *,` to ensure `blocktime` and `poolid` are keyword-only args.

💬 maflcko commented on pull request "doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes":
(https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2345289075)
It would be nice to fix all remaining instances in one go. See also https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2320892444 and https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2321610186

💬 kravens commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2345407161)
> have you tested this? https://github.com/bitcoin-core/gui-qml

Not yet, but I will try it!

💬 naumenkogs commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1756250067)
Could you expand the comment similar to what's above? "The time is set, but after 3 attempts...". Otherwise ACK :)

👍 naumenkogs approved a pull request: "refactor: move m_is_inbound out of CNodeState"
(https://github.com/bitcoin/bitcoin/pull/30233#pullrequestreview-2299394122)
ACK 129f73cc6e2ed4147f57c3d1dfe2841b8a46e9fe

🤔 bainpa reviewed a pull request: "26.2 final changes"
(https://github.com/bitcoin/bitcoin/pull/30376#pullrequestreview-2299511514)
MAZENTERPRISE

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756344649)
<details>
<summary>New <code>git diff -w</code></summary>

```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
index c6ee1ffed9..3fed361e7e 100644
--- a/src/test/util_string_tests.cpp
+++ b/src/test/util_string_tests.cpp
@@ -10,62 +10,140 @@ using namespace util;

BOOST_AUTO_TEST_SUITE(util_string_tests)

-BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
+// These are counted correctly, since tinyformat will require the provided number of args.
...

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1756353021)
I did modify `HaveCoin` in this commit - but since you think it's noise, I've reverted

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1756356272)
I don't think there's any difference (except for having to use `ret->second.coin.DynamicMemoryUsage` and `ret->second.coin.IsSpent()` instead), but I don't mind moving it up.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756387736)
I pushed a test-only change to reduce the verbosity of the test and also drop the accidental duplicate line.

The tests now look like `PassFmt<0>("");` for the passing case and `FailFmtWithError<0>("%s", check_num_spec);` for the failing case.

I hope this change is acceptable for reviewers to review. Otherwise, I'll revert back to the previous commit hash.

🚀 fanquake merged a pull request: "build: Fix `ENABLE_WALLET` option"
(https://github.com/bitcoin/bitcoin/pull/30867)

💬 Sjors commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2345614402)
Taken both suggestions.

I modified the test to actually check the signet commitment absence.

👍 l0rinc approved a pull request: "ci: Print inner env, Make ccache config more flexible"
(https://github.com/bitcoin/bitcoin/pull/30869#pullrequestreview-2298725366)
utACK facb8f16389d1fbf27ee70f887de9cb082028374

💬 l0rinc commented on pull request "ci: Print inner env, Make ccache config more flexible":
(https://github.com/bitcoin/bitcoin/pull/30869#discussion_r1756384586)
the dot is dropped since it's not a [home dir](https://ccache.dev/manual/latest.html#config_cache_dir), right?