💬 fanquake commented on pull request "depends: fix compiling bdb with clang-16 on aarch64":
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1508818821)
Guix Build:
```bash
ed1f83a77d00:/bitcoin# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
05fb39e87c26ce80c12c9a1e054eb5b558a454120c19cc555d7ab1d6508c0749 guix-build-0cc90822d502/output/aarch64-linux-gnu/SHA256SUMS.part
c488ff0d8eb0e1ec605f5ca6836b3dba4ab468f2f51ad2356ec9a632f6aee4c5 guix-build-0cc90822d502/output/aarch64-linux-gnu/bitcoin-0cc90822d502-aarch64-linux-gnu-debug.tar.gz
32c33bdb964d7eb3fe16f30f00b5eb3582e6
...

👍 real-or-random approved a pull request: "Update src/secp256k1 subtree to release v0.3.1"
(https://github.com/bitcoin/bitcoin/pull/27445#pullrequestreview-1385704456)
utACK 621c17869d3754559c03e4f2bee73885659e0c68 subtree matches. diff to linter script looks good

💬 fanquake commented on pull request "Update src/secp256k1 subtree to release v0.3.1":
(https://github.com/bitcoin/bitcoin/pull/27445#issuecomment-1508834870)
cc @jonasnick too

👍 ryanofsky approved a pull request: "blockstorage: Adjust fastprune limit if block exceeds blockfile size"
(https://github.com/bitcoin/bitcoin/pull/27191#pullrequestreview-1385705001)
Code review ACK e930814fdb9261f180d5c436cefe52e9cf38fd67, but I think it would be great to add the test written by @pinheadmz https://github.com/bitcoin/bitcoin/pull/27191#pullrequestreview-1353171052 because the blockmanager code is fragile and a regression could happen, and because the test setup is very clean so it could probably be used to branch out and test other things.

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