💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102613956)
In ccf618bf3b88a95a01463e6a96fc8d9bc52fe57d wallet: Remove ISMINE_USED

While I'm in favour of cleaning up the `ismine` enum and type, I find the consistent passing of `avoid_reuse` bool in all the caller functions quite cognitively loaded; the related if/else checks in the functions of this struct adds more.

I did like `CachableAmount` previously storing the amounts in an array. Keeping the array but with a different enum `CachableAmountType` would get rid of the if/else blocks and `Cachab
...

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102670556)
```diff
- /* Set some defaults for depth, spendable, solvable,
+ /* Set some defaults for depth, solvable,
```

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102737979)
Note: this is okay because this is used only during wallet migration from legacy to descriptor.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102693407)
Would removing this property from the response categorise the diff as breaking change?
Looks a little odd to me to display this as `true` for watch only wallets as well because the wallet doesn't have private keys and thus I feel it should not be a decision maker on this.

💬 theStack commented on pull request "test: fix and augment block tests of invalid_txs":
(https://github.com/bitcoin/bitcoin/pull/32591#issuecomment-2901522359)
Concept ACK, good catch!

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102766865)
Please ignore this suggestion. I had suggested because it felt a bit odd to me to have just 1 item here but leaving `ismine` is actually helpful to give context in the help section.

🤔 vasild reviewed a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2860857095)
Posting review midway. Reviewed up to and including 980a9cd3d38abd0e0da85363056a2be6098d3919 `http: read requests from connected clients`.

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