💬 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

💬 fanquake commented on pull request "depends: sqlite 3.46.1":
(https://github.com/bitcoin/bitcoin/pull/29991#issuecomment-2437444272)
Looks like as of `3.48.0` (current release is `3.47.0`), SQLite is going to migrate it's build system from Autotools to [autosetup](https://msteveb.github.io/autosetup/): https://sqlite.org/src/timeline?r=autosetup.

🚀 fanquake merged a pull request: "depends: sqlite 3.46.1"
(https://github.com/bitcoin/bitcoin/pull/29991)

💬 brunoerg commented on pull request "[tests] New fuzz target wallet_rpc":
(https://github.com/bitcoin/bitcoin/pull/30570#discussion_r1816460307)
In 26591de46c93a0c73df8c9133af69dcc7f4db569: `g_limit_to_rpc_command_wallet` is not being set, so `LIMIT_TO_RPC_COMMAND` has no effect in this target. When using `LIMIT_TO_RPC_COMMAND` for this target, it's probably setting `g_limit_to_rpc_command` in `fuzz/rpc.cpp`.

💬 brunoerg commented on pull request "[tests] New fuzz target wallet_rpc":
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2437465419)
> @brunoerg I think this should be ready for review now, I think I just need to currate the list of WALLET_RPC_COMMANDS_SAFE_FOR_FUZZING and WALLET_RPC_COMMANDS_NOT_SAFE_FOR_FUZZING

Did you advance on curating the list? Is this the final list?

💬 brunoerg commented on pull request "[tests] New fuzz target wallet_rpc":
(https://github.com/bitcoin/bitcoin/pull/30570#discussion_r1816464048)
In 26591de46c93a0c73df8c9133af69dcc7f4db569: nit: If something got wrong with `wallet/test/fuzz/rpc.cpp`, this will ask to update the `test/fuzz/rpc.cpp` file.

🤔 fanquake reviewed a pull request: "Drop miniupnp dependency"
(https://github.com/bitcoin/bitcoin/pull/31130#pullrequestreview-2394931860)
Not sure how much you want to do here, but I think this code can be cleaned up further, given it was written to cater for using two protocols, but after this change, we've got 1 protocol (with a fallback).

💬 fanquake commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1816467880)
I think you can just drop this comment entirely, as there is no-longer a "next protocol" in the loop.

💬 fanquake commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1816475493)
Above here:
```cpp
if (g_mapport_enabled_protos & g_mapport_current_proto) {
// Enabling another protocol does not cause switching from the currently used one.
return;
}
```
This code is either dead, or the comment is outdated, as we can no-longer "enable another protocol".

💬 fanquake commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1816468353)
Just up from here, could drop `// High priority protocol.`.

👍 instagibbs 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-2394949723)
ACK 5c299ecafe6f336cffa145d28036b04b87e26712

feel free to ignore nit

💬 instagibbs 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_r1816479232)
nit: logging here seems excessive considering a failure of the assert makes it obvious what went wrong

👍 tdb3 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-2394975338)
ACK 5c299ecafe6f336cffa145d28036b04b87e26712

I would also re-ACK quickly if the changes recommended by @rkrux and @instagibbs were implemented, and `DEFAULT_MAX_ORPHAN_TRANSACTIONS` moved to `test_framework/mempool_util.py`