💬 fanquake commented on pull request "refactor: Avoid unsigned integer overflow in `script/interpreter.cpp`":
(https://github.com/bitcoin/bitcoin/pull/29543#issuecomment-2385715734)
Closing in favour of the other one.

✅ fanquake closed a pull request: "refactor: Avoid unsigned integer overflow in `script/interpreter.cpp`"
(https://github.com/bitcoin/bitcoin/pull/29543)

💬 maflcko commented on pull request "doc: add testnet4 section header for config file":
(https://github.com/bitcoin/bitcoin/pull/31007#discussion_r1782730746)
```suggestion
- placed into sections with headers `[main]` (not `[mainnet]`), `[test]` (not `[testnet]`, for testnet3), `[testnet4]`, `[signet]` or `[regtest]`;
```

It would be `[testnet4]`, not `testnet4`.

💬 Sjors commented on pull request "depends: Print ready-to-use `--toolchain` option for CMake invocation":
(https://github.com/bitcoin/bitcoin/pull/31008#issuecomment-2385746639)
tACK 605926da0ab4ac7ae4e9aed55a059f37c31c15b5

💬 pablomartin4btc commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782780671)
My bad, my suggestion was more for discussion as I did it on top of @achow101's one. The tests can be updated but I see we could be breaking things on users end if they pass named args (`filename=`) as in `wallet_startup,py`.

💬 MarnixCroes commented on pull request "doc: add testnet4 section header for config file":
(https://github.com/bitcoin/bitcoin/pull/31007#discussion_r1782784699)
fixed, my bad...

📝 ryanofsky opened a pull request: "refactor: move util/pcp and util/netif to common/"
(https://github.com/bitcoin/bitcoin/pull/31011)
Move util/pcp.cpp and util/netif.cpp to common/ because they depend on netaddress.cpp which is part of the common library. This was causing check-deps.sh script to fail as reported by _fanquake_ in https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2385475097.

Also fix CI to fail when the `check-deps.sh` script fails. Previously it would output errors but not cause the job to fail (which was unintentional).

💬 ryanofsky commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2385820890)
> Looks like this doesn't fail when it detects a problem? i.e the tidy job is passing (most recent run: https://api.cirrus-ci.com/v1/task/5757501226876928/logs/ci.log), but is currently outputting:

Nice catch. Opened #31011 to address this

🤔 danielabrozzoni reviewed a pull request: "Add waitFeesChanged() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31003#pullrequestreview-2340337601)
Code review ACK 9a723c784ad5170b1e01e91f018e5794a51dd038 - code looks good to me, I don't have an opinion on merging both commits vs waiting for cluster mempool for the second one.

🤔 hodlinator reviewed a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2339765974)
Concept ACK 4f33e9a85c3a8f546deddc2f78cf74bceff30315

It would be cleaner *in the implementation* to use `util::Result` instead of overwriting the returned value with the error message, but it would mess up the call sites too much probably. Current approach in this PR seems one of the least worse ones.

Passed locally: `FUZZ=str_printf src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/str_printf/*`.

### PR summary nit
> rename the throwing `tfm::format` functions to `tfm::format_raw`

T
...

💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1782505271)
Agree the current message is confusing. The distinction between format string and parameters to the format string should ideally be made clearer, as well as the actual string type.
```suggestion
- *Rationale*: Tinyformat handles `std::string` parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
```

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1782953943)
done

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2386103070)
@maflcko done, thanks

💬 Sjors commented on pull request "doc: add testnet4 section header for config file":
(https://github.com/bitcoin/bitcoin/pull/31007#issuecomment-2386113770)
ACK 61cdb1c9d83778b95f4f9596f34617b7a191d0a5

💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1782965796)
Exactly like that, thanks!

💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1782975091)
Ah, yeah, `mempool_util.py` looks like a better alternative than `blocktools.py`.

`test_node.py` feels more like the high-level P2P stuff to me at least + lower level dynamic RPC invocation.

💬 Sjors commented on pull request "refactor: move util/pcp and util/netif to common/":
(https://github.com/bitcoin/bitcoin/pull/31011#issuecomment-2386200566)
utACK fd38711217cafbd62e8abd22d2b43f85fede8cde

I suppose the alternative would be to move `netaddress.h` to the util library.

The `libraries.md` document is a bit vague on what belongs to `util` vs `common`. It just says "low level". Since util is also used by `libbitcoin_kernel` it could make sense to move things out of it that the kernel doesn't need, but #28690 introduces a whole new `kernel_util` for that purpose.

💬 hebasto commented on pull request "build, qt: Add Wayland support for Linux builds with depends":
(https://github.com/bitcoin/bitcoin/pull/22708#issuecomment-2386230968)
This PR might be reconsidered along with https://github.com/bitcoin/bitcoin/issues/29914.

👍 theuni approved a pull request: "depends: Print ready-to-use `--toolchain` option for CMake invocation"
(https://github.com/bitcoin/bitcoin/pull/31008#pullrequestreview-2340626823)
ACK 605926da0ab4ac7ae4e9aed55a059f37c31c15b5

💬 ryanofsky commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2386241193)
> MSAN says https://cirrus-ci.com/task/6248232848719872

Thanks! I created https://github.com/chaincodelabs/libmultiprocess/issues/115 with details about this, and I think I have a potential fix.