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

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721880533)
In commit "Replace RPCNotifyBlockChange with waitTipChanged()" (b4d37fa70506129ba4a4d218bc49ce8ca9d8380c)

Not important, but could simplify these two lines to `while (IsRPCRunning() && block.hash != hash)`

Similarly, could simplify to `while (IsRPCRunning() && block.height < height)` in waitforblockheight below.

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2296723316)
Ok, the flamegraph helps. With a quick skim, it looks like all flames are somehow IO (read or write) related. This means that you are not evaluating the fuzz target itself, but the speed of your storage and how it can handle the data that the fuzz engine produces for it to read and write. I guess, starting with an empty fuzz input set somehow encourages the fuzz engine to create large inputs, which then cause a high IO usage.

My recommendation would be to make sure you are fuzzing on a ramdis
...

💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2296762977)
Makes sense, was just working with Niklas to get this done. The flame graphs are a useful new tool for me too, I'll be utilizing those more. Thanks for the help @maflcko.