💬 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.

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3352416170)
`39f90a4a78...c652deb3c1`: address suggestions

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2391694137)
Added `local`, thanks!

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2391694968)
Done.

💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2391749840)
A third alternative would be to manually re-run *both* tasks on an unrelated or intermittent failure, instead of just one.

I guess this is good enough for now.

💬 pablomartin4btc commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3352499001)
> That’s the idea. Have a single place where we log all possible DB engines and configurations.

At the moment, we use Berkeley DB when we want to migrate a Legacy wallet, the above statement maybe should consider this case.

> That would print the same message multiple times. The goal is to print it only once during init so we know the available DB engine version.

Nop, previous one was 5e130a1e999b8a15d709cb698814c26a85eb5447, but I'm ok to centralize the DB engines logging in one place,
...