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

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163090187)
Added to reference in commit message.

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163987318)
Broke out removal of `NodeRef` & `MakeNodeRef()` as mentioned in other comment.

Let me know if you still see potential in for a non-contrived scripted diff.

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163836697)
It got a bit complicated since `NodeRef` is internally `const` (`std::unique_ptr<const Node<Key>>`). Did it as a separate commit for now: effcf701474eacd85a24da4d1707120fc756aa91

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163180665)
Was doing that in earlier pushes of the PR but changed back to `!= nullopt` since it disambiguates pointer/optional cases and is closer to master.

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163967956)
Broke out removal of `NodeRef` & `MakeNodeRef()` into its own commit: c4d30115f5cefe3945ac8b69c021a551eb2b6a72

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2164001056)
Agree I went overboard in those 3 cases, :+1: reverted.