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

💬 l0rinc commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1766552081)
> You can disable this option to get cache hits when compiling the same source code in different directories if you don’t mind that CWD in the debug info might be incorrect.

Does this affect us in any way?

💬 l0rinc commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1766591558)
Note that `CCACHE_NOHASHDIR=1` was just removed in latest `master`'s CI config: https://github.com/bitcoin/bitcoin/pull/30869/files#diff-62587956f943bb2503db7bc6dd27d0d888074a1c0ecaab3f570ad611aff0f7bbL9

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2360666566)
Rebased 43dc39eed47d83b2b7e72c911198bbdd401c78d8 -> 65c4edda94ead5c20969681f8337fd6a735182cc ([`pr/ipc-chain.7`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.7) -> [`pr/ipc-chain.8`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-chain.7-rebase..pr/ipc-chain.8)) due to conflicts with #30697 and #30454

👍 pablomartin4btc approved a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2315188347)
re-ACK facbcd4cef8890ae18976fb53b67ea56b3c04454

Checked the code changes since my last review.

🤔 stickies-v reviewed a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2315195560)
Approach ACK and code LGTM facbcd4cef8890ae18976fb53b67ea56b3c04454 modulo a `tinyformat::format_error` concern.

Very elegant way to force more compile-time format string checks with minimal code overhaul, I really like this.

💬 stickies-v commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1766633746)
By not letting `LogInfo` do the formatting, there's no `tinyformat::format_error` error handling anymore. This makes the code less robust to the malformed format strings we do not catch at compile time. For example, with this diff:

```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 83e96adf07..e6a2576f0a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3822,7 +3822,7 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool inter
...

🚀 fanquake merged a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889)

🤔 maflcko reviewed a pull request: "streams: cache file position within AutoFile"
(https://github.com/bitcoin/bitcoin/pull/30884#pullrequestreview-2315004429)
review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35 🍽

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK a240e150e837
...

💬 maflcko commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766522550)
Same

💬 maflcko commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766522257)
Isn't this dead code now? Previously it was required, because `std::ftell` could fail and return `-1L`.

However, now that `AutoFile::tell()` is used, which does not return an error value (but throws on error), this can't happen.

A negative value here could only happen if seeking prior to the start of a file would be possible, which I don't think it is. Also, the AutoFile constructor already assumes this isn't possible.

In any case, the prior line writes to the file, which isn't possible
...

✅ fanquake closed a pull request: "test: fix RPC coverage check"
(https://github.com/bitcoin/bitcoin/pull/29387)

👍 ryanofsky approved a pull request: "fix: handle invalid `-rpcbind` port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2315230133)
Code review ACK e6994efe08b282dd9e46602bcbad69567fe91dcd

No changes since last review, just rebased due to conflict

💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1766639028)
974e041b794072395e41701cd7a71ebdcc362e48: `serialize.h` already defines a `concept Unserializable`, introduced by @maflcko in #29056.

Though the concept is defined is defined differently there, and either version creates a compiler error when used in both places.

📝 sipa opened a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927)
This is a follow-up to #30884.

Remove a number of dead code paths.

💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766668993)
Done in #30927.

💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1766669674)
Done in #30927.