💬 maflcko commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816298165)
nit: Maybe to avoid the churn here (changing the same line of code twice), and the c_str bloat, a simple alias could be introduced in an early commit, so that only the alias has to be changed internally in later commits?

For example

```cpp
template<typename... Args>
void fuzz_fmt(const std::string &fmt, const Args&... args){(void)tfm::format_raw(fmt.c_str(), args...);}

💬 rkrux commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1816314261)
Agree, having this assertion would be good because the valid tx would still not make to mempool in this case.

🤔 rkrux reviewed a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139#pullrequestreview-2394696857)
LGTM, will ACK again once @instagibbs's suggestion is also added.

https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1815824499

💬 rkrux commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1816314768)
Are these blank lines intentional?

💬 laanwj commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437290310)
i've checked that no references no miniupnp as dependency remain after this change (except in historical release notes ofc).

Also, successful guix build of 2a541e93cf13:
```
d0674bc3aa12577fb8d0319d51b34ab8650b810f56868a701b388f4f91cfc9b2 guix-build-2a541e93cf13/output/aarch64-linux-gnu/SHA256SUMS.part
6be1f0a8fa6596bd4e6e68996b9a53cb438fb727e55755f6bf101656e1517c47 guix-build-2a541e93cf13/output/aarch64-linux-gnu/bitcoin-2a541e93cf13-aarch64-linux-gnu-debug.tar.gz
a9386bc64b0222ce03a3a
...

👍 hodlinator approved a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2394616462)
re-ACK 80d35bc20797e59571e82fe6919705b33dcc40cd

* Commit message improvements.
* `test_path` -> `test_name` (improved naming).
* Made `test_name` be included as part of randomized test directory when *not* setting `-testdatadir`. Including it is helpful for locating test data of a specific test when multiple tests fail. :+1:

Verified correct unit test directory behavior using `build/src/test/test_bitcoin -- -testdatadir=/tmp/foo/`, then not passing the `-testdatadir` and simultaneously r
...

💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1816263171)
(Since with the latest non-rebase push you are now touching 2/4 lines mentioning `TEST_DIR_PATH_ELEMENT`, iff you were to re-touch again, it would be nice if you also renamed it to `TESTS_DIR_NAME` as per @ryanofsky's [suggestion](https://github.com/bitcoin/bitcoin/pull/30748#pullrequestreview-2283763614)).

✅ fanquake closed a pull request: "depends: bump miniupnpc to 2.2.8"
(https://github.com/bitcoin/bitcoin/pull/30301)

💬 fanquake commented on pull request "depends: bump miniupnpc to 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2437321903)
I think #31130 is going to go in for `v29.0`. If that doesn't happen, we can reopen here.

💬 rkrux commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1816346116)
Was this https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815889280 done in 5c299ecafe6f336cffa145d28036b04b87e26712? I could not find it.

👍 rkrux approved a pull request: "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped"
(https://github.com/bitcoin/bitcoin/pull/31040#pullrequestreview-2394740828)
ACK 5c299ecafe6f336cffa145d28036b04b87e26712

Thanks for adding this test, it's easy to follow through.

💬 rkrux commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1816349294)








```suggestion
assert_equal(len(node.getorphantxs()), DEFAULT_MAX_ORPHAN_TRANSACTIONS)
```
Storing in another variable is not necessary only for one time usage.

You don't need to invalidate ACKs only for this though.

💬 laanwj commented on pull request "depends: bump miniupnpc to 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2437336876)
> I think https://github.com/bitcoin/bitcoin/pull/31130 is going to go in for v29.0. If that doesn't happen, we can reopen here.

yes... i dont think i've seen any PR that popular in my time here 😄

💬 dergoegge commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2437349340)
Waiting for #31150 to go in

👍 fanquake approved a pull request: "ci: Temporary workaround for old CCACHE_DIR cirrus env"
(https://github.com/bitcoin/bitcoin/pull/31146#pullrequestreview-2394783289)
ACK fa9747a896188f4dd70f275aec2469dba5cd435e - have seen this now.

🚀 fanquake merged a pull request: "ci: Temporary workaround for old CCACHE_DIR cirrus env"
(https://github.com/bitcoin/bitcoin/pull/31146)

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680)
Not sure about the CI failure. IIRC it happened after this push: https://github.com/bitcoin/bitcoin/compare/15cfeebb47587af6ce7be72fd52c57a0483d86d2..5757fdf0dc74ec9d6bbefba937d6e23b09652605

The logs don't say anything except: `validation_chainstatemanager_tests ...Exit code 0xc0000409
2024-10-24T23:23:18.2978457Z ***Exception: 9.33 sec` and the debug log is truncated (presumably when the exception happens).

It would be good if ctest/Windows actually printed the exception and error mess
...

👍 brunoerg approved a pull request: "util: Treat Assume as Assert when evaluating at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31150#pullrequestreview-2394823764)
ACK fa69a5f4b76a4e2a02db6c32d9c3311ce5fe29bd

💬 laanwj commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1816413934)
thanks!
factoring it out was more fo a readablity thing, i don't really mind if it's in netutil
i'm fine with leaving it where it is, if some other code needs it it can always be moved to a more general place

👍 marcofleon approved a pull request: "util: Treat Assume as Assert when evaluating at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31150#pullrequestreview-2394880609)
ACK fa69a5f4b76a4e2a02db6c32d9c3311ce5fe29bd