💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3001109067)
> I haven't reviewed in depth, but I don't think the children are supposed to be mined and then reorged, only the parent is.

Agree! I shall change the test to match this!

💬 Prabhat1308 commented on pull request "test: add functional test for upgradewallet rpc":
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3001128290)
> are there any plans that the rpc will be used anytime soon in the next couple of years? If not, it could make sense to just remove it, and add it back, in the unlikely case it will be used?

Looking through the code this is pretty much a dead rpc where no one can use it to actually upgrade wallet and there is only 1 check that `multiwallet` can't call it before this PR . I am not very sure about what the future plans are for wallet but the only case where I see this being used is if there is
...

💬 fanquake commented on pull request "build: add root dir to CMAKE_PREFIX_PATH in toolchain":
(https://github.com/bitcoin/bitcoin/pull/32798#issuecomment-3001155413)
If we ship this, it should also be backported to `29.x`.

📝 hebasto opened a pull request: "Fix build on macOS when `qt@6` is installed"
(https://github.com/bitcoin/bitcoin/pull/32804)
Fix https://github.com/bitcoin/bitcoin/issues/31009.

💬 hebasto commented on issue "29.x Having qt(@6) breaks build for qt@5 on macOS 15.0 and 13.7":
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-3001185852)
Mind testing https://github.com/bitcoin/bitcoin/pull/32804?

🤔 furszy reviewed a pull request: "wallet: remove dead code in legacy wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32758#pullrequestreview-2954618622)
Concept ACK

I think the second commit would look nicer and simpler after something like e86d71b749c08bde6002b9aa2baee824975a518a (which is part of #31423). We’d be able to inline the function at that point.

👍 instagibbs approved a pull request: "[29.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/32589#pullrequestreview-2954625132)
ACK 0922f6bbc33ac2abe3f3d9dc98dade896718864f

💬 josibake commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3001202596)
At the risk of further derailing this thread from FreeBSD and OpenBSD (sorry!), I also ran into a similar problem with not being able to override the compiler in depends. TLDR; built a nix dev environment that includes all llvm tooling (no gcc, g++) and was trying to do a depends build. This fails like so:

```
make -C depends NO_QT=1 MULTIPROCESS=1
make: Entering directory '/home/josie/bitcoin/depends'
Extracting native_capnp...
/home/josie/bitcoin/depends/sources/capnproto-cxx-1.2.0.tar.
...

💬 Prabhat1308 commented on pull request "test: add functional test for upgradewallet rpc":
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3001218662)
I am not so sure why the CI fails to call `createwallet` for the added test while works on other rpc tests . Will try to debug

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3001260759)
Rebased 53b12cac6badc2746e0fbb8f94eae5fe84fc842b -> 3ed3f7f979fb8e4cf3023deb64acf9b0a545057a ([`pr/ipc-stop.10`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-stop.10) -> [`pr/ipc-stop.11`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-stop.11), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-stop.10-rebase..pr/ipc-stop.11)) with no changes after https://github.com/bitcoin-core/libmultiprocess/pull/160 merge

Will change status from draft -> ready for review

👋 ryanofsky's pull request is ready for review: "ipc: Handle unclean shutdowns better"
(https://github.com/bitcoin/bitcoin/pull/32345)

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3001291327)
Code review ACK 071483f64932e806ac8280d8c16e9bdf6be23eb3

💬 mzumsande commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2164560686)
right, it just sends an empty headers message - kind of surprising that a test called `test_initial_headers_sync` doesn't send a single header. I think it's fine to leave the order as is.

🤔 mzumsande reviewed a pull request: "test: headers sync timeout"
(https://github.com/bitcoin/bitcoin/pull/32677#pullrequestreview-2954748917)
Code Review ACK 8d257ed5937a47e1a60b7fe085f2e984eb60a956

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2164597067)
UsageFromPeer and UsageByPeer appear to be identical functions?

🤔 pablomartin4btc reviewed a pull request: "test: add functional test for upgradewallet rpc"
(https://github.com/bitcoin/bitcoin/pull/32803#pullrequestreview-2954825150)
You need to add this (CI "no wallet..." failure) to `test/functional/rpc_upgradewallet.py`:

```
def skip_test_if_missing_module(self):
self.skip_if_no_wallet()

```

💬 pablomartin4btc commented on pull request "test: add functional test for upgradewallet rpc":
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3001520895)
As @maflcko mentioned, not sure how useful would be to keep the RPC, and maybe you can create the PR for its removal. I thought perhaps you could move this test to `wallet_backwards_compatibility.py`, but again not sure about its purpose.

cc @achow101, @furszy

🤔 hodlinator reviewed a pull request: "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/31713#pullrequestreview-2952377149)
New push in response to all concerns from https://github.com/bitcoin/bitcoin/pull/31713#pullrequestreview-2948159254

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163825798)
Thanks for the test! Added as the last commit (78db9e119136154bd733c95fc6379c780703234a). Had to increase the depth from `100'000` to `1000'000` for it to cause stack overflow. Could simplify `Clone()`ing.

> so this basically flattens out the tree structure to make sure the desctructor's call stack has at most 1 indirection? Would `TreeEval` complicate this too much?

I've brought back an earlier version of the destructor I had that destroys entire `vector<Node>`s at once in a separate comm
...

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163833758)
I prefer moving whole `vector`s. See reply to https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2160320908.