💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723086287)
Agree `sizeof` was more of a footgun.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723087133)
Done in latest push without any hoisting.

💬 theStack commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#issuecomment-2298558011)
Thanks for the reviews! Took the suggestions and refined the UTXO check against the `scantxoutset` result to also include the vout index (=0), rather than only verifying the txid.

💬 theStack commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#discussion_r1723108255)
Introduced `snapshot_hash` and `snapshot_num_coins` in the beginning and used the already existing `SNAPSHOT_BASE_HEIGHT` for the height.

✅ fjahr closed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678)

💬 fjahr commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2298564117)
> See #30221. It fixes this in a more comprehensive way.

Ok, I didn't find it and weirdly there seems to be no conflict...

💬 fjahr commented on pull request "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate":
(https://github.com/bitcoin/bitcoin/pull/30636#issuecomment-2298593717)
utACK 917e70a6206c62c4c492fa922425fc8e00d3f328

Only changes since last review were addressing my minor review comments.

💬 Sjors commented on issue "TSAN/MSAN fails with vm.mmap_rnd_bits=32 even with llvm 18.1.3":
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-2298597110)
@maflcko clang 19 fixes neither, see https://github.com/Sjors/bitcoin/pull/59.

🤔 fjahr 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-2247673045)
Concept ACK

💬 fjahr 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_r1723134640)
Could you choose a different name for this variable than `nHeight`? Aside from the outdated style `next_height` or something like that is describing it better.

💬 fjahr 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_r1723117531)
This is also used to enforce the block storm mitigation.

👍 stickies-v approved a pull request: "refactor: Replace ParseHex with consteval HexLiteral"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2247641047)
re-ACK e29741857e900d933b5cf0fb22e3a63bfa1ecd6a

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723140049)
nit to reduce variable scope and make it slightly more readable:

<details>
<summary>git diff on bad1ffb725</summary>

```diff
diff --git a/src/wallet/crypter.cpp b/src/wallet/crypter.cpp
index a574c8909e..a4b1fd9b3b 100644
--- a/src/wallet/crypter.cpp
+++ b/src/wallet/crypter.cpp
@@ -100,12 +100,11 @@ bool CCrypter::Decrypt(const std::vector<unsigned char>& ciphertext, CKeyingMate
plaintext.resize(ciphertext.size());

AES256CBCDecrypt dec(vchKey.data(), vchIV.data(), tru
...

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723098790)
nit: `std::remove_reference_t`
```suggestion
static_assert(WALLET_CRYPTO_IV_SIZE <= std::remove_reference_t<decltype(iv)>::size());
```

💬 Sjors 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_r1723150526)
Renamed to `height` and added a comment.

💬 Sjors 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_r1723150550)
Updated the comment to reflect that.

👍 tdb3 approved a pull request: "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate"
(https://github.com/bitcoin/bitcoin/pull/30636#pullrequestreview-2247731072)
ACK 917e70a6206c62c4c492fa922425fc8e00d3f328

These tests are great additions.
Changes incorporate comments nicely.
Ran functionals locally (passed).

🤔 fjahr reviewed a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2247713859)
Concept ACK

I can confirm that best block locator and `last_processed_block` being different is confusing, see also https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719951882

Currently, this needs a rebase and I'm curious if @achow101 plans to make further changes based on @ryanofsky 's comments.

💬 fjahr commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r1723141952)
I'm thinking that it might make sense to keep this backup where it is and add second backup created at 199. Then this could test that both cases work as expected, i.e. 199 errors below and 299 doesn't. 299 is an interesting edge case in general (snapshot height == backup height).

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1723158537)
> I think adding "missing" space can be done in the rename commit.

Agree, would have preferred it that way as well.
But to be fair, git can ignore whitespaces during blame: https://git-scm.com/docs/git-blame#Documentation/git-blame.txt--w