💬 hebasto commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#discussion_r2390916307)
Reworked.

👍 stickies-v approved a pull request: "wallet: reduce unconditional logging during load"
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3284021822)
ACK fc861332b351c9390400054ff74193ce26eb0713

nit: Would still prefer doing the logging from `WalletInit::Construct`, `VerifyWallets` feels out of place to me, but no biggie.

<details>
<summary>git diff on fc861332b3</summary>

```diff
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 79e2c9688d..4013e12967 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -20,6 +20,7 @@
#include <util/translation.h>
#include <wallet/coincontrol.h>
#include <wallet/walle
...

💬 stickies-v commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2390924243)
nit: docstring doesn't correspond to what's actually being printed. To avoid it going stale quickly, how about a simple:

> // Logs information about the database(s) used by the wallet.

💬 stickies-v commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2390909544)
nit: the function name is informative enough imo, no need for docstring here

📝 maflcko opened a pull request: "ci: Check macos-cross"
(https://github.com/bitcoin/bitcoin/pull/33509)
bla

💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2391260220)
Hmm, reminds me that this hunk (and the one above) for `windows-cross` and `windows-native-test` was erroneous to replace, because the two tasks depend on each other and must checkout the exact same commit. Otherwise, there could be a CI failure when a pull is merged in between the run of the first task and the second.

Possible solutions could be:

* Revert the two hunks and restore the default behavior here.
* Export the commit ID from the first task and plug it into the checkout of the
...

👋 willcl-ark's pull request is ready for review: "ci: fix buildx gha cache authentication on forks"
(https://github.com/bitcoin/bitcoin/pull/33508)

🤔 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