💬 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

👍 MarcoFalke approved a pull request: "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)"
(https://github.com/bitcoin/bitcoin/pull/27333)
left a question/nit, feel free to ignore

💬 MarcoFalke commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1148907877)
```suggestion
${CI_RETRY_EXE} dnf -y --allowerasing install $CI_BASE_PACKAGES $PACKAGES
```

I guess I know nothing about bash, but putting it this way may also work at no downside?

💬 MarcoFalke commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1484668108)
> FUZZ=process_message ./src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/http_request/75d231d5e022cc38377bbbf4549058d37b7c59a3

You'll need to set the correct fuzz target:


`FUZZ=http_request ./src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/http_request/75d231d5e022cc38377bbbf4549058d37b7c59a3`

💬 MarcoFalke commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1148926296)
If there are steps to reproduce this, it would be good to write a detection for it at Bitcoin Core init time and refuse to start the program (if this isn't already done).

📝 MarcoFalke opened a pull request: "ci: Use Cirrus CI dockerfile env"
(https://github.com/bitcoin/bitcoin/pull/27340)
Currently the CI env has many intermittent issues:

* The Ubuntu package servers are frequently down
* Occasionally other stuff is down, such as dnf, pip, or the android sdk
* Installing packages is slower than downloading them, at least on Cirrus, which has a fast download speed

Fix all issues by using the Cirrus CI dockerfile env.

📝 MarcoFalke converted_to_draft a pull request: "ci: Use Cirrus CI dockerfile env"
(https://github.com/bitcoin/bitcoin/pull/27340)
Currently the CI env has many intermittent issues:

* The Ubuntu package servers are frequently down
* Occasionally other stuff is down, such as dnf, pip, or the android sdk
* Installing packages is slower than downloading them, at least on Cirrus, which has a fast download speed

Fix all issues by using the Cirrus CI dockerfile env.