💬 ryanofsky commented on pull request "blockstorage: Adjust fastprune limit if block exceeds blockfile size":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1166995654)
In commit "blockstorage: Adjust fastprune limit if block exceeds blockfile size" (e930814fdb9261f180d5c436cefe52e9cf38fd67)

What is the +1 for? Would be helpful to add a code comment saying what the extra byte is.

💬 ryanofsky commented on pull request "blockstorage: Adjust fastprune limit if block exceeds blockfile size":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1167004409)
In commit "blockstorage: Adjust fastprune limit if block exceeds blockfile size" (e930814fdb9261f180d5c436cefe52e9cf38fd67)

Would be good to add to add `assert(nAddSize <= max_blockfile_size);` here to make it clear what this function is assuming and catch the problem instead of going into an infinite loop if outside code changes.

💬 ryanofsky commented on pull request "blockstorage: Adjust fastprune limit if block exceeds blockfile size":
(https://github.com/bitcoin/bitcoin/pull/27191#issuecomment-1508895445)
> I think assert is the appropriate move here to prevent the infinite memory loop!

I think throwing an exception or calling abort() would be ok, but better to reserve the assert macro for conditions which are actually impossible and not use it for error handling, because that would water down the meaning of other asserts.

💬 theuni commented on pull request "depends: fix compiling bdb with clang-16 on aarch64":
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1508919253)
May be interesting: I don't hit any issues when cross building for aarch64-linux-gnu from x86_64:
```
make NO_QT=1 HOST=aarch64-linux-gnu CC="clang --target=aarch64-linux-gnu" CXX='clang++ --target=aarch64-linux-gnu'
```
^^ (Where clang/clang++ are clang16 via PATH) builds fine.

🤔 stickies-v reviewed a pull request: "httpserver, rest: fix segmentation fault on evhttp_uri_get_query"
(https://github.com/bitcoin/bitcoin/pull/27253#pullrequestreview-1385130381)
Left some comments on the bugfix commit. I'd carve that out in a separate PR to get it merged for v25.

Also, suggested rephrasing for the commit message:

```
bugfix: rest: avoid segfault for invalid URI

`evhttp_uri_parse` can return a nullptr, for example when the URI
contains invalid characters (e.g. "%").
`GetQueryParameterFromUri` passes the output of `evhttp_uri_parse`
straight into `evhttp_uri_get_query`, which means that anyone calling
a REST endpoint in which query paramet
...

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1166625610)
`uri` is unhandled user input so including that in the error seems dangerous to me, would just change it to

```cpp
throw std::runtime_error("URI parsing failed, it likely contained RFC 3986 invalid characters);
```

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1167041616)
What's the benefit of re-handling this error? The error message from `GetQueryParameterFromUri` is enough (or could be updated there to reference query param fetching, but it doesn't really have anything to do with the URI being invalid, so I'd prefer leaving it out).

<details>
<summary>git diff</summary>

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index eb1f0bd93..3f0d82497 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -667,13 +667,7 @@ std::optional<
...

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1167045700)
I'd rephrase to the below - any invalid character in the URI will trigger this, not just in the query string.

```
// URI with invalid characters (%) raises a runtime error regardless of which query parameter is queried
```

📝 Riahiamirreza opened a pull request: "Fix typo in developer-notes.md"
(https://github.com/bitcoin/bitcoin/pull/27465)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1167076574)
Or use `SanitizeString(uri)`.

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1167081733)
That wouldn't prevent someone from passing really long URIs, causing e.g. bloated logs. We're already (debug) logging the full URI in `http_request_cb`. Adding it here introduces risks and little value imo.

👍 vasild approved a pull request: "httpserver, rest: fix segmentation fault on evhttp_uri_get_query"
(https://github.com/bitcoin/bitcoin/pull/27253#pullrequestreview-1385842606)
ACK 9b9c48cf81549c7eacfbfb78534b7eaba46ecb34 modulo https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1166625610

I agree with suggestions from @stickies-v, would be happy to re-review.

👍 vasild approved a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411#pullrequestreview-1385887419)
ACK 32b9c328c096adc636571a88f5af292e35fd26e9

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1167110228)
ah yeah "...Appears in a function declaration, enumeration declaration, or class declaration. "

but theres a few leaks in bitcoin...
```
--> git grep nodiscard *.cpp | wc -l
8
```

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1167115865)
Yeah, it's a clearer message. Thanks.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1509002838)
> Left some comments on the bugfix commit. I'd carve that out in a separate PR to get it merged for v25.

I'll create a new PR linked to this.

> Also, suggested rephrasing for the commit message:

Sure, I'll take your suggestion, thanks.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1509003472)
> I agree with suggestions from @stickies-v, would be happy to re-review.

I'll let you both know ones the new PR is ready, very soon. Thanks.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1167123408)
Yeah, the intention was to add the param key on top of the previous error, adding more details at this level but that info is also available from the caller of `GetQueryParameterFromUri`. Ok, I'll leave it out.

📝 jonatack opened a pull request: "p2p: skip netgroup diversity follow-up"
(https://github.com/bitcoin/bitcoin/pull/27467)
In #27374 the role of the `setConnected` data structure in `CConnman::ThreadOpenConnections` changed from the distinct set of outbound peer netgroups to those of outbound IPv4/6 peers only.

In accordance with the changed semantics, this pull updates a code comment regarding feeler connections and proposes to update the naming of `setConnected` to `outbound_ipv46_peer_netgroups_set`.

🤔 jonatack reviewed a pull request: "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns"
(https://github.com/bitcoin/bitcoin/pull/27374#pullrequestreview-1386023175)
Post-merge ACK.

I am not completely sure the additional complexity involved is worth it versus reverting [`72e8ffd` (#27264)](https://github.com/bitcoin/bitcoin/pull/27264/commits/72e8ffd7f8dbf908e65da6d012ede914596737ab), but the change itself here seems fine, modulo a couple of minor loose ends.

> if we run a node only on tor/i2p/cjdns

Via interactions with users, my understanding is that it may possibly be fairly often the case that node operators are running on non-clearnet network
...