📝 dergoegge opened a pull request: "refactor, net processing: Avoid CNode::m_relays_txs usage"
(https://github.com/bitcoin/bitcoin/pull/27270)
`CNode::m_relays_txs` is meant to only be used for the eviction logic in `net`. `TxRelay::m_relay_txs` will hold the same value and is meant to be used on the application layer to determine if we will/should relay transactions to a peer.

(Shameless plug: we should really better specify the interface for updating eviction data to avoid refactors like this in the future -> #25572)

💬 dergoegge commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1139046536)
```suggestion
if (m_txreconciliation) m_txreconciliation->TryRemovingFromSet(pfrom.GetId(), wtxid);
```

You are already checking internally that the peer is registered.

💬 dergoegge commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1139078818)
```suggestion
{
LOCK(m_peer_mutex);
for (auto [id, peer] : m_peer_map) {
const auto state{State(id)};
if (!state) continue;

if (auto tx_relay = peer->GetTxRelay()) {
LOCK(tx_relay->m_bloom_filter_mutex);

...

📝 hernanmarino opened a pull request: "Wallet : Allow user to navigate options while encrypting at creation"
(https://github.com/bitcoin-core/gui/pull/722)
This fixes https://github.com/bitcoin-core/gui/issues/394
It adds a "Go back" button to th "Confirm wallet encryption" window, allowing the users to change the password if they want to. It also adds a Cancel button to "Wallet to be encrypted window"
Prior to this change users had no option to alter the password, and were force to either go ahead with wallet creation or cancel the whole process. Also, at the final window, they were shown a Warining but with no option to cancel.
The new workf
...

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1139144192)
> bug fixes should be separate from refactoring commits

Commits have been reorganized.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1472457091)
Updated changes:
- move the validation/parsing logic up in the stack as soon as the request is initiated even before the work item thread is created
- updated unit and functional tests

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

```suggestion
const std::optional<CNetAddr> addr{LookupHost(str_addr, /*fAllowLookup=*/false)};
```

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139037448)
Hmm, `std::optional` without the template parameter? I guess the compiler is smart enough to deduct it, but in other places you used `std::optional<CNetAddr>`, better add the template parameter here for consistency. Or use `const auto addr = ...` eveywhere?

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139005096)
This change in the comment about `Lookup()` was done in commit
```
5a656f60fb p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost`
```
but it belongs to the next commit
```
786ad94614 p2p, refactor: return vector/optional<CService> in `Lookup`
```

💬 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 ?