🤔 janb84 reviewed a pull request: "cmake: Respect user-provided configuration-specific flags"
(https://github.com/bitcoin/bitcoin/pull/32356#pullrequestreview-2798512878)
Few suggestions, although the changes do work as described (tested)

<details>

Master:
`cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"`
```
C++ compiler flags .................... -O2 -std=c++20 -fPIC -fdebug-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -fmacro-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function
...

💬 janb84 commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2063214810)
```suggestion
set_property(CACHE "${var_name}" PROPERTY VALUE "${${var_name}}")
```
NIT: add quotes to ensure proper handling of variable name

💬 janb84 commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2063210467)
```suggestion
string(REGEX REPLACE "(^| )${old_flag}( |$)" "\\1${new_flag}\\2" "${var_name}" "${${var_name}}")
```
NIT: add quotes to ensure proper handling of variable name

💬 janb84 commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2063192800)
NIT: `precious_variables` is used but not defined or passed into the function. Would it not be better (more pure) to pass the variable into the function ?

💬 fanquake commented on pull request "Crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2834519435)
Backported to 29.x in #32292.

💬 fanquake commented on issue "build: CMake caching failing PIE check":
(https://github.com/bitcoin/bitcoin/issues/32323#issuecomment-2834538457)
Not sure I understand your suggestion; are you saying we don't fix this? Are you saying we should add a recommendation to the build docs to always delete the build dir before running CMake? Something else? Note that this is a generic issue, and the PIE check is just the first check that fails in this way, if you remove the PIE check the threads check will fail the same.

💬 vasild commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2063248425)
Does it make sense to advance `pindexLastCommonBlock` in that case? I am just curious, it is definitely not the aim of this PR.

💬 fanquake commented on pull request "doc: Fix fuzz test_runner.py path":
(https://github.com/bitcoin/bitcoin/pull/32353#issuecomment-2834572446)
Backported to 29.x in #32292.

💬 fanquake commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063264880)
Can you update zmq version used to 4.3.5 (https://github.com/bitcoin/bitcoin/pull/28627).

💬 fanquake commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063265648)
Can you update sqlite version used to 3.46.1 (#29991).

💬 fanquake commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834586735)
> but was found by https://github.com/bitcoin/bitcoin/pull/32339 momentarily

It was actually first pointed out here, https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2500175006, by @dergoegge.

💬 letetrautre commented on pull request "test: Force named args in RPCOverloadWrapper optional args":
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063286019)
bc1qw4lvl39reqarm65hlq2rfnvr4s4hpr8cmdttaf

💬 letetrautre commented on pull request "test: Force named args in RPCOverloadWrapper optional args":
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063287118)
bc1qw4lvl39reqarm65hlq2rfnvr4s4hpr8cmdttaf

💬 hebasto commented on pull request "test: avoid stack overflow in `FindChallenges` via manual iteration":
(https://github.com/bitcoin/bitcoin/pull/32351#issuecomment-2834604584)
@theuni

Due to your https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2524210640, you might b e interested in reviewing this PR.

🤔 Sjors requested changes to a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2798630677)
Concept ACK.

bc3f07e384c2e145d6d14cfa3ad65b976233b538 looks good, except:

1. The GUI now shows a watch-only balance for descriptor wallets. See inline for the culprit.

2. In that same commit, the churn in `wallet/scriptpubkeyman.cpp` to follow, both here on Github as well as a `git show --color-moved=dimmed-zebra`. I'm guessing you're moving stuff around, deleting and modifying?

And a few details:
- there's one more `-DWITH_BDB=ON` in `test-each-commit-exec.sh`
- `test_framework/bd
...

💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063255286)
In dc7bf5fd6a320c4528a28cef2a565366bbab3877 "wallet: Delete LegacySPKM": why doesn't this just return `false` like before? And if so, can't the entire helper be dropped?

💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063278522)
In dc7bf5fd6a320c4528a28cef2a565366bbab3877 "wallet: Delete LegacySPKM": this test (file) and the one below still exist.

💬 maflcko commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834611665)
> I'd say for the CI we want to reset `/Ob0 /Od` back to `/O2 /Ob1` to avoid slowness.

@hebasto If you don't mind, could you update your CI branch with this. If not, that's fine also and I'll try it myself.

💬 hebasto commented on issue "build: CMake caching failing PIE check":
(https://github.com/bitcoin/bitcoin/issues/32323#issuecomment-2834619343)
> are you saying we don't fix this?

Correct.

> Are you saying we should add a recommendation to the build docs to always delete the build dir before running CMake? Something else?

Not "always" — only when configuration fails. Similar to https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2737914378.

💬 tomasandroil commented on pull request "Fix missing error check in `set_clo_on_exec` for FD_CLOEXEC handling":
(https://github.com/bitcoin/bitcoin/pull/32342#issuecomment-2834621951)
@hebasto
I have upstreamed the changes as requested: [arun11299/cpp-subprocess#117](https://github.com/arun11299/cpp-subprocess/pull/117).