💬 maflcko commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#issuecomment-2360050658)
> Is there a way to see complete force-push history?

GitHub sometimes(?) sends commit hashes by email, it was 8ffb79805d3589a86ca97e976b9d271cd8062ae8

💬 maflcko commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2360075066)
> Well, the approach seems to be similar with pruning:

Sure, if that is the case, the pull request description should mention it. Also, the error message could probably be updated, because it only mentions pruning, no AU. (Note to self: Reminds me of https://github.com/bitcoin/bitcoin/pull/26282)

💬 maflcko commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2360121854)
> ### Expected behaviour
>
> `getblocktemplate` should return a result consistent with the information announced via ZMQ and correctly reflect the current chain tip.

So to clarify my previous reply. What you describe as "expected behavior" is impossible to achieve. A `C` message just contains the block hash of a previous `BlockConnected` event. While often this will be equal to the result of an RPC `getbestblockhash` or `getblocktemplate` done after receiving the `C` message, there isn't (
...

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1766285389)
`FRESH` is definitely a [very confusing concept](https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L143-L151)
> the parent cache does not have this coin or that it is a spent coin in the parent cache

Maybe after https://github.com/bitcoin/bitcoin/pull/30673 we can find a better name (and description) for it

🤔 vasild reviewed a pull request: "Fix display issues for IPv6 proxy setup in Options Dialog (UI only, no functionality impact)"
(https://github.com/bitcoin-core/gui/pull/836#pullrequestreview-2314606389)
Approach ACK a7d8de94adc3e94ea33d698c307a87ab775c3d20

💬 vasild commented on pull request "Fix display issues for IPv6 proxy setup in Options Dialog (UI only, no functionality impact)":
(https://github.com/bitcoin-core/gui/pull/836#discussion_r1766284649)
`SplitHostPort()` returns "true if port-portion is absent or within its allowed range". Here we want the port to be present and with its allowed range. So if we check that `SplitHostPort()` is true, that is not sufficient. For our usecase `SplitHostPort()` must be true and the port must be present. How to check whether the port is present? That would be `port != 0`. So that condition should be:

```suggestion
if (SplitHostPort(proxy.toStdString(), port, hostname) && port != 0) {

...

💬 Muskreeve commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2360191658)

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1766373294)
Had a few versions before, played around with it a bit more, thanks for the pointers, what do you think of the new structure and names.

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2360294161)
Rebased f9245e4bb880a6d76bbb994a6e1907c6f8c9f205 -> 54611e99f5009def3a4559874823ed7fd91c9252 ([`pr/mine-types.12`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.12) -> [`pr/mine-types.13`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.13), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.12-rebase..pr/mine-types.13)) due to conflict with #30875

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1766374252)
Clever, thanks!

💬 l0rinc commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#issuecomment-2360387991)
> changing HasReason::m_reason itself into a string_view as well(?), which I agree would increase the risk of dangling issues

While string literals have static storage duration, referencing them via `std::string_view` should be safe (i.e. for `HasReason` storage as `const std::string_view m_reason`).
But you're both right that lifetime management can be more complex than what we have in this test, so using an `std::string` copy avoids dangling references.
Thanks for the reviews, pushed.

🤔 ryanofsky reviewed a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2314893465)
Updated 54611e99f5009def3a4559874823ed7fd91c9252 -> d59f5a31a1b888051eb47e2f8a5fb8d901de1d10 ([`pr/mine-types.13`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.13) -> [`pr/mine-types.14`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.14), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.13..pr/mine-types.14)) addressing review comments to clarify code

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1766453086)
re: [#30510 (comment)](https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708942643)

> Nit: it would be nice to give this the global `g_` prefix (though it is not introduced in this PR).

Added :: prefix to clarify it is a global

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1766451969)
re: [#30510 (comment)](https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1715214130)

> I should add a comment explaining this better

Improved comment now

💬 pablomartin4btc commented on pull request "Fix display issues for IPv6 proxy setup in Options Dialog (UI only, no functionality impact)":
(https://github.com/bitcoin-core/gui/pull/836#discussion_r1766515133)
After playing with the function and the tests (`netbase_tests` - `TestSplitHost` - & `net_tests` - `addr.IsValid()`), what you propose looks more correct/ understandable to me as the use cases we were looking at were "::1:1080" (which passed to `SplitHostPort()` returns valid/ true and port 0) vs "[::1]:1080" (returning true and port 1080).

> If there is a bug in SplitHostPort() that should be fixed separately.

I think it's clear to me now that if you have multiple ":" it's because it's an
...

🚀 fanquake merged a pull request: "ci: Print inner env, Make ccache config more flexible"
(https://github.com/bitcoin/bitcoin/pull/30869)

💬 pablomartin4btc commented on pull request "Fix display issues for IPv6 proxy setup in Options Dialog (UI only, no functionality impact)":
(https://github.com/bitcoin-core/gui/pull/836#issuecomment-2360536957)
Updates:
- Addressed @vasild's feedback.

👍 ryanofsky approved a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2315076449)
Code review ACK facbcd4cef8890ae18976fb53b67ea56b3c04454

Only change since last review is log function declaration

👍 l0rinc approved a pull request: "build: Improve `ccache` performance for different build directories"
(https://github.com/bitcoin/bitcoin/pull/30861#pullrequestreview-2315045578)
ACK 3f8afcefb53f18d735d9cd196df492d2d140c284

### Normal build on master:

`% ccache --clear --zero-stats`
`% cmake -B build1 && cmake --build build1 -j10`
`% ccache --show-stats`
```bash
Cacheable calls: 440 / 440 (100.0%)
Hits: 0 / 440 ( 0.00%)
Direct: 0
Preprocessed: 0
Misses: 440 / 440 (100.0%)
Local storage:
Cache size (GiB): 0.2 / 5.0 ( 4.89%)
Hits: 0 / 440 ( 0.00%)
Misses: 440 / 440 (100.0%)
...

💬 l0rinc commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1766546695)
Will this [affect coverage](https://github.com/ccache/ccache/discussions/268) or is that an old issue that doesn't affect us anymore?