💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1755723676)
Might be clearer like this:
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "You may pass in the absolute path of your wallet directory (or .dat file for legacy wallets). Otherwise, the interpreted path will be relative to the default wallet directory."},
```

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755737721)
> > The diff you shared also changes `util::hex_literals::detail::Hex`, which I don't really understand.
>
> It's a consequence of clashes between `util::detail` namespaces. (Added note about that in a later edit).

Thanks for explaining. I think this is another reason to keep `string.h` as-is for now.

> My main point remains adding the interlaced tinyformat tests to prove parity.

I think the `CheckTooMany` may be a bit too verbose and mostly unit tests for tinyformat itself. Especial
...

💬 maflcko commented on pull request "Lint: improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2344790531)
Are you still working on this?

👍 hodlinator approved a pull request: "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage"
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2298831735)
ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa

Provides better error messages for tests.

nit: 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa - "Compare FromUserHex result against other hex validators and parsers" is not dependent on the other changes, so could be moved first instead of last if you re-touch.

💬 kravens commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2344798265)
Screen recording: [BitcoinCore-screen0.webm](https://github.com/user-attachments/assets/db653667-6b83-4089-b85e-69d84bcebede)
I'm testing 28.X release candidate branch, is this behaviour due to these lock-ups in the GUI code?

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755770730)
Same re: `HaveCoin`

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755770946)
Same re: `HaveCoin`

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755753080)
This line should be moved up to just under the if statement to keep behavior the same. The coin is already moved into `ret->second.coin` before we potentially call `AddFlags`.

Although this will hopefully go away in #30673.

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755770549)
I don't think we should be changing `HaveCoin` lines in a commit about `GetCoin`.

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755771231)
Same re: `HaveCoin`

💬 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 :)