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

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721808875)
In commit "refactor: Make code more tolerant of constexpr std::arrays" (72256f9d78d2187411f55aa3f779170bed9bde1e):

Would be nice to add:

```c++
static_assert(WALLET_CRYPTO_IV_SIZE <= iv.size());
```

Since it's not obvious `iv` size matches `WALLET_CRYPTO_IV_SIZE` (I was surprised to see it's actually twice the size).

💬 Sjors commented on pull request "CMake replaces Autotools":
(https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2296707458)
Concept ACK on dropping Autotools quickly after merging cmake. Maintaining both sounds like a nightmare. Mainly for build system folks, but it's also more work for people making pull requests that add/remove files. Best just get it over with.

I tested rebasing my Stratum v2 related pull requests (#29346, #30315, #29432 and several on my own fork). It was pretty straight-forward, see https://github.com/Sjors/bitcoin/pull/58.

I haven't tried rebasing the IPC (multiprocess) variant of my PR.
...

💬 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_r1721879299)
I'm fine with that, extracted to a branch for now, will push as a draft after your PR stabilizes

👍 ryanofsky approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2245698982)
Code review ACK e8176694bb5508b2b488a8c79a6001d2fa625302, just reduced a variable scope since last review.