💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#issuecomment-2156393347)
> tACK [8135632](https://github.com/bitcoin/bitcoin/commit/8135632c33f4bad8434403b5807c48d7dba3b3d7)
>
> Still one open comment regarding specifying bitcoincore.org, but happy to reACK if that gets changed, and it's not a blocker for me.

I wrote a better error message that gives a "Did you mean: <closest_match>" when the platform typed isn't in the SHA256SUMS. Avoiding the domain issue and being significantly more helpful. LMK if you like it enough to add to this PR.

💬 maflcko commented on pull request "ci: Native Windows CI job cleanup":
(https://github.com/bitcoin/bitcoin/pull/30242#issuecomment-2156398733)
ACK 0d3ef83433805d3f367130fd5bd227a8ed5a7ccd

💬 maflcko commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1632216319)
> I'm not sure if I'm doing something wrong or if there is indeed a bug where the divergent chain is not rewound. I will continue investigating.

This sounds like a bug. Just to clarify, the active chain is stuck at height `298` and the background chain continues to sync past `399`?

💬 maflcko commented on issue "AssumeUTXO Mainnet Readiness Tracking":
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2156407545)
29996 doesn't seem optional, but possibly a blocker, see https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1632216319

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1632217106)
I tested this myself, and the reason seems to be that absent a `HEAD` request, a 404.html would be downloaded as the targz, which then fails the shasum check.

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156409463)
> 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.

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