💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1725063869)
Makes sense that const can expose things even in simple cases and lead to insights and improvements like the one you are suggesting. In this case, I think "if starts with prefix call remove_prefix" might be more straightforward than "if starts with prefix call substr and return portion of the string after the prefix" but the difference is not great and I take your point in general const can have benefits in places like this.

💬 furszy commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2302084858)
> As a user, I want to start bitcoind irrespectively of coins being in a wallet.

There are four ways to opt-out of running a wallet:
1) Do not run any wallet: `-nowallet=1`.
2) Do not run a specific wallet `-disablewallet=<wallet_name>`.
3) Remove the wallet json object manually from the settings.json file.
4) Disable wallet support at the binaries level during build.

Any of them will work fine in your specific case of not caring about your existing wallet.

💬 pablomartin4btc commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2302232259)
<details>
<summary>Continued testing on a full node until validation is finished (<code>getchainstate</code>) and fully synced, also on a pruned node (<code>-prune=30000</code>).</summary>

```
./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
{
"headers": 857301,
"chainstates": [
{
"blocks": 857301,
"bestblockhash": "000000000000000000017a865975aac40cdb7d05f4010a6e915ffe7af260a151",
"difficulty": 86871474313761.95,
"verificationprogress": 0.9999
...

🤔 pablomartin4btc reviewed a pull request: "validation: assumeutxo params mainnet"
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2251032570)
<details>
<summary>Continued testing on a full node until validation is finished (<code>getchainstate</code>) and fully synced, also on a pruned node (<code>-prune=30000</code>).</summary>

```
./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
{
"headers": 857301,
"chainstates": [
{
"blocks": 857301,
"bestblockhash": "000000000000000000017a865975aac40cdb7d05f4010a6e915ffe7af260a151",
"difficulty": 86871474313761.95,
"verificationprogress": 0.9999
...

👍 ryanofsky approved a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2251033417)
Code review ACK 888a167446901c12960e6776613a3a1fb9789f59. I think this would be a good change and improvement, however:

- I don't have a high level of confidence about this so would be interested in opinions from achow, furszy, and others who could point out other effects and drawbacks.
- If we are going to update the locator when backing up wallets, it seems like it might make sense to update it in other cases like when unloading wallets (https://github.com/bitcoin/bitcoin/pull/30678#discus
...

💬 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


...