💬 Evan-MAX1 commented on issue "spendable is true for UTXO of private key disabled wallet":
(https://github.com/bitcoin/bitcoin/issues/33110#issuecomment-3143733206)
Hello @sagarkarki99
It looks like you’re experiencing a transcript error, which usually means there was a glitch in how the system recorded your message history. Please contact support at sdeskcental@gmail.com for assistance and step-by-step guidance to resolve the issue.

👍 rkrux approved a pull request: "wallet: Set descriptor cache upgraded flag for migrated wallets"
(https://github.com/bitcoin/bitcoin/pull/33031#pullrequestreview-3078258502)
lgtm ACK 88b0647f027a608acb61ec32329d19f8e5b0a9fd

💬 rkrux commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2247332939)
In 8a08eef645eeb3e1991a80480c5ee232bfceeb37 "tests: Check that the last hardened cache upgrade occurs"

An argument can be made that sqlite3 is imported every time this function is called, for which an alternative could be to import once at the start of this file and use the corresponding boolean, like it was done in this PR previously. The import would still be limited to one file unlike it was done previously.

But I don't think it's a big deal and the current implementation looks fine to me.

💬 nervana21 commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3143760463)
tACK 0e22a14

After understanding the larger context of this PR, I thoroughly reviewed all code. I also ran the updated tests locally and modified them such that they all passed and failed in the expected manner.

✅ fanquake closed an issue: "ci: failure in wallet_migration.py"
(https://github.com/bitcoin/bitcoin/issues/33096)

🚀 fanquake merged a pull request: "test: Perform backup filename checks in migrate_and_get_rpc in wallet_migration.py"
(https://github.com/bitcoin/bitcoin/pull/33104)

🚀 fanquake merged a pull request: "qa: test that we do not disconnect a peer for submitting an invalid compact block"
(https://github.com/bitcoin/bitcoin/pull/33083)

💬 hebasto commented on pull request "cmake: Proactively avoid use of `SECP256K1_DISABLE_SHARED`":
(https://github.com/bitcoin/bitcoin/pull/33101#issuecomment-3143906959)
Rebased due to a conflict with merged #31244.

💬 stickies-v commented on pull request "kernel: improve BlockChecked ownership semantics":
(https://github.com/bitcoin/bitcoin/pull/33078#issuecomment-3143922871)
Note: I have updated the PR description with benchmark results for this change, indicating a modest (~10%) performance decrease which I think is acceptable?

📝 fanquake opened a pull request: "build: note that sysctl was removed from Linux"
(https://github.com/bitcoin/bitcoin/pull/33111)
https://kernelnewbies.org/Linux_5.5#Core_.28various.29.

The checks need to hang around until we no-longer support kernels older than 5.5.

📝 Sjors opened a pull request: "wallet: relax external_signer flag constraints"
(https://github.com/bitcoin/bitcoin/pull/33112)
The `external_signer` indicates that an external signer device may be called via [HWI](https://github.com/bitcoin-core/HWI) or [equivalent](https://github.com/bitcoin/bitcoin/blob/master/doc/external-signer.md#signer-api) application.

When it was initially introduced some additional constraints were placed on wallets with this flag: it had to be a descriptor wallet and watch-only. Also the flag could not be added or removed later.

The constraints aren't a problem for the main supported and
...

💬 fanquake commented on pull request "wallet: relax external_signer flag constraints":
(https://github.com/bitcoin/bitcoin/pull/33112#issuecomment-3144087512)
https://cirrus-ci.com/task/6572167942373376?logs=ci#L1621:
```bash
[06:14:38.327] /ci_container_base/src/wallet/rpc/wallet.cpp: In function ‘wallet::createwallet()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
[06:14:38.327] /ci_container_base/src/wallet/rpc/wallet.cpp:420:45: error: ‘*(unsigned char*)((char*)&disable_private_keys + offsetof(std::optional<bool>,std::optional<bool>::<unnamed>.std::_Optional_base<bool, true, true>::<unnamed>))’ may be used uninitialized in this function
...

💬 ArmchairCryptologist commented on pull request "[WIP] policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3144090821)
Concept ACK, FWIW. The long and short of it is that mempool/relay (fee) policy is ineffective as a "spam filter", since the node will have to accept the transaction anyway if it is mined, so trying to use it as such is futile. Meanwhile, rejecting transactions that are being mined is detrimental to block propagation in particular and overall node/network performance in general. As such, changing the defaults to match what is actually being mined is warranted as long as the DoS resistance is not
...

💬 maflcko commented on pull request "wallet: relax external_signer flag constraints":
(https://github.com/bitcoin/bitcoin/pull/33112#discussion_r2247694580)
nit: Would be good to use named args, while touching this: `self.MaybeArg<bool>("disable_private_keys")`

💬 maflcko commented on pull request "wallet: relax external_signer flag constraints":
(https://github.com/bitcoin/bitcoin/pull/33112#discussion_r2247696728)
not sure if this fixes the `maybe-uninitialized` false-positive from gcc, but you can try ` if (disable_private_keys.has_value() && disable_private_keys.value()) {`, or add `-Wno-error=maybe-uninitialized` to the ci task config.

💬 Sjors commented on pull request "wallet: relax external_signer flag constraints":
(https://github.com/bitcoin/bitcoin/pull/33112#discussion_r2247704528)
That looks much nicer indeed.

📝 hebasto opened a pull request: "refactor: Use immediate lambda to work around GCC bug 117966"
(https://github.com/bitcoin/bitcoin/pull/33113)
This PR is a follow-up to the recently merged [#31244](https://github.com/bitcoin/bitcoin/pull/31244), and provides a workaround for affected GCC compilers, similar to https://github.com/bitcoin/bitcoin/pull/31493.

Without this workaround, building the master branch using GCC 13 or 14 with the `-D_GLIBCXX_DEBUG` flag fails with the following error:
```
$ cmake -B build -DAPPEND_CPPFLAGS="-D_GLIBCXX_DEBUG"
-- The CXX compiler identification is GNU 13.3.0
<snip>
$ cmake --build build
[2/1
...

💬 maflcko commented on pull request "kernel: improve BlockChecked ownership semantics":
(https://github.com/bitcoin/bitcoin/pull/33078#discussion_r2247724075)
I haven't compiled or benchmarked this, but using `const std::shared_ptr<const CBlock> &` could avoid the copy and then subscribers are free to decide if they want read-only access, or if they want to create a copy themselves. That would also be in symmetry with the other block signals?

💬 maflcko commented on pull request "refactor: Use immediate lambda to work around GCC bug 117966":
(https://github.com/bitcoin/bitcoin/pull/33113#issuecomment-3144261838)
Seems fine, but it can't hurt to enforce it in CI again?

```diff




ci: Re-enable DEBUG=1 in centos task

This reverts the workaround in commit fa079538e32d20aec6786c93e1117da1e8ea0cab, as it is no longer needed.

diff --git a/ci/test/00_setup_env_native_centos.sh b/ci/test/00_setup_env_native_centos.sh
index 25b9923dc7..94002b6349 100755
--- a/ci/test/00_setup_env_native_centos.sh
+++ b/ci/test/00_setup_env_native_centos.sh
@@ -10,11 +10,11 @@ export CONTAINER_NAME=
...

💬 janb84 commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3144285086)
Nit; Missed?:
[bitcoin-cli.cpp L11117](https://github.com/l0rinc/bitcoin/blob/71911a5912c0778b10308b4e28ff183921f4a275/src/bitcoin-cli.cpp#L1117)
``` C++
if (proxy_networks.find(proxy) == proxy_networks.end()) ordered_proxies.push_back(proxy);
```
=>

``` C++
if (!proxy_networks.contains(proxy)) ordered_proxies.push_back(proxy);
```