💬 hebasto commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1484197246)
Updated 09383b612216b63f2aec0f1fffc889989c66f212 -> 03ec5b6f9ca3af28c9ce25cf2393e28ae852d808 ([pr26642.12](https://github.com/hebasto/bitcoin/commits/pr26642.12) -> [pr26642.13](https://github.com/hebasto/bitcoin/commits/pr26642.13), [diff](https://github.com/hebasto/bitcoin/compare/pr26642.12..pr26642.13)):

- addressed @martinus's [comment](https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1148594367)

💬 hebasto commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1148626538)
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1484197246).

💬 Ayush170-Future commented on issue "ci: Cleanup ci/test/01_base_install.sh":
(https://github.com/bitcoin/bitcoin/issues/27321#issuecomment-1484198989)
I guess SuppressWarnings was not a good practice. It's great that you found a workaround.

Instead of giving package strings as an array, I believe the bash -c technique should be sufficient here. @MarcoFalke, I think you wanted it that way as well.

💬 theuni commented on pull request "depends: fix osx build with clang 16":
(https://github.com/bitcoin/bitcoin/pull/27328#issuecomment-1484213695)
> It fixes building `make -C depends HOST=x86_64-apple-darwin FORCE_USE_SYSTEM_CLANG=1` with clang-16.

Yes, exactly this thanks.

This can be verified very simply outside of depends:
Broken:
> $ /opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang -c test.c -o test.o -Xclang -internal-externc-isystem/tmp
> error: unknown argument: '-internal-externc-isystem/tmp'

Working:
> $ /opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang -c test.c -o test.o -Xclang -intern
...

👍 TheCharlatan approved a pull request: "depends: fix osx build with clang 16"
(https://github.com/bitcoin/bitcoin/pull/27328)
ACK 87afcb0029b8dab933c122fb8f7263c2e7272731

💬 Empact commented on pull request "Torcontrol opt check":
(https://github.com/bitcoin/bitcoin/pull/25136#issuecomment-1484225313)
@vstoyanov No need to ask for permission to contribute. I'd say go for it.

📝 EthanHeilman opened a pull request: "Fixes compile errors in MSVC build #27332"
(https://github.com/bitcoin/bitcoin/pull/27335)
This PR is designed to address the issue https://github.com/bitcoin/bitcoin/issues/27332. The MSVC build is failing because of two bugs in how the build is configured.

The issue
====

When running `msbuild build_msvc\bitcoin.sln -property:Configuration=Release -maxCpuCount -verbosity:minima`l the build fails with following two errors.

* `C:\Users\e0\Documents\GitHub\bitcoin\src\httpserver.cpp(637,9): error C2664: 'void evhttp_connection_get_peer(evhttp_connection *,const char **,uint16_
...

💬 hernanmarino commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1148657332)
Is there a chance that this is not the case in any situation ?

💬 hernanmarino commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1148659470)
Aren't there other scripts relying on the exported aliases ? Won't they fail ?

👍 hernanmarino approved a pull request: "build, qt: Fix handling of `CXX=clang++` when building `qt` package"
(https://github.com/bitcoin/bitcoin/pull/27314)
tested ACK

👍 hernanmarino approved a pull request: "log: Check that the timestamp string is non-empty to avoid undefined behavior"
(https://github.com/bitcoin/bitcoin/pull/27317)

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1148665626)
I don't know. I would prefer a solution more in line with how this was solved in automake builds but I don't have time to learn enough about msbuild to implement it and it is currently broken in the default case.

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1148666703)
In fact the build in CI is now failing because it expects the other function signature.

`C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\httpserver.cpp(635,56): error C2664: 'void evhttp_connection_get_peer(evhttp_connection *,char **,uint16_t *)': cannot convert argument 2 from 'const char **' to 'char **' [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]`

👍 hernanmarino approved a pull request: "depends: qrencode 4.1.1"
(https://github.com/bitcoin/bitcoin/pull/27312)
tested ACK, both compiling and QR generation .

👍 hernanmarino approved a pull request: "wallet: finish addressbook encapsulation"
(https://github.com/bitcoin/bitcoin/pull/26836)
tested ACK, both unit and functional tests.

⚠️ apoelstra opened an issue: "Should be able to import an xpub descriptor to a privkey-enabled wallet if the wallet has the privkeys"
(https://github.com/bitcoin/bitcoin/issues/27336)
### Please describe the feature you'd like to see added.

If you have a privkey-enabled wallet, you should be able to import a public descriptor where your wallet knows the private keys corresponding to (some of) the xpubs in the descriptor.

### Is your feature related to a problem, if so please describe it.

If you try to import a descriptor into a Core wallet that has private keys, and you only provide the public descriptor, it will fail with the error `Cannot import descriptor without privat
...

💬 apoelstra commented on issue "Should be able to import an xpub descriptor to a privkey-enabled wallet if the wallet has the privkeys":
(https://github.com/bitcoin/bitcoin/issues/27336#issuecomment-1484283918)
It looks like the wallet actually does the right thing already. The only fix needed is to delete the lines https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/backup.cpp#L1539-L1541 which cause the `importdescriptors` RPC to fail unnecessarily.

📝 apoelstra opened a pull request: "wallet: allow importing descriptors that have no xprivs, even in a privkey-enabled wallet"
(https://github.com/bitcoin/bitcoin/pull/27337)
Importing pubkey-only descriptors works fine as long as the wallet already has the required privkeys.

Happy to add whatever tests people advise me to.

Fixes #27336

📝 apoelstra converted_to_draft a pull request: "wallet: allow importing descriptors that have no xprivs, even in a privkey-enabled wallet"
(https://github.com/bitcoin/bitcoin/pull/27337)
Importing pubkey-only descriptors works fine as long as the wallet already has the required privkeys.

Happy to add whatever tests people advise me to.

Fixes #27336

**Edit** Never mind -- we can update balances but not sign coins. More work needs to be done to support this.

💬 pablomartin4btc commented on pull request "wallet: finish addressbook encapsulation":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1148773136)
nit:
```suggestion
bool require_is_mine{record.purpose == "receive" && !IsMine(dest)};
bool copied{false};
```