👍 willcl-ark approved a pull request: "doc: Fixup bitcoin-wallet manpage chain selection args"
(https://github.com/bitcoin/bitcoin/pull/31264#pullrequestreview-2425778116)
utACK fa729ab4a276c3462e390bf2fab6cad93d3a590d

Thanks Marco, this makes sense to me.

💬 Sjors commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#discussion_r1835720824)
Fixed

💬 Sjors commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#discussion_r1835720992)
I updated the commit message to include a reference to a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed where this check was added.

(will push shortly)

📝 Sjors opened a pull request: "GitHub skip branch"
(https://github.com/bitcoin/bitcoin/pull/31265)
Testing https://github.com/bitcoin/bitcoin/pull/30487

✅ Sjors closed a pull request: "GitHub skip branch"
(https://github.com/bitcoin/bitcoin/pull/31265)

👍 tdb3 approved a pull request: "doc: Fixup bitcoin-wallet manpage chain selection args"
(https://github.com/bitcoin/bitcoin/pull/31264#pullrequestreview-2425820823)
Code Review ACK fa729ab4a276c3462e390bf2fab6cad93d3a590d

Would also support adding an example:
```diff
- "To change the target wallet, use the -datadir, -wallet and (test)chain selection arguments.\n"
+ "To change the target wallet, use the -datadir, -wallet and (test)chain selection arguments (e.g. -signet).\n"
```

💬 Sjors commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2466833320)
Github documentation is pretty terrible. The change here should work, but the jobs are not skipped in https://github.com/Sjors/bitcoin/pull/70

👍 andrewtoth approved a pull request: "rpc: increase the defaults for -rpcthreads and -rpcworkqueue"
(https://github.com/bitcoin/bitcoin/pull/31215#pullrequestreview-2425823091)
ACK e56fc7ce6a92eae7e80657d9f57a148cc002358d

Bumping the default threads and work queue enables a lot of workflows by default, like using it with a local address indexer. Right now these users have to manually configure the higher limits.
The higher limits won't increase memory unless they are being used. And these defaults can be set lower on memory constrained systems to disable using them.

👍 tdb3 approved a pull request: "ci: make ctest stop on failure"
(https://github.com/bitcoin/bitcoin/pull/31257#pullrequestreview-2425829015)
code review and test ACK 36a22e5683375b7925767de9daa9df4c48831c15

- Grepped for `ctest` in other directories (in case other files should be adjusted). Didn't see anything else to update.
- Sanity checked by inserting an intentional failure (`BOOST_CHECK_EQUAL(1, 2)`) into `addrman_simple` and ran `ctest` locally as defined in the CI ymls to confirm the commands with `--stop-on-failure` behave as expected (i.e. stop early). Behaved as expected.

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1835766013)
This is unfortunate that `privatebroadcast` is incompatible with `connect`. The use case would be to connect to a single trusted peer for listening, but broadcast through ephemeral connections to not link the trusted peer to our wallet.

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2466864426)
@fanquake done and rebased after #31105.

💬 1440000bytes commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2466865095)
> @1440000bytes, thanks for asking! There is some discussion at #27509 (the previous attempt on this).
>
> > Is it necessary to open new short lived tor/i2p connections for broadcasting the transaction?
>
> Yes, it is. See below.
>
> > What are the trade-offs in this implementation vs a simple implementation to relay tx to one or more peers that our node is already connected to?
>
> Sending the transaction over clearnet reveals the IP address/geolocation of the sender. A spy with man
...

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2466904285)
It looks like the `no multiprocess, i686, DEBUG` functional tests are still trying to check multiprocess stuff despite `NO_MULTIPROCESS=1`.

> FileNotFoundError: [Errno 2] No such file or directory: 'bitcoin-node'

🤔 l0rinc reviewed a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2425834191)
Nice, simple approach, like it a lot!
I think we can simplify the validator a bit more - let me know what you think.

Also, not exactly sure why `%n` parity wasn't added like in https://github.com/bitcoin/bitcoin/pull/30999/files#diff-71badc1cc71ba46244f7841a088251bb294265f4fe9e662c0ad6b15be540eee4R69 (is it controversial or unnecessary or not useful)?

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1835766554)
Given that we have two separate number "parsers" (one that keeps the result and one that throws it away), we might as well extract number parsing to a local lambda like you did with the other ones.

<details>
<summary>Diff</summary>

```patch
diff --git a/src/util/string.h b/src/util/string.h
--- a/src/util/string.h (revision ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba)
+++ b/src/util/string.h (date 1731267170701)
@@ -45,14 +45,16 @@
continue;
}

+
...

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1835773438)
Checked and failures seem to be validated successfully from command line, but - unlike the previous versions - doesn't seem to be shown in the IDE... Weird :/

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1835770569)
These are all here to check if we're inside a format string, but we don't have a `%%`, right?
Could we maybe simplify that to something like:
```suggestion
if (*it != '%' || *(++it) == '%') continue; // Skip escaped %%
```
?

💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1835770704)
In C++23 this could be a simple `.contains`, but even in C++20 we should be able to group the flags to something like:
```suggestion
while ("#0- +"sv.find(*it) != std::string_view::npos) ++it;
```
(we could even extract the flag in which case we could get rid of the comment)

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2466909081)
I set `export BITCOIND=bitcoin-node` just for the ARM job for now.

💬 grzegorzblaszczyk commented on something "":
(https://github.com/bitcoin/bitcoin/commit/c5a9d2ca9e3234db9687c8cbec4b5b93ec161190#commitcomment-148931454)
@ditto-b Nice catch!