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

💬 furszy commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3352513225)
> At the moment, we use Berkeley DB when we want to migrate a Legacy wallet, the above statement maybe should consider this case.

You can't initialize a BDB wallet during startup. The BDB engine is not part of the binary anymore. We just have a BDB reader that parses the db file on-demand during migration.

🤔 hebasto reviewed a pull request: "CMake: Add dynamic test discovery"
(https://github.com/bitcoin/bitcoin/pull/33483#pullrequestreview-3285383953)
> Instead of parsing the test names from the source code at configure time, query the list of tests from the test executables at testing time.

It might also be helpful to explain *why* this approach is an improvement.

The current implementation does not handle skipped tests correctly:
- on the master branch:
```
$ ctest --test-dir build -j 20
<snip>
73/148 Test #88: script_assets_tests ..................***Skipped 0.13 sec
<snip>

100% tests passed, 0 tests failed out of 148


...

💬 hebasto commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2391884367)
The property name started with `CTEST_` seems to be reserved, no?

💬 mzumsande commented on pull request "p2p: Use network-dependent timers for inbound inv scheduling":
(https://github.com/bitcoin/bitcoin/pull/33464#discussion_r2391996003)
Good question - while the map initially empty, we insert entries with
`auto& timer{m_next_inv_to_inbounds_per_network_key[network_key]};`
Even though default-initialisation doesn't happen in general for `std::atomic`, this should still be correct because `std::map:operator[]` uses _value initialisation_ (to 0).
That said, I think it's better to use `try_emplace` to make the intent clearer, so I will change it to that.

💬 mzumsande commented on pull request "p2p: Use network-dependent timers for inbound inv scheduling":
(https://github.com/bitcoin/bitcoin/pull/33464#issuecomment-3352747761)
[beb75e4](https://github.com/bitcoin/bitcoin/commit/beb75e48ae1d5771932f427a490c7e1b6c1720d3) to [0f7d4ee](https://github.com/bitcoin/bitcoin/commit/0f7d4ee4e8281ed141a6ebb7e0edee7b864e4dcf):
- used `try_emplace` instead of `[]` for map insertion
- Removed the atomic and guarded `m_next_inv_to_inbounds_per_network_key` by `g_msgproc_mutex`. `NextInvToInbounds` is already guarded by `g_msgproc_mutex` anyway, so I'm not sure what the benefit of the atomic was (there was even a comment, removed n
...

💬 optout21 commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3352795379)
ACK 0c718da693ad4727009e656938a1208ec52866af

Fairly reduced scope; motivated by #29415 .

⚠️ ismaelsadeeq opened an issue: "Mempool Expiry eviction might remove txs that could be mined in the next block"
(https://github.com/bitcoin/bitcoin/issues/33510)
`LimitMempoolSize` always try to remove transactions from the mempool whose age has passed the mempool expiry time limit, which is 336 hours (i.e., two weeks) by default.

There might be a case where you evict something that might be mined next. For example, the mempool was cleared and some low feerate transaction that has been in the mempool and has not confirmed in a long time due to having a low feerate is now at the top of the mempool.
However, when we want to limit the mempool size, we will
...

💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392113431)
potential refactoring idea: the derivation path lookup prior to calling `SignMuSig2` currently happens for both key- and script-path spending. Could it be moved inside of `SignMuSig2` in order to deduplicate (and also reduce the interface by one parameter, as the `KeyOriginInfo` object would be created inside)? Tried this here, and the tests still pass: https://github.com/theStack/bitcoin/commit/fc20504660aed9674ae27482fad885b5b9fe399f