🤔 stratospher reviewed a pull request: "p2p: Use network-dependent timers for inbound inv scheduling"
(https://github.com/bitcoin/bitcoin/pull/33464#pullrequestreview-3284507245)
ACK beb75e4. liked the way the cache_id logic from getaddr caching was reused here.
observed something similar with @danielabrozzoni's patch!

```
2025-09-30T12:29:05Z Scheduling next inv send time for inbound peer id = 0 on network 8495781695020407500 at time 1759235352548196µs
2025-09-30T12:29:05Z Scheduling next inv send time for inbound peer id = 4 on network 8495781695020407500 at time 1759235352548196µs
2025-09-30T12:29:06Z Scheduling next inv send time for inbound peer id = 2 on net
...

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2391266074)
Added `// Avoid non-loopback network traffic during tests.`

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2391272544)
I added brief comments to the newly added functions. `ci/README.md` seems to me too high level for this. No strong opinion though.

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3351911963)
`f4fdf81d3a...39f90a4a78`: add some comments, suggested by @fjahr above. Also, restore the `exit 1` which I accidentally removed in a previous push.

> I was hoping I could easily test ...

The traffic is detected and reported in your CI jobs (search for `Error: outbound`), but there was no `exit 1` 🤦. Sorry for that :face_in_clouds: :face_with_head_bandage:

💬 vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3351986932)
`2ae966b222...0c718da693`: https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2390323798 plus use the correct comment: `/*proxy_override=*/`.

💬 vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2391342175)
I agree. Changed.

💬 stratospher commented on pull request "p2p: Use network-dependent timers for inbound inv scheduling":
(https://github.com/bitcoin/bitcoin/pull/33464#discussion_r2391353585)
shouldn't we initialise this with 0?

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2391362922)
In commit "test: Add functional tests for named argument parsing" (24391ed7804a724a62034b21150c89e45ac9b625)

Not important, but I feel like "only if" in this comment is a little misleading, since the test is not confirming parameter is treated as positional only in these cases. Also it is not checking the "valid JSON" case at all. Could also be nice to replace comments in this function with log.info or log.debug calls. And it might be nice to make this example mirror the `my=wallet` example m
...

👍 ryanofsky approved a pull request: "ci: detect outbound internet traffic generated while running tests"
(https://github.com/bitcoin/bitcoin/pull/31349#pullrequestreview-3284682908)
Code review ACK 39f90a4a78020087d19491be7b315ad91f252e46 just documenting things better since last review and adding missing `exit 1` to trigger CI failure (nice find!)

💬 ryanofsky commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2391395521)
In commit "ci: detect outbound internet traffic generated while running tests" (39f90a4a78020087d19491be7b315ad91f252e46)

Not important but these are global variables. Might be a little better to set as `local test_name="$1"` etc

💬 ryanofsky commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2391385737)
In commit "test: avoid non-loopback network traffic from node_init_tests/init_test" (d8372a220fb9691347d88547e381b8579ad35edb)

Could be nice to add the same comment to functional test setup as well

https://github.com/bitcoin/bitcoin/blob/25212dfdb4cd7291392b6a94130f658c5bfa0a48/test/functional/test_framework/util.py#L460

💬 willcl-ark commented on issue "[29.x] guix build failure on ppc with xcb":
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3352106263)
I suspect might be the same fundamental issue we saw here: https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3351288235

Where a cached package is found in depends (and used) despite the package definition having changed.

@ajtowns do you have set/are you using `$BASE_CACHE` ?

💬 sdaftuar commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#issuecomment-3352132872)
ACK 06df14ba75be5f48cf9c417424900ace17d1cf4d

💬 maflcko commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2391472496)
Added minimum CI check (for the cross-build) in https://github.com/bitcoin/bitcoin/pull/33509 (draft), but not sure if this is worth it to review/merge/maintain. Feel free to leave a comment there.

🤔 katesalazar reviewed a pull request: "build: Drop support for EOL macOS 13"
(https://github.com/bitcoin/bitcoin/pull/33489#pullrequestreview-3284999451)
Was this change in src/test/fuzz/fuzz.cpp hinted by this starkshade account or did you find about it by looking the code for interesting references?

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-3352321074)
<!-- begin push-18 -->
Updated 7ba7ec9d08927f244da8d5c1a7a1d7ced3239465 -> 7a526a161fdb31a04b0afaaa112a137b9a595977 ([`pr/ipc-chain.17`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.17) -> [`pr/ipc-chain.18`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-chain.17..pr/ipc-chain.18))<!-- end --> to fix clang-tidy errors https://github.com/bitcoin/bitcoin/actions/runs/17978986033/job/51139663003?pr=29409

💬 furszy commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3352324667)
> If we have a diff DB format in the future we'd need to change this LogDBInfo() (once someone finds it).

That’s the idea. Have a single place where we log all possible DB engines and configurations. The DB engine and its configuration are not specific to any wallet; they are part of the general setup.

> I'd prefer first the previous approach or as an alternative move the logic to MakeDatabase() in src/wallet/walletdb.cpp

That would print the same message multiple times. The goal is to
...

💬 katesalazar commented on pull request "ci: Check macos-cross executables on macOS":
(https://github.com/bitcoin/bitcoin/pull/33509#issuecomment-3352353279)
Concept ACK

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin-wallet -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/19460#issuecomment-3352384455)
> I guess on the next rebase, this would have to revert #33459 ?

Thanks! I incorporated this into next #10102 update (will push when #29409 passes CI, currently there are clang-tidy errors)

💬 fjahr commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3352408932)
tACK 39f90a4a78020087d19491be7b315ad91f252e46

Thanks for addressing my comments! Tested again by pushing the latest version of the code with the change of the first commit commented out. I could observe the expected failure in the CI this time: https://github.com/fjahr/bitcoin/actions/runs/18131366221/job/51598667988

I would be happy to re-review if you decide to address @ryanofsky 's latest comments.