💬 ryanofsky commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2302254740)
Agree with furszy and marco that current behavior makes sense and there are a lot of existing options for dealing with this situation. I could imagine adding new options, however, like -reindex=auto to reindex automatically when needed, or `-allowfail=wallets` to ignore failures from wallets `-allowfail=indexes` to allow starting up when wallets or indexes fail to load.

👋 hebasto's pull request is ready for review: "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled"
(https://github.com/bitcoin/bitcoin/pull/30685)

👍 ryanofsky approved a pull request: "fix: handle invalid `-rpcbind` port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2251103866)
Code review ACK 8892150c2d8ea9e09046e288cecd4c35472c0267. Looks good! Nice to have earlier feedback and clearer checking for these options. I left a suggestion, but it's not important, so feel free to ignore

💬 ryanofsky commented on pull request "fix: handle invalid `-rpcbind` port earlier":
(https://github.com/bitcoin/bitcoin/pull/30679#discussion_r1725239115)
In commit "fix: handle invalid rpcbind port earlier" (8892150c2d8ea9e09046e288cecd4c35472c0267)

Not important, but to make AppInitMain function shorter and make the bugfix commit fix more readable you could consider adding an earlier refactoring commit the moves these for loops into a `CheckHostPortOptions` function, and have AppinitMain call it like `if (!CheckHostPortOptions(args)) return false;`

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