📝 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};
```

💬 pablomartin4btc commented on pull request "wallet: finish addressbook encapsulation":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1148773578)
```suggestion
const std::string& dest{EncodeDestination(address)};
```

💬 martinus commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1484531112)
ACK 03ec5b6f9ca3af28c9ce25cf2393e28ae852d808

💬 TheCharlatan commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1484550109)
re-ACK [03ec5b6](https://github.com/bitcoin/bitcoin/pull/26642/commits/03ec5b6f9ca3af28c9ce25cf2393e28ae852d808)

💬 vstoyanov commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1148824467)
Yes, but the next time they are used in `ci/test/04_install.sh`, they are already redefined on lines 79-84:
`CI_EXEC () {
$CI_EXEC_CMD_PREFIX bash -c "export PATH=${BINS_SCRATCH_DIR}:\$PATH && cd \"$P_CI_DIR\" && $*"
}
CI_EXEC_ROOT () {
$CI_EXEC_CMD_PREFIX_ROOT bash -c "export PATH=${BINS_SCRATCH_DIR}:\$PATH && cd \"$P_CI_DIR\" && $*"
}`

This PR only relates to their usage in base setup.

💬 MarcoFalke commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1148898438)
I am thinking that the second user account can be removed completely. I think it is fine to run all tests as root. The reason for https://github.com/bitcoin/bitcoin/pull/25900 (`./depends/` being owned by root) no longer applies as `./depends/` is a docker/podman volume.
However, this is probably for a follow-up.

💬 MarcoFalke commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#issuecomment-1484646255)
lgtm ACK f52d482408af68458f0b5c2775b8a1dffc380466