💬 vasild commented on pull request "net: remove unnecessary check from AlreadyConnectedToAddress()":
(https://github.com/bitcoin/bitcoin/pull/32338#discussion_r2063129623)
The added fuzz test does assert that if addr:port equals then `CNetAddr` must also equal. Could add `Assume()` here, something like:

```cpp
bool CConnman::AlreadyConnectedToAddress(const CAddress& addr)
{
const bool match{FindNode(static_cast<CNetAddr>(addr))};
if (!match) {
Assume(!FindNode(addr.ToStringAddrPort()));
}
return match;
}
```

I think that is a kind of an overkill, given the fuzz and unit tests added, but if reviewers want it, then I will add t
...

📝 maflcko opened a pull request: " test: Force named args in RPCOverloadWrapper optional args "
(https://github.com/bitcoin/bitcoin/pull/32360)
This can avoid bugs and makes the test code easier to read, because the
order of positional args does not have to be known or assumed.

Also, contains two commits to remove dead code.

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2063194316)
I was referring to `if 'descriptors' not in wallet_info or ('descriptors' in wallet_info and not wallet_info['descriptors']):`. However, I see that it may be used by tests using previous releases.

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2063195071)
Actually, the `cli` field can be removed. Done in https://github.com/bitcoin/bitcoin/pull/32360

🤔 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?