💬 hebasto commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1149059179)
```suggestion
/* Assume that an MSVC build will get dependencies from vcpkg and thus use the latest version of libevent */
```

👍 hebasto approved a pull request: "Fix segfault when shutdown during wallet open"
(https://github.com/bitcoin-core/gui/pull/693)
ACK 9a1d73fdffa4f35e33bc187ac9b3afbba28b1950, I have reviewed the code and it looks OK.

💬 hebasto commented on issue "Segmentation fault when closing while loading wallets":
(https://github.com/bitcoin-core/gui/issues/689#issuecomment-1484922884)
@MarcoFalke

Can you confirm that https://github.com/bitcoin-core/gui/pull/693 fixes this issue for you?

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1149028723)
nit:
```suggestion
const std::string strURI{hreq->GetURI()};
```

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1149108682)
It it possible to enforce that this argument will never be null - if changed to reference. Simplifies the code a bit:

```diff
diff --git i/src/httpserver.cpp w/src/httpserver.cpp
index e9b68a36c1..1dccb196be 100644
--- i/src/httpserver.cpp
+++ w/src/httpserver.cpp
@@ -683,20 +683,18 @@ HTTPRequest::RequestMethod HTTPRequest::GetRequestMethod() const
return UNKNOWN;
}
}

std::optional<std::string> HTTPRequest::GetQueryParameter(const std::string& key) const
{
-
...

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1149034300)
```suggestion
/** Get requested URI. Will always return non-nullptr.
```

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1149110958)
`GetURI()` (which calls `evhttp_request_get_uri()`) could still return null. It seems too indirect to that we rely that we have called `evhttp_request_get_uri()` before on the same input and back then it returned non-null (from the constructor) so it must also return non-null now.

I think it would be better to save the result in a variable and document that it will never be null. This will also avoid calling `evhttp_request_get_uri()` two times. Like this:

```diff
diff --git i/src/httpser
...

🚀 fanquake merged a pull request: "depends: qrencode 4.1.1"
(https://github.com/bitcoin/bitcoin/pull/27312)

💬 hebasto commented on pull request "Wallet : Allow user to navigate options while encrypting at creation":
(https://github.com/bitcoin-core/gui/pull/722#discussion_r1149162701)
Could we avoid changes in translatable strings in this PR?

💬 hebasto commented on pull request "Wallet : Allow user to navigate options while encrypting at creation":
(https://github.com/bitcoin-core/gui/pull/722#discussion_r1149164776)
I'm not sure which label, "Go back" or just "Back", is more expected in user experience.

ping @GBKS

💬 hebasto commented on issue "Add go back button to `Confirm wallet encryption` window, add cancel button to `wallet to be encrypted` window":
(https://github.com/bitcoin-core/gui/issues/394#issuecomment-1484969506)
@jarolrod

Mind reviewing https://github.com/bitcoin-core/gui/pull/722?

✅ fanquake closed a pull request: "Torcontrol opt check"
(https://github.com/bitcoin/bitcoin/pull/25136)

💬 fanquake commented on issue "depends does not compile with clang-16 (fontconfig)":
(https://github.com/bitcoin/bitcoin/issues/27299#issuecomment-1484978452)
> Same for qrencode:
> The qrencode issue also with clang-15:

This portion of this issue should have been addressed by #27312.

🚀 fanquake merged a pull request: "depends: fix osx build with clang 16"
(https://github.com/bitcoin/bitcoin/pull/27328)

💬 prusnak commented on pull request "depends: qrencode 4.1.1":
(https://github.com/bitcoin/bitcoin/pull/27312#issuecomment-1484985732)
There is this library which is maintained and available in multiple programming languages (C and C++ are two of them).

💬 MarcoFalke commented on pull request "ci: Use Cirrus CI dockerfile env":
(https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1484988445)
No idea why CI fails. Seems to be related to https://superuser.com/questions/1642858/git-on-debian-10-backports-throws-fatal-unable-to-access-https-github-com-us

💬 hebasto commented on pull request "Switch RPCConsole wallet selection to the one most recently opened/restored/created":
(https://github.com/bitcoin-core/gui/pull/696#discussion_r1149178367)
1. Should it be declared as a slot?

2. In the `qt` subdirectory we usually follow Qt's naming convention. Mind renaming please
```suggestion
void setCurrentWallet(WalletModel* const wallet_model);
```
?

💬 hebasto commented on pull request "Switch RPCConsole wallet selection to the one most recently opened/restored/created":
(https://github.com/bitcoin-core/gui/pull/696#discussion_r1149184463)
Why `const`? The parameter of all connected signals is `WalletModel* wallet_model`, right?

💬 GBKS commented on pull request "Wallet : Allow user to navigate options while encrypting at creation":
(https://github.com/bitcoin-core/gui/pull/722#discussion_r1149186485)
I think both are fine. Personally, I would go for "Back", as the word "Go" doesn't add any additional information.

💬 josibake commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1149193825)
This line was really confusing for me at first. Perhaps something like this could make it more clear?

```suggestion
Dbt prefix_key(prefix.data(), prefix.size()), prefix_value{};
```