💬 maflcko commented on pull request "doc: Add missing top-level description to pruneblockchain RPC":
(https://github.com/bitcoin/bitcoin/pull/32333#discussion_r2091138637)
```suggestion
"Attempts to delete block and undo data up to a specified height or timestamp, if eligible for pruning.\n"
```

nit: No need to start with a newline. It will be trimmed anyway.

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2883841466)
Thank you for the reviews @stringintech and @stickies-v!

16a695fbff4a92eba3eb72ec6aa766e71c52f773 -> e8ac6d29273c48328f1f77886c3c3f653e6cc58e ([spendblock_1](https://github.com/TheCharlatan/bitcoin/tree/spendblock_1) -> [spendblock_2](https://github.com/TheCharlatan/bitcoin/tree/spendblock_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/spendblock_1..spendblock_2))

* Addressed @stickies-v's comments [1](https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2065965592), [2
...

💬 NicolaLS commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2883845232)
Squashed and reworded commits.

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2091194407)
We could also use `AccessCoin`, but I think accessing it directly is a bit more efficient, because we don't allocate another vector. Do you have a better suggestion for the naming there? It is all a bit confusing.

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2091195129)
Thanks, added the the concept.

📝 m3dwards opened a pull request: "ci: remove 3rd party js from windows dll gha job"
(https://github.com/bitcoin/bitcoin/pull/32513)
We only need to find MSBuild.exe for the job to function and can do that ourselves using vswhere.exe and so can remove the dependency on ilammy/msvc-dev-cmd.

Fixes: #32508

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2091196979)
Dropped it again, but I also changed things slightly to get the block hash from the block index now.

💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2883865539)
Concept ACK.

Thank you for taking this!

💬 nervana21 commented on pull request "doc: Add missing top-level description to pruneblockchain RPC":
(https://github.com/bitcoin/bitcoin/pull/32333#discussion_r2091213444)
Updated, thanks!

💬 maflcko commented on pull request "doc: Add missing top-level description to pruneblockchain RPC":
(https://github.com/bitcoin/bitcoin/pull/32333#issuecomment-2883881160)
lgtm ACK 135a0f0aa711b95c50aa4cbe0c38d82d647f1c8b

📝 maflcko opened a pull request: " scripted-diff: Remove unused leading newline in RPC docs "
(https://github.com/bitcoin/bitcoin/pull/32514)
It is harmless, but newlines in the beginning read a bit odd ("nReturns"). So just require them to not be present.

The diff is large, but a trivial scripted-diff.

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090527952)
Shouldn't this be `assert False`? Here the expectation is that the `recv()` will throw an exception due to timeout.

https://docs.python.org/3/library/socket.html#socket.socket.recv
> A returned empty bytes object indicates that the client has disconnected.

An "empty bytes object" will not trigger the assert `assert not res` but if that happens (= disconnect) then the test should fail.

suggestion:
```diff
- assert not res
+ assert False
`

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090840593)
This will also pass if no exception is thrown. Either add `assert False` after line 214 or have a boolean variable to false before the `try` and set it to true inside `except` and assert that it is true afterwards.

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090912262)
Is the intention here to send 8MB of data?

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090917556)
Here and elsewhere in the added tests, `assert_equal()` produces a better error message:

`assert` (value of `out1` is not printed):
```
assert out1 == b'{"result":"high-hash","error":null}\n'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
```

vs

`assert_equal()`:
```
AssertionError: not(b'{"result":null,"error":{"code":-32700,"message":"Parse error"},"id":null}\n' == b'{"result":"high-hash","error":null}\n')
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2091093913)
This would not recognize numbers larger than `FFFFFFFF` (4 bytes, 8 chars). That should be `if (str.size() > 16) return false;` instead of `8`.

But better not roll our own and use `std::from_chars()` like the others:

<details>
<summary>[patch] Use ParseIntegral() which uses std::from_chars(), like the other Parse*() functions</summary>

```diff
diff --git i/src/util/strencodings.cpp w/src/util/strencodings.cpp
index d923bdd121..05a5dced62 100644
--- i/src/util/strencodings.cpp
+++ w
...

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2091083332)
Add some more test cases and avoid `BOOST_CHECK(A && B)` because when that fails it is unclear which one of `A` or `B` was false. Prefer `BOOST_CHECK(A); BOOST_CHECK(B);`. Also, because `B` is `n == 123`, do `BOOST_CHECK_EQUAL(n, 123)`:

```cpp
BOOST_AUTO_TEST_CASE(test_ParseUInt64Hex)
{
uint64_t n;
// Valid values
BOOST_CHECK(ParseUInt64Hex("1234", nullptr));
BOOST_CHECK(ParseUInt64Hex("1234", &n));
BOOST_CHECK_EQUAL(n, 0x1234);
BOOST_CHECK(ParseUInt64Hex("a",
...

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090504818)
1 second timeout to send or receive seems more than enough for local testing on a dev machine. However, CI virtual machines sometimes are surprisingly slow. To avoid unnecessary test failures maybe it would be better to have this be 5 or 10 seconds for the `sendall()` calls and then set to 1 for the `recv()` call which we expect to timeout.

🤔 vasild reviewed a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2842608359)
Posting review midway. Reviewed up to and including 144a777f86 `string: add CaseInsensitiveComparator`. I like that the PR starts with some new tests to enforce correct behavior.

💬 NicolaLS commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2883956445)
(fixed typo in commit message)