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

💬 brunoerg commented on pull request "test: check xor.dat creation when missing":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1721922038)
```suggestion
assert_equal(read_xor_key(node=node), NULL_BLK_XOR_KEY)
```

💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2296780238)
ACK fa0b66400c12d11579b38b1ac76f677b23e8c82a

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721929753)
Thanks, will do if I have to retouch.

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2296824949)
Force pushed to remove stray, unused lines in the diff of the last commit. Should be easy to re-review.

📝 romanz opened a pull request: "http: set TCP_NODELAY when creating HTTP server"
(https://github.com/bitcoin/bitcoin/pull/30675)
Otherwise, the default HTTP server config may result in high latency, due to Nagle's algorithm (on the server) and delayed ACK (on the client):

[1] https://www.extrahop.com/blog/tcp-nodelay-nagle-quickack-best-practices
[2] https://eklitzke.org/the-caveats-of-tcp-nodelay


Without the fix, fetching a small block takes ~40ms (when connection keep-alive is enabled):
```
$ ab -k -c 1 -n 100 http://localhost:8332/rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bi
...

💬 andrewtoth commented on pull request "http: set TCP_NODELAY when creating HTTP server":
(https://github.com/bitcoin/bitcoin/pull/30675#discussion_r1721978296)
`LogPrintf` is deprecated. See https://github.com/bitcoin/bitcoin/pull/29641