💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1632218405)
Does this load the full response into memory, which curl does not do? (Probably fine, but if the approach is extended to debug symbols, which are 0.5GB, this may become an issue)

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156411645)
Also tend toward NACK here, as there are several issues here from a quick glance:

* This is more lines of code, so possibly more to maintain.
* This worsens the UX, due to lack of progress.
* Review and testing this change may not be worth it, considering the limited benefit, if any?

💬 iw4p commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156419616)
> > Moreover, the optimization to reduce two requests into one significantly lowers the traffic load on the host,
>
> This isn't true? There are still two requests before and after this change, so this isn't changed. Also a `HEAD` request shouldn't cause any meaningful traffic, compared to a full download.

Twice I asked if you think it's fine, we can use one request to check the status code (if it's 404) or download (if status code isn't 404) and I got no answer. Right now the code send tw
...

⚠️ maflcko opened an issue: "fuzz: crypter: Abrt in __cxxabiv1::failed_throw"
(https://github.com/bitcoin/bitcoin/issues/30251)
From https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69500

In:
* https://github.com/bitcoin/bitcoin/blob/4a020ca443ba370bf41583962d16aa8551876f53/src/wallet/test/fuzz/crypter.cpp#L65
* https://github.com/bitcoin/bitcoin/blob/4a020ca443ba370bf41583962d16aa8551876f53/src/wallet/crypter.cpp#L98

I haven't looked into this in detail, but it may be allocating a large chunk of memory. Given that encryption is only used for small chunks of memory (private keys), I wonder if it makes sense
...

💬 maflcko commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1632225431)
https://github.com/bitcoin/bitcoin/issues/30251

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156434898)
> Twice I asked if you think it's fine [...] and I got no answer.

As the author of a pull request, there is an expectation that you spend time to review and to test the code you have written yourself. I try to ask questions during review to motivate pull request authors to look into interesting areas to explore in the change. There are currently more than 300 open pull request and I currently don't have the time to test everything I can imagine in all of them. In this case, it is not really t
...

📝 maflcko opened a pull request: "test: Remove redundant verack check"
(https://github.com/bitcoin/bitcoin/pull/30252)
Currently the sync in `connect_nodes` mentions the `version` and `verack` message types, but only checks the `verack`. Neither check is required, as the `pong` check implies both. In case of failure, the debug log will have to be consulted anyway, so the redundant check doesn't add value.

Also clarify in the comments that the goal is to check the flag `fSuccessfullyConnected` indirectly.

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1632236142)
Done in https://github.com/bitcoin/bitcoin/pull/30252, also mentioning `fSuccessfullyConnected`. Feel free to NACK/ACK/nit.

📝 maflcko opened a pull request: "refactor: performance-for-range-copy in psbt.h"
(https://github.com/bitcoin/bitcoin/pull/30253)
A copy of the partial signatures is not required before serializing them.

For reference, this was found by switching the codebase to `std::span` (not sure why it wasn't found with `Span` though):

```
./psbt.h:240:23: error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
240 | for (auto sig_pair : partial_sigs) {
| ^
| const &
...

💬 maflcko commented on pull request "refactor: performance-for-range-copy in psbt.h":
(https://github.com/bitcoin/bitcoin/pull/30253#issuecomment-2156448257)
Reference CI run: https://github.com/bitcoin/bitcoin/runs/25847037350

📝 theStack opened a pull request: "test: doc: fix units in tx-size standardness test (s/vbytes/weight units)"
(https://github.com/bitcoin/bitcoin/pull/30254)
This small fixup PR is a late follow-up for #17947 (commit 4537ba5f21ad8afb705325cd8e15dd43877eb28f), where the wrong units has been used in the comments for the large tx composition.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632256491)
This is done in commit e5658e7a40f03b0b07b1f83f22a9d834bc8fd5c6 which only improves the code organization. Specifically here the reason for moving is to have all the non-exposed functions in `random.cpp` within one anonymous `namespace` at the start of the file, and all public one below.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632268413)
I see how this is confusing; `MixExtract` is indeed a deterministic function, and arguably, all of `RNGState` is deterministic when considered in isolation.

The "determinism" referred here is the introduction of a new deterministic mode for the entire RNG (module-wide, including seeding/initializations/hardware, ...). In order to do that, a separate RNG is embedded within `RNGState` which is only ever explicitly initialized, and if it is, `MixExtract` can be asked to draw its output from ther
...

👍 tdb3 approved a pull request: "test: doc: fix units in tx-size standardness test (s/vbytes/weight units)"
(https://github.com/bitcoin/bitcoin/pull/30254#pullrequestreview-2106188976)
cr ACK
Normally I would think this change could be included in a future change to `test_IsStandard`, but there is value in preventing confusion and not hoping that a future reviewer/author remembers to include this change.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632290445)
Hmm. The intent was that this is supported behavior; `GetStrongRandBytes()` never sets `allow_deterministic`, and thus in deterministic mode would still return from the normal fully-seeded RNG (because I don't want to introduce anything the code path of `GetStrongRandBytes` that could return anything but high quality randomness). This means that if `GetStrongRandBytes` is called anywhere in test code that enables deterministic randomness, the condition you state occurs.

It's probably possible
...

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632292752)
Yes, because `m_cache_entry_expiration` is expressed in microseconds anyway; no need to reduce accuracy. I've moved the behavior change to a separate commit.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632293002)
I think I was inspired by Python's `random.expovariate`, but I agree "exp" is much more naturally associated with "exponential". Done.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632293085)
Done (I left the "instead" in place).

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632293173)
Nice catch. Done, and added a comment to explain.

💬 Gary19751957 commented on pull request "refactor: performance-for-range-copy in psbt.h":
(https://github.com/bitcoin/bitcoin/pull/30253#discussion_r1632294059)
```suggestion
for (const auto& sig_pair : partial_sigs) {
```