💬 SALIMMOASIN commented on something "":
(https://github.com/bitcoin/bitcoin/commit/13161ecf032b7a850686e5942c12222c8f3d0d52#commitcomment-145608572)
src/wallet/coinselection.cpp

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725282465)
`rand_seed` is also wrong after 022cf47dd7ef8f46e32a184e84f94d1e9f3a495c, so removing `rand_seed` is a "bugfix" commit.

🤔 pablomartin4btc reviewed a pull request: "assumeutxo: Add dumptxoutset height param, remove shell scripts"
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2251181767)
Concept ACK

I agree with the reasoning behind removing the scripts, specially after founding this [bug](https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2251032570) during testing. I'll also test this PR soon.

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2302386080)
Force pushed to add a bugfix commit to fix an incorrect test log message, and to remove unused symbols after a finding by @hodlinator in https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724749568 (Thanks!).

Should be easy to re-review by looking at the new commit and then checking the other commits via:

```
git range-diff bitcoin-core/master 3836bf1e0a b6b2b38aa7 --word-diff-regex=.
```

(or similar)

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725309351)
Thanks for noticing this! The unused symbols and the wrong logging should now be fixed in `prevector_tester`.

💬 naiyoma commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#issuecomment-2302472756)
Tested ACK [https://github.com/bitcoin/bitcoin/commit/27c976d11a68d66db97d9a7a30c6a6a71c6ab586](https://github.com/bitcoin/bitcoin/commit/27c976d11a68d66db97d9a7a30c6a6a71c6ab586)

Thanks for providing the comprehensive list above. I have tested it, and the output for empty `-rpcauth` is as expected: bitcoind fails to start, and Invalid -rpcauth argument is printed.

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1725390683)
You're right, that's simpler. Fixed.

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1725390903)
Done.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1725291235)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (4672a451efe65ed190b943be3cb2646344d06b56)

I think from https://en.cppreference.com/w/cpp/language/user_literal#Literal_operators you want to get rid of the space between `""` and `_hex`, because that seems to be a deprecated way of declaring literals. Though I'm not really clear why. (I found discussion about the space in https://github.com/fmtlib/fmt/issues/3607 though that didn't really clear things up either.)

👍 ryanofsky approved a pull request: "refactor: Replace ParseHex with consteval ""_hex literals"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2251172614)
Code review ACK c139a788e0052ece8f6c5689e4cd04b406032875. Switched to template literals since last review, so final state of this is a lot nicer. I like the new choice of suffixes, they seem to provide clarity and convenience.

I did leave one code suggestion to move literal operators to an inline namespace, but it could easily be a followup if it would complicate this PR, since it should only add new code.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1725375903)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (4672a451efe65ed190b943be3cb2646344d06b56)

I think commit message is not explaining reasons why different types of literals are being chosen, and not pointing out dangers of choosing the wrong type. For example, I think if _hex_v were used instead of _hex on this line, the code would still compile, but an improperly formatted network message would be sent.

Would suggest:

- refactor: Hand-replace some ParseHex -> ""_hex


...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1725284239)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (4672a451efe65ed190b943be3cb2646344d06b56)

I think I'd suggest putting these in util namespace to avoid risk of name collisions, especially since the util library is used by libbitcoinkernel and can be used externally.

```c++
namespace util {
inline namespace hex_literals {
// ...consteval auto operator""_hex...
} // inline namespace hex_literals
} // namespace util
```

Should allow code to use these by adding `using namespa
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1725358235)
In commit "util: Add consteval ""_hex[_v][_u8] literals" (4672a451efe65ed190b943be3cb2646344d06b56)

I think comment could also warn that difference between `_hex_v` and `_hex` suffixes is important when serializing, because vectors are always assumed to be variable-length and serialized with a size prefix, while arrays are considered fixed length and serialized without a prefix.

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1725418870)
You're right, I think you're talking about this part here:
https://github.com/bitcoin/bitcoin/blob/60b816439eb4bd837778d424628cd3978e0856d9/src/net_processing.cpp#L4754-L4758

It was added as a bug fix in https://github.com/bitcoin/bitcoin/pull/25717/commits/ed6cddd98e32263fc116a4380af6d66da20da990 and is part of what this harness is meant to test. If you remove that code from `net_processing` and run this test, it should crash.

💬 achow101 commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#discussion_r1725419006)
With debug enabled, 2016 blocks takes 3.4 minutes for me, vs 20 seconds for 144. Without debug, it's 54 seconds for 2016 and 15 seconds for 144.

💬 achow101 commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#discussion_r1725393568)
In e929054e12210353812f440c685a23329e7040f7 "Add timewarp attack mitigation test"

This constant is unused in this commit.

💬 achow101 commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2302548350)
ACK 59ff17e5af4e382cbe16f183767beef1bdcd9131

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2302558442)
> Perhaps it would be good to mention this in docs?

Agreed @brunoerg. We can add it post-merge, assuming people are on board with the method.

👍 ryanofsky approved a pull request: "test: [refactor] Use m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2251432210)
Code review ACK b6b2b38aa77162420babb34e01dd510512411abf. Just simplified prevector_tests and removed misleading BOOST_CHECK_MESSAGE print since last review.

💬 ryanofsky commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725435882)
In commit "test: Correct the random seed log on a prevector test failure" (faa2608e6c3e1763d0991443c3ce6d9bb1466da8)

I think it would be nice to just fix the bug and print the seed when the test fails rather than requiring looking up the seed in logs. It seems like we could easily support this by adding `return seed` as the last line of `SeedRandomForTest` and this would be helpful for debugging tests that only fail with certain seeds, because they could be run without always enabling logging
...