💬 sipa commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220914013)
I don't think there are *any* `std::shuffle` in the fuzz tests, though?

It may make sense to add something like the old `Shuffle` back if we do need it, though.

💬 maflcko commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220931147)
> I don't think there are _any_ `std::shuffle` in the fuzz tests, though?

I hope all code touched in this pull request is covered by fuzz tests. If not, then that should be fixed ;)

For example, `joinpsbts` is an RPC (not in the fuzz dir), but it is fuzzed, and it calls `std::shuffle`: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/rpc/rawtransaction.cpp.gcov.html#L1793

I don't have a strong feeling either way, because if an effort is made to have uniform fuzz behavior across compi
...

📝 fanquake opened a pull request: "contrib: simplify `test-security-check`"
(https://github.com/bitcoin/bitcoin/pull/30423)
The current `test-security-check` script is hard to understand, and change (i.e https://github.com/bitcoin/bitcoin/pull/29987/files#diff-52aa0cda44721f089e53b128cb1232a876006ef257b211655456b17dfb2ec712); tests are also not done in isolation (when-possible). Fix that, and add missing checks. Simplifies future toolchain/security/hardening changes.

👍 hebasto approved a pull request: "util: Catch translation string errors at compile time"
(https://github.com/bitcoin/bitcoin/pull/30383#pullrequestreview-2169704590)
ACK fa601ab9f7142cdb18c18c1128fc893cdffb3463.

nit: The error messages for the cases `_(0);` and `_(NULL);` can be confusing a bit. For instance, clang-18 reports "conversion from 'long' to 'ConstevalStringLiteral' is ambiguous" for the latter.

💬 brunoerg commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2220979817)
Concept ACK

💬 hebasto commented on pull request "contrib: simplify `test-security-check`":
(https://github.com/bitcoin/bitcoin/pull/30423#issuecomment-2220993948)
> tests are also not done in isolation (when-possible).

Mind elaborating this statement please? I mean, `contrib/devtools/test-security-check.py` is supposed to run in the Guix environment. So what kind of "isolation" do you mean?

💬 cobratbq commented on pull request "doc: external-signer, example 'createwallet' RPC call requires eight argument":
(https://github.com/bitcoin/bitcoin/pull/30354#issuecomment-2221003676)
> Highly likely it was always broken. I'd be surprised if a PR that added a positional argument in the middle would make it through code review.

Okay, that's clear. Well we can fix it now, and add the named arguments.

> > The docs on descriptors (`descriptors.md`) also show examples, such as `pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8)`, and I have no idea whether this is supposed to be supported or not.
>
> That su
...

💬 maflcko commented on pull request "util: Catch translation string errors at compile time":
(https://github.com/bitcoin/bitcoin/pull/30383#issuecomment-2221012027)
> nit: The error messages for the cases `_(0);` and `_(NULL);` can be confusing a bit. For instance, clang-18 reports "conversion from 'long' to 'ConstevalStringLiteral' is ambiguous" for the latter.

Unrelated to this codebase, but I'd say this should be fixed upstream by doing `#define NULL nullptr` (under C++11 or later) instead of using an integer literal.

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672669253)
> variation of it should actually probably go right here

Maybe, but I would still be surprised, regardless of the context, if `not X` leads to `X`. Could the parameter explain it better than "erase", so that the reviewer isn't surprised that it means "keep"?

> invalidate your ACK

I expect that I will have to review and ack your changes many more times before it's merged, don't worry about it!

👍 itornaza approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2169749598)
tested ACK bdc2a656c2d2a61d226fde1d1fd4e79664106e18

Built from source and run all unit, functional and extended tests and all of them pass. Looked through the code changes and they look great to me. I particularly like the default definition of the move constructors in `KeyPair` since there is a pointer member `m_keydata` in that class, plus the fact that the generation of the copy constructors that are not needed anywhere is suppressed due to the use of the default keyword which I consider a
...

💬 itornaza commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1672656999)
The same header is already and very thoughtfully provided for the following function a few lines above

`KeyPair ComputeKeyPair(const uint256* merkle_root) const;`

Maybe here a shorter header would be more appropriate to avoid repeating ourselves and make maintenance easier to handle (at the comment level), like so

```
/** KeyPair
*
* Wrapper class for the `secp256k1_keypair` type */
*/ ```

🤔 ismaelsadeeq reviewed a pull request: "test: fix inconsistency in fundrawtransaction weight limits test"
(https://github.com/bitcoin/bitcoin/pull/30353#pullrequestreview-2169806132)


Concept ACK

I could not recreate [this failure](https://cirrus-ci.com/task/5319226913718272?logs=ci#L1923) locally. Looking closely at the code, 4f571fb73526b55a83af3c04fbc662ccdd1307ae might not solve the issue.
For us to load coins from `AutomaticCoinSelection` and reach the failure in the C.I.
`selection_target` in `SelectCoins` must be greater than 0.


But when `selection_target` is > 0 and we prevent adding input with `m_allow_other_inputs=False`, just as it's done in 4f571
...

⚠️ maflcko opened an issue: "intermittent issue in feature_reindex.py: Reindex fails to continue after restart; `initload` thread hangs forever"
(https://github.com/bitcoin/bitcoin/issues/30424)
The goal of `continue_reindex_after_shutdown` is to start with `-reindex`, then stop as soon as possible and restart the node to pick up the reindex again, then stop the node as soon as possible again.

However, sometimes the second shutdown stalls forever (without progress). So it seems like there are several bugs:

* The shutdown stalls forever (it should interrupt and shut down in a reasonable time)
* There is no reindex progess (it should pick up the previous work and try to make proges
...

🚀 ryanofsky merged a pull request: "Fix issues with CI on forks"
(https://github.com/bitcoin/bitcoin/pull/29274)

💬 ryanofsky commented on pull request "kernel, refactor: return error status on all fatal errors":
(https://github.com/bitcoin/bitcoin/pull/29700#issuecomment-2221105889)
Rebased 692fc11f0aae3c8013f6f01d133139ce8eba7135 -> ad1e73590d102edf3738986c1382b5e36a0ed727 ([`pr/fatalresult.17`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.17) -> [`pr/fatalresult.18`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.17-rebase..pr/fatalresult.18)) to fix conflicts

💬 hebasto commented on pull request "Bugfix: Correct first-run free space checks":
(https://github.com/bitcoin/bitcoin/pull/29678#issuecomment-2221122443)
Indeed, there is inconsistency in GB/GiB unit usage.

For instance, the value of the `m_assumed_blockchain_size` variable in GiB is used with the "GB" units in the user-faced messages here: https://github.com/bitcoin/bitcoin/blob/45f757c72672fc351bf669e731744f2f5e269233/src/init.cpp#L1707 and here: https://github.com/bitcoin/bitcoin/blob/45f757c72672fc351bf669e731744f2f5e269233/src/qt/forms/intro.ui#L206

Concept ACK.

---

It would be nice to mention the second commit changes in the PR
...

💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#discussion_r1672758736)
oh whoops, thanks. Should I move it now?

🤔 mzumsande reviewed a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2169946051)
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6

> Our own test framework was behaving that way before #29353, so maybe the functional tests would have caught it? I thought about reverting [c340503](https://github.com/bitcoin/bitcoin/commit/c340503b67585b631909584f1acaa327f5af25d3) locally to find out, but didn't have the time yet.

FYI, I tested reverting c340503b67585b631909584f1acaa327f5af25d3 on top of this branch now and without the latest fix (2) to ProcessMessages, other messages are act
...

🤔 paplorinc requested changes to a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2169947102)
Not yet sure about adding stuff that "will hopefully used this way one day".

Refactoring is welcome, as long as it's easy to review, which is not yet the case here, could you split the change into trivial, focused commits?

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1672775882)
Moving and modifying the code in the same commit (while many of the changes serve different purposes, mixing low and high-risk changes) makes the review a lot harder since the diff isn't really useful – having separate red and green parts, not really a diff.

Additionally, the commit doesn't explain each change separately. For example:
* The exact reason for memory management changes (via `make_secure_unique`) is not explained, nor are the security implications or potential performance impact
...