💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2305497107)
> nSubsidyHalvingInterval being evenly divisible by 1000

That shouldn't be important in the new implementation anymore since we're counting every block until the block reward exists.

> The old test went on for ~2x the subsidy-granting height, seems like a limit as least as good as 10m?

Reverted to 14m, thanks.

💬 fjahr commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727720687)
> I aimed to view the code from the perspective of layman Bitcoin enthusiasts

I don't think that many of them will look at our unit tests and if they do it's probably easier if you stick to writing a comment that is understandable. But I don't think they are the target audience of our unit tests.

> I prioritized a high signal-to-noise ratio

I don't think that's the right strategy for our unit tests in general. I don't think we should remove checks unless there is a clear reason to do so
...

💬 fjahr commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2305510069)
As I indicated above, even with the changes, approach NACK from me. I think clarification can be achieved without a complete refactoring of this test. I'm neutral on adding comments and more checks.

💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727725376)
I've added back the intermediary asserts.

👋 ismaelsadeeq's pull request is ready for review: "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings"
(https://github.com/bitcoin/bitcoin/pull/30697)

💬 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_r1727754817)
@l0rinc It seems to me like you are thinking this PR has any behavior change. Any coin retrieved from `GetCoin` will be unspent if `GetCoin` returns true. You can follow this logically from seeing the return value [here](https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L65) and then the only base class is `CCoinsViewDB` which does not save spent coins so cannot retrieve them in `GetCoin`.

This PR is recognizing that `FetchCoin` logically cannot get to [here](https://github.com/bit
...

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727755299)
done

👍 tdb3 approved a pull request: "test: replace deprecated secp256k1 context flags usage"
(https://github.com/bitcoin/bitcoin/pull/30687#pullrequestreview-2255570973)
light CR and test ACK 60055f1abc4b4ad5f66a2fcf2e61c65efc777036

Thanks for finding/fixing.
Ran unit tests locally (including `key_tests`) (passed).
grep'ed `src/` for other (non `src/secp256k1/*`) instances of `SECP256K1_CONTEXT_SIGN` but didn't see any.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727759303)
Done thanks.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727762827)
Done.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727763363)
Added

💬 l0rinc 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_r1727769365)
Yeah, I understand that.

What I'm struggling with is the cache retrieval, and especially the invalidation logic, which seems to be scattered throughout the code and tightly coupled with the business logic.

Would you be open to a screen-sharing session to discuss this? It would help me better understand the constraints and likely speed up my review process as well.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727770914)
Done.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727771800)

This is what was written initially.
Clang format changed it to this format, which I thought was the recommended approach:
https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy

I've now switched to your suggestion.

💬 ismaelsadeeq commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1727772841)
This is fixed.

💬 TheCharlatan commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1727777786)
Couple of months ago I was thinking how to get rid of `g_best_block` and its friends entirely in the context of the kernel library. We already have two interfaces for notifying if something changed during validation, having a third one, that is also globally mutable and potentially dangerous if multiple chainstate managers are used is not ideal. I did not make progress then, but looking at it again now, I think this might be the opportunity to get rid of it.

How about duplicating the best blo
...

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727782615)
```suggestion
* use `std::byte` instead of `unsigned char` and `uint8_t`.
```

💬 cryptoquick commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2305593889)
I ran into the error earlier on in the IBD this time. It seems like there's no rhyme or reason as to why this is occurring.

```
2024-08-22T12:58:13Z UpdateTip: new best=0000000000000000011a06e4b1a3c497d247f785a4899e44ae800a9236438e16 height=424472 version=0x20000000 log2_work=85.108754 tx=148148770 date='2016-08-10T00:15:52Z' progress=0.137300 cache=124.8MiB(1146194txo)
2024-08-22T12:58:13Z UpdateTip: new best=0000000000000000003a58cdf5401248a1330480a1c9b99440a5f974fb61ce17 height=424473 ve
...

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727788962)
```diff
--- before 2024-08-22 22:41:41.949431649 +0200
+++ after 2024-08-22 22:41:56.957571134 +0200
@@ -1,7 +1,7 @@
-/**
+/**
* ""_hex is a compile-time user-defined literal returning a
- * `std::array<std::byte>`, equivalent to `ParseHex`. Variants include:
- *
+ * `std::array<std::byte>`, equivalent to ParseHex(). Variants provided:
+ *
* - ""_hex_v: Returns `std::vector<std::byte>`, useful for heap allocation or
* variable-length serialization.
*
@@ -21,5 +21,8 @@

...

📝 l0rinc opened a pull request: "CI: fix 3 simple codespell warnings"
(https://github.com/bitcoin/bitcoin/pull/30700)
Seen while investigating https://github.com/bitcoin/bitcoin/pull/30377/checks?check_run_id=29134897722:
* src/consensus/params.h:112: Enfore ==> Enforce
* src/test/fuzz/utxo_snapshot.cpp:75: Re-use ==> Reuse
* src/test/util/cluster_linearize.h:158: encounted ==> encountered, encounter