💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139066512)
Same here:

```suggestion
return serv.value_or(CService{});
```

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139153162)
The `i->first.empty()` is missing in the new code. In this case the old code would have printed the warning, but the new code will not because `LookupHost("", false)` will return an optional without value.

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139049268)
The previous code would have returned a default constructed `CNetAddr` if lookup failed. The new code will throw `std::bad_optional_access` because it will call `addr.value()` when the optional has no value. I guess this will fix it:

```cpp
return addr.value_or(CNetAddr{});
```

`BOOST_CHECK_MESSAGE()` will continue execution if the expression is `false`. `BOOST_REQUIRE_MESSAGE()` will stop the execution of the test.

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139167447)
This can be shorter:

```cpp
BOOST_CHECK(LookupHost("127.0.0.1"s, false).has_value());
BOOST_CHECK(!LookupHost("127.0.0.1\0"s, false).has_value());
BOOST_CHECK(!LookupHost("127.0.0.1\0example.com"s, false).has_value());
BOOST_CHECK(!LookupHost("127.0.0.1\0example.com\0"s, false).has_value());
```

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139172258)
There was a bug in the old code - if lookup failed it would have returned `true` (i.e. pretend that it is IPv6). The new code fixes this, nice!

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139129291)
```suggestion
const std::optional<CService> serv{Lookup("252.1.1.1", 7777, false)};
BOOST_CHECK(serv.has_value());
CAddress addr{serv.value_or(CService{}), NODE_NONE};
std::optional<CNetAddr> resolved{LookupHost("252.2.2.2", false)};
BOOST_CHECK(resolved.has_value());
AddrInfo info{addr, resolved.value_or(CNetAddr{})};
```

or replace `BOOST_CHECK()` with `BOOST_REQUIRE()`. Maybe that was the original intention of this? Looks to me that a successful resolve is a
...

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139178852)
This is removed in its own commit
```
p2p: remove duplicated `ContainsNoNUL` calls in `Lookup`
```
but I guess that removal can be squashed into one of the preceding commits:
```
p2p, refactor: return vector/optional<CService> in `Lookup`
```
like the same check is removed from `LookupHost()` in
```
p2p, refactor: return `std::optional<CNetAddr>` in `LookupHost`
```

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139143140)
nit
```suggestion
if (!m_uri_parsed) return std::nullopt;
```

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139180604)
See https://github.com/bitcoin/bitcoin/pull/24012 - I couldn't get mocking an `evhttp_request*` past the ASan test. That's why in #24098 I introduced the `GetQueryParameterFromUri()` helper function: it could easily be unit tested, and then `HTTPRequest::GetQueryParameter()` was just a simple wrapper left untested.

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139181920)
Seems like unnecessary churn imo, and redefining HTTP status codes in a single functional test doesn't seem like a productive approach. Would leave this out?

💬 mhmdawnallah commented on issue "test: use-of-uninitialized-value in sqlite3Strlen30":
(https://github.com/bitcoin/bitcoin/issues/27222#issuecomment-1472533007)
Hi @MarcoFalke, I'd like to work on this issue. Could you please let me know where I should start ?

💬 achow101 commented on pull request "test: psbt: check non-witness UTXO removal for segwit v1 input":
(https://github.com/bitcoin/bitcoin/pull/27200#issuecomment-1472543537)
ACK 3dd2f6461b4bb28b2b212c691a3df28ac793ad91

👍 pablomartin4btc approved a pull request: "Wallet : Allow user to navigate options while encrypting at creation"
(https://github.com/bitcoin-core/gui/pull/722)
Tested ACK.
It works as expected according to the description of the PR and it fulfils the requirements of the related issue.
Just as a reminder for other reviewers, if you want to [verify if the wallet was encrypted](https://bitcoin.stackexchange.com/questions/101664/why-is-bitcoin-core-not-asking-me-to-enter-the-decryption-key-for-my-allegedly) or [not](https://bitcoin.stackexchange.com/questions/101664/why-is-bitcoin-core-not-asking-me-to-enter-the-decryption-key-for-my-allegedly), when a u
...

🚀 achow101 merged a pull request: "test: psbt: check non-witness UTXO removal for segwit v1 input"
(https://github.com/bitcoin/bitcoin/pull/27200)

💬 MarcoFalke commented on issue "test: use-of-uninitialized-value in sqlite3Strlen30":
(https://github.com/bitcoin/bitcoin/issues/27222#issuecomment-1472581735)
I am not sure. This was an intermittent issue, so it may be:

* A bug/race in our code
* A bug/race in sqlite
* A bug/race in msan
* A bug/race in the CI env
* A bug/race somewhere else
* A cosmic ray

📝 instagibbs opened a pull request: "Fix fund transaction case at 0-value, 0-fee"
(https://github.com/bitcoin/bitcoin/pull/27271)
and when no inputs are pre-selected.

triggered via:

walletcreatefundedpsbt '[]' '[{"data": "deadbeef"}]' 0 '{"fee_rate": "0"}'

💬 0xB10C commented on pull request "doc: fix typo in interface_usdt_utxocache.py":
(https://github.com/bitcoin/bitcoin/pull/27268#issuecomment-1472641877)
ACK

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139308077)
thanks, corrected other similar 2.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1139309346)
I'm now reusing the HTTPStatus enum from the python http module in python; there were like 20 places where these codes were introduced as int. I think it makes it clearer but pls let me know if that's not the case.

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1472679470)
@theStack: Thanks for the suggestion. I tried implementing this and found that it does save the six lines here, but it also required special casing two places downstream in my follow-up PR. While the fixes were simple, I feel that it makes the interface slightly worse for the downstream consumer, so I’m ambivalent on the change.