📝 furszy opened a pull request: "init: fix init fatal error on invalid negated option value"
(https://github.com/bitcoin/bitcoin/pull/30684)
Currently, if users provide a non-boolean convertible value to a negated option,
such as '-nowallet=not_a_boolean', the initialization process results in a fatal
error, causing an unclean shutdown and displaying a poorly descriptive error
message:
"JSON value of type bool is not of expected type string."

This PR fixes the issue by checking the command-line argument type before
any code attempts to access it. Additionally, it improves the returned error
message, changing it to:
"Invalid
...

💬 fjahr commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2299019789)
> I wonder if something else could be done to resolve this confusion

Ok, I would say it's a problem when users see behavior that is inconsistent because of this difference. If the users don't notice it I guess these don't have to be the same at all times. There could be a more "pull-based" approach where they are aligned when the user interacts with the wallet, similar to what I tried to do for the backup case in https://github.com/bitcoin/bitcoin/pull/30678. But I am not clear yet on which a
...

💬 glozow commented on pull request "MiniWallet funcitonality for more readable code in functional test `rpc_signtransactionwithkey.py`":
(https://github.com/bitcoin/bitcoin/pull/30667#issuecomment-2299024211)
Thanks for your interest in contributing. I recommend reading the [contributing guidelines](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md) and [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md), which contain helpful tips for getting started, squashing commits and rebasing, motivation for refactoring, etc.

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2299042222)
> does not make the binary smaller (built x86_64-linux-gnu guix bitcoind binary) - actually grows by 0.04% (~49.6 KB).

Not sure if it's a dealbreaker of not, but it seems that we're not actually storing the hexadecimal values as an array of bytes, but rather as separate stack pushes, which might be the reason for the size increase, i.e.
```C++
HexLiteral("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f")
```
...

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2299048734)
It would be great if we could open an issue describing the user behavior that's confusing. I read the comment you linked to but I didn't really understand the context of what was going on with those backups. If there can be a github issue that simply describes steps to reproduce, expected behavior, and actual behavior, we can probably figure out if expected behavior is reasonable, and implement a fix if so.

👍 ryanofsky approved a pull request: "test: [refactor] Use m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2248303256)
Code review ACK 3836bf1e0a207a6cb659053404bcba3a44cf7bb2

Just tweaked cuckoocache_tests since last review

💬 marcofleon commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2299172118)
Code review ACK 3836bf1e0a207a6cb659053404bcba3a44cf7bb2

🤔 marcofleon reviewed a pull request: "test: [refactor] Use m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2248388003)
Code review ACK 3836bf1e0a207a6cb659053404bcba3a44cf7bb2

🤔 glozow reviewed a pull request: "test: check xor.dat creation when missing"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2248421893)
ACK 7b7c287a33dc0c6f09bf32f31a8f05941b0cc9a2

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1723577797)
re: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688616750

> Let me try a different angle: if a method is inline, I would expect to be able to copy-paste its content to the call site directly (at least conceptually, to understand its effect). If the method is pure in a functional sense (including that it doesn't even mutate its parameters), it's a lot easier to reason about its overall behavior. Does this resonate more?

I actually learned something new about this looking at b
...

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723595926)
I see that my example above wasn't working with GCC and had a stupid `sizeof` bug.

Here's an alternative using [string literal operator template](https://en.cppreference.com/w/cpp/language/user_literal#Literal_operators) which seems to be working correctly for me:
```C++
template<size_t N>
struct _Hex {
std::array<std::byte, N / 2> hex;
consteval _Hex(const char (&hex_str)[N]) requires (N % 2 == 1) {
if (hex_str[N - 1]) throw "null terminator required";
for (std
...

🤔 glozow reviewed a 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#pullrequestreview-2248488562)
concept ACK

💬 glozow 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_r1723610963)
It would be nice to reduce the magic numbers in this test a bit

`DIFFICULTY_ADJUSTMENT_INTERVAL` instead of 144, `MAX_FUTURE_BLOCK_TIME` instead of 2 * 3600, etc.

💬 glozow 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_r1723607722)
> In order for the test to run faster, we reduce its difficulty retarget period to 144,

The difference between using 144 and 2016 = mining_basic.py takes 30sec longer on my machine, which doesn't seem like such a big deal that we should change the difficulty adjustment period. I don't feel strongly, but it could be easier to just count mining_basic.py as one of the 30-60sec functional tests. :shrug:

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1723625119)
> function parameters passed by value doesn't make sense as a general rule

[Google cpp guide](https://google.github.io/styleguide/cppguide.html#Use_of_const ) mostly aligns with your view on this. However, I approach this from a more functional programming perspective, where I want to infer the possible state changes a method can perform based solely on its signature - or at least what it cannot do.

Thanks for following up on this, @ryanofsky!

👍 ryanofsky approved a pull request: "refactor: Replace ParseHex with consteval HexLiteral"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2248351160)
Code review ACK c3fa29db1b05aa51f74cc8be5bdae59be4f3b7c0. Main changes since last review were cleanup commits being splitup, and ToScript being changed to accept std::byte so it works better with HexLiteral

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723616777)
In commit "refactor: Hand-replace some ParseHex -> util::HexLiteral" (288f995f71fd8e6f750d668e47237e64d893cc1f)

I feel like commit message could benefit from a summary of the changes it is making. Maybe would suggest:

- The following scripted-diff commit will replace `ParseHex(...)` with `HexLiteral<uint8_t>(...)` but replacement will not work in cases where vectors are needed instead of arrays, and is not ideal in cases where std::byte can be used instead of uint8_t, so make replacements
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723537853)
In commit "refactor: de-Hungarianize CCrypter" (7713b5fb5c5bf0897037b9857a4adfbbc7e8db56)

Commit message seems to suggest that this is only changing names and adding braces, but this is also switching an iterator type to auto. Might be good to mention in commit message, assuming this change wasn't intended for a different commit.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723523414)
In commit "refactor: add util::HexLiteral and util::Vec using statements" (af751d7030ccb5c2e3e699bd90655a53529953a3)

Renaming `container` variable to `span` would seem clearer, since this is now a span not a generic container.

Also this commit message is out of date since it is no longer using util::Vec, and the commit is no longer mostly about adding using statements. I'd probably just call it "Prepare for HexLiteral scripted-diff"

💬 andrewtoth commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1723632249)
What you are suggesting is basically a concept NACK on this PR. If we aren't confident that we will never get a spent coin from GetCoin, then we need to keep the logic to handle spent and FRESH coins.