💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2102323308)
> TODO: Should HTTPStatusCode and HTTPReason be moved since they are not RPC protocols?

I think no because this file already contains generic HTTP constants, e.g. `HTTP_SERVICE_UNAVAILABLE = 503` above.

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2102548479)
Maybe enum is better for this?

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2102677611)
If the body comes in pieces, then this parsing will be repeated for every piece and the `return` on line 460 will be executed a few times until enough data is received. This is suboptimal - to parse the same thing multiple times. I guess it is fine because we are not building a top performance HTTP server.

If this becomes an issue at some point, then it would be better to remember that it has been parsed until e.g. byte 456 and only continue parsing from there as new data is received over the
...

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2102608338)
It is fine as it is. I am just wondering if it is not more appropriate for `LoadHeaders()` to `throw` instead of succeeding because the header `Content-Length: eleven` is already not according to the spec: https://httpwg.org/specs/rfc9110.html#field.content-length:

> [Content-Length](https://httpwg.org/specs/rfc9110.html#field.content-length) = 1*[DIGIT](https://httpwg.org/specs/rfc9110.html#core.rules)

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2102764352)
```suggestion
Mutex requests_mutex;
std::deque<std::unique_ptr<HTTPRequest>> requests GUARDED_BY(requests_mutex);
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2102772840)
Passing `unique_ptr` by value looks a bit strange since it is not supposed to be copied. I guess this works when the caller `std::move`s the object, but isn't it more clear to use `std::unique_ptr<HTTPRequest>&& req`?

💬 hebasto commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#discussion_r2102807235)
No longer relevant.

💬 fanquake commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#discussion_r2102813675)
Why is this test being scoped to only Windows?

💬 i-am-yuvi commented on pull request "BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/32247#issuecomment-2901612231)
Concept ACK

📝 romanz converted_to_draft a pull request: "rest: fetch spent transaction outputs by blockhash"
(https://github.com/bitcoin/bitcoin/pull/32540)
Today, it is possible to fetch a block's spent prevouts in order to build an external index by using the `/rest/block/BLOCKHASH.json` endpoint. However, its performance is low due to JSON serialization overhead.

We can significantly optimize it by adding a new REST endpoint, using a binary response format:

```
$ BLOCKHASH=00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054

$ ab -k -c 1 -n 100 http://localhost:8332/rest/block/$BLOCKHASH.json
Document Length: 1327815
...

💬 danielabrozzoni commented on pull request "Drop log category in `SeedStartup`":
(https://github.com/bitcoin/bitcoin/pull/29480#issuecomment-2901629864)
Hey! I found this PR while looking at the up for grabs, but I noticed that #30750 replaced LogPrint with LogDebug, and currently on master the log line is printed. As far as I understand, this PR shouldn't be needed anymore, and the "Up for grabs" label can be removed :)

```
❯ ./build/bin/bitcoind -debug=rand
2025-05-22T15:16:52Z [rand] Feeding 133269 bytes of environment data into RNG
```

💬 hebasto commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#discussion_r2102823739)
On other platforms, `execl()` might fail only if there is problem with `/bin/sh`. I can't see any trivial way to adjust the test to trigger a failure. Any suggestions?

💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2102866377)
Thanks, fixed in https://github.com/bitcoin/bitcoin/pull/32540/commits/9d7e23e2f505ce6cbe830fc607cf203b1a48ba0d.

💬 davidgumberg commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2102887073)
I think this approach is no less invasive then what is done currently using an external script, setting up an environment that behaves the same as the VS Powershell utility. The advantage of this approach is that it makes use of Visual Studio utilities and commands zero maintenance cost, manually setting variables voids that, and we are subject to being broken by MS upstream, or wrestling with strange misconfigurations when some command that works on a local VS powershell environment is broken o
...

💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2102902795)
> The advantage of this approach is that it makes use of Visual Studio utilities and commands zero maintenance cost, manually setting variables voids that, and we are subject to being broken by MS upstream, or wrestling with strange misconfigurations when some command that works on a local VS powershell environment is broken on CI because some environment variable is not set.

That sounds compelling.

💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#issuecomment-2901770608)
> Not sure we should export a custom binary format for undo data like this.

In my opinion, using a binary format allows us to achieve significantly better performance when building an external index (compared to using `/rest/block/HASH.json`).

I have followed the example of the `/rest/getutxos/` endpoint, which also uses binary encoding when it returns a `std::vector<CCoin> outs`:
https://github.com/bitcoin/bitcoin/blob/d2c9fc84e17120f186a54ef92bab76ea7e8d31b5/src/rest.cpp#L906

💬 romanz commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#issuecomment-2901770885)
> At least there should be a JSON option?

Sounds good, added in https://github.com/bitcoin/bitcoin/pull/32540/commits/9d7e23e2f505ce6cbe830fc607cf203b1a48ba0d.

🤔 jonatack reviewed a pull request: "Replace cluster linearization algorithm with SFL"
(https://github.com/bitcoin/bitcoin/pull/32545#pullrequestreview-2861830018)
Concept ACK

💬 jonatack commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2102935031)
Appreciate the excellent doxygen documentation here.

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2102983324)
> I think the message you posted in the PR description only shows a single error in a single dependency. I am wondering what happens if there is more than one error in different targets, possibly ones where cmake will never get to print the second error.

[Here](https://api.cirrus-ci.com/v1/task/5614699137466368/logs/ci.log) multiple error are reported.