💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2296587174)
> it would make sense to add a test that reproduces some aspect of the old behavior before making the change—one that would need to be updated in the commit that introduces the change?

Good point, I can add a test that that expects flush every 24 hours, then reduce it to 1 hour after this change.

✅ furszy closed a pull request: "bugfix: update chainman best_header after block reconsideration"
(https://github.com/bitcoin/bitcoin/pull/29913)

💬 furszy commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2296594476)
closing in favor of https://github.com/bitcoin/bitcoin/pull/30666.

💬 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_r1721800990)
This behavior is documented [here](https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L306-L310), so not sure why test code bothered to do this. Indeed, switching to `std::optional` would be cleaner, but I think a much bigger refactor. This is also suggested in the original PR [here](https://github.com/bitcoin/bitcoin/pull/18746#issuecomment-842393167).

💬 paplorinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1721803828)
To simplify review, could you please separate the concerns - having low-risk changes in separate (moves, renames, inlines, rewording) commits from high risk ones?

👍 stickies-v approved a pull request: "refactor: Replace ParseHex with consteval HexLiteral"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2240152108)
ACK 67fc994bedf14e360b3e51fa1a71dc6c1684b532, but

> I think it would be better to remember the commit and then just update the called sites to accept `std::byte` (and then use that as an excuse to change the tests one-by-one to use the new HexLiteral function) in a follow-up. Otherwise, the tests will be changed again anyway for that reason (to replace `HexLiteral<uint8_t>` with `HexLiteral`).

I agree with this approach, although I also wouldn't object to keeping the PR as-is.

Otherwise
...

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721597256)
nit: unnecessary `std::begin`
```suggestion
std::vector<unsigned char> expected(HEX_PARSE_OUTPUT, std::end(HEX_PARSE_OUTPUT));
```

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721561190)
rename nit
```suggestion
// Limiting the value of rounds since it is otherwise uselessly expensive and causes a timeout when fuzzing.
```

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721607971)
nit: these checks can be tidied up a bit:

<details>
<summary>git diff on 67fc994bed</summary>

```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 49bc4b1b50..792fdfde3e 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -217,17 +217,9 @@ BOOST_AUTO_TEST_CASE(consteval_hex_digit)

BOOST_AUTO_TEST_CASE(util_HexStr)
{
- BOOST_CHECK_EQUAL(
- HexStr(HEX_PARSE_OUTPUT),
- HEX_PARSE_INPUT);
-
- BOOST_CHECK_EQUAL(
-
...

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721609212)
nit: phantom newline

💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2296603677)
I'm using Debian. Here's my recent run (2 million iterations) using an initial corpus for both targets:


The new target is clearly faster. I think this is a more useful run because we generally fuzz from a corpus of inputs. I also reproduced the bug using only `-use_value_profile=1`. So overall, I would say this PR is good to go. I agree that it's probably not really wort
...

💬 paplorinc 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_r1721809523)
> but I think a much bigger refactor

Maybe, would it help if I did that in a different PR?

💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2296610487)
Let me know if you think there is anything more I can do to test. Looking at that flame graph is fun so I'm happy to keep fuzzing.

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721816302)
I think it's referring to:
https://github.com/bitcoin/bitcoin/blob/master/src/wallet/crypter.cpp#L40
i.e.
```C++
const unsigned int nRounds
```

💬 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_r1721817389)
Yes, but it would conflict heavily with this one. Many of the tests would have to be removed as well.

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721830663)
If you follow the call graph, I don't see how that can be true?

💬 marcofleon commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2296648809)
Sorry for taking a bit here. I still would like to figure out the stability issues for this harness (it's at about 70%), but it's turning into a whole rabbithole of its own. I don't think it needs to be a blocker for this PR. It would be good to have the test in and I'll continue to use this harness as my example for my "identifying instability" tool.

ACK 22f4d12eb25d34d8f2f6a23b44074dc1fb73f71d.

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721841148)
You're right, had an older version locally!

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721865864)
Done here and in `waitforblockheight`.

👍 ryanofsky approved a pull request: "refactor: Replace ParseHex with consteval HexLiteral"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2245582079)
Code review ACK 67fc994bedf14e360b3e51fa1a71dc6c1684b532, but it'd be fine to drop last two commits of this PR. I do think they would be improvement despite drawbacks Marco listed, but they aren't necessary and could be saved for a followup which fixes their shortcomings.