👋 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

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727790062)
Just spotted the whitespace at end of line.. fixing. :)

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1727794829)
("ParseHex()" becomes a link in Doxygen).

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2305609525)
ACK df92661444f46790b12d5061344d72106ef820d9

Doc updates

💬 theStack commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1727850357)
Not sure if there are guidelines for this (and if yes, how strict we are), but I would tend to avoid using the "property" decorator for non-trivial methods that access state outside of the class (in this case, by interacting with the file system). At least I wouldn't expect as user that accessing a class field ever involves any I/O. I think dropping the property and keeping the name as-is "read_xor_key" might be just fine?