💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755612640)
Thanks, I'll fix it up in the next follow-up, or if I have to re-push here, to avoid invalidating review over a harmless test style-nit.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755647995)
> While I agree with maflcko that [keeping functionality hard to use outside of the intended context](https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752494409) has value, keeping tests straightforward is more important. If you prefer...

Thank you for the diff. However, I think that real code should be designed without taking tests into account (apart from being testable). Tests should always follow real code, not the other way round. I am sure the verbosity in the test can be redu
...

👍 TheCharlatan approved a pull request: "build: Add more cmake presets"
(https://github.com/bitcoin/bitcoin/pull/30871#pullrequestreview-2298717809)
ACK f15e817811e328423ea489870ead089128a6ef8c

👍 hodlinator approved a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2298723649)
re-ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7

`git range-diff master e6a5ab7 5e190cd`

- [Disagree](https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755611792) with `AppendData`/`AppendDataSize` names over `PushData`/`PushDataSize`, but will let it go.
- Nice switch back from pointer arithmetic to `span` in `AppendData`.
- Touched up commit messages per my latest review, cheers!
- Implements legacy `operator<<` in terms of the other. I agree with l0rinc's reservations against
...

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755702207)
> I think that real code should be designed without taking tests into account (apart from being testable). Tests should always follow real code, not the other way round.

Making the function return a value allowed for C) dropped negatives (and less runtime testing). But I can try to provide a new diff bringing them back and without changing *string.h*. My main point remains adding the interlaced tinyformat tests to prove parity.

> The diff you shared also changes `util::hex_literals::detail
...

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