🤔 l0rinc requested changes to a pull request: "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries"
(https://github.com/bitcoin/bitcoin/pull/30673#pullrequestreview-2260190262)
Dug deeper and added more relevant questions - bear with me if they're outside the scope of this PR.

💬 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_r1730941804)
+1 for moving the mutation to the loop
nit: for [consistency](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?plain=1#L76-L79) consider adding braces to the condition

💬 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_r1730970736)
nit: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?plain=1#L70

💬 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_r1731011814)
The docs state:
> When false is returned, coin's value is unspecified.

Eventually I hope we can untangle these two return values (coin & spentness), but temporarily maybe we can improve this guarantee (since I'm very much against unspecified mutations of non-local state) so that we only set the value of coin when it's unspent (similarly to how `Uncache` handles this case), e.g.:
```C++
bool GetCoin(const COutPoint& outpoint, Coin& coin) const final
{
if (auto it = m_data.find(outpoin
...

💬 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_r1730990127)
Seems a bit hacky, will check if there's a cleaner way to do this

💬 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_r1731019220)
nit: are we repeating business logic here? Can we make it more black-box somehow?

💬 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_r1731021714)
nit: can be modernized to:
```suggestion
if (CCoinsMap::iterator it = cacheCoins.find(hash); it != cacheCoins.end() && !it->second.IsDirty()) {
```

💬 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_r1731055682)
I'm having a hard time with https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L97, where the coin is obviously spent and can also be non-dirty (according to `updatecoins_simulation_test`, `ccoins_flush_behavior` and others), which violates `attr != 4`.

If it's valid for coin to (temporarily?) have that state, shouldn't `SanityCheck` allow it?

Or if this is the state when the parent-cache gets invalidated because of a re-org, could we maybe simplify the possible states by always
...

💬 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_r1731020615)
this will be redundant once we assert it in Prev and Next

💬 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_r1731110623)
Now that this structure is simpler, we could modernize (and optimize) this `find` / `try_emplace` construct as:
```C++
auto [itUs, inserted] = cacheCoins.try_emplace(it->first);
if (inserted) {
```

💬 Yihen-Liu commented on pull request "contrib: add dockerfile for building image fron source code":
(https://github.com/bitcoin/bitcoin/pull/30702#discussion_r1731144133)
Thanks for your replies, I learned a lot. This is my first bitcoin core path, I will continue to learn

👍 hodlinator approved a pull request: "test: [refactor] Use m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2253710691)
ACK 948238a683b6c99f4e91114aa75680c6c2d73714

Gets rid of `g_insecure_rand_ctx`-global state along with global functions using it, in favor of local `m_rng` contexts.

Tested adding..
```C++
BOOST_CHECK_EQUAL(m_rng.rand64(), 0);
```
..to one of the tests and verified that the random number emitted varied when re-running the test, and became constant when called repeatedly with `RANDOM_CTX_SEED` env var set, and different constant when modifying `RANDOM_CTX_SEED` value.

(Also uncovered
...

💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1729526826)
One could argue `ZEROS` is also "fixed", but I still agree `FIXED_SEED` with the documentation would be a step a good direction.

💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1726503732)
(Thread https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724794173)

Seems that this is not an issue (based on my fallible reading):

`SeedRandomStateForTest` -> `MakeRandDeterministicDANGEROUS` -> `GetRNGState().MakeDeterministic(seed)` -> which initializes the `std::optional` `RNGState::m_deterministic_prng`.

`GetRandHash` -> `GetRandBytes` -> `ProcRand(..., RNGLevel::FAST, /*always_use_real_rng=*/false)` -> which will end up sampling hardware-based entropy via `SeedFast` but
...

💬 hebasto commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2310056894)
> The point of this PR has never been to add any additional metadata to the binaries/make any other changes, the point is just to explictly use the hardening option when configuring our compiler (like we do with other options).

I see. Perhaps mention in the PR description that the resulting release binaries are not affected?

👍 pablomartin4btc approved a pull request: "qt: 28.0 translations update"
(https://github.com/bitcoin/bitcoin/pull/30715#pullrequestreview-2260572607)
ACK a0cdf43c4dde4f8d1983311fee2393fcde9123fe

No diff running `make -C src translate` on this PR.

👍 brunoerg approved a pull request: "test: Avoid intermittent block download timeout in p2p_ibd_stalling"
(https://github.com/bitcoin/bitcoin/pull/30705#pullrequestreview-2260594620)
utACK fa5b58ea01fac1adb6336b8b6b5217193295c695

🤔 furszy reviewed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2260723558)
I'm not seeing any drawback from doing it prior to a backup so far.

But the `Flush()` discussion (https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1724059212) made me think about the migration process. We aren't updating the best locator before unloading the wallet. So, might also want to add https://github.com/furszy/bitcoin-core/commit/5b618f1b024f16640a2a05f1e269f61b33b86577.

👍 brunoerg approved a pull request: "test: XORed blocks test follow up"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2260728801)
reACK e1d5dd732d5dc641faf1dde316275c84b6bb224b

🤔 MarnixCroes reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2260753659)
41051290ab3b6c36312cec26a27f787cea9961b4 succesfully built on debian12 and fedora40

- the build requirements for Fedora at `doc/build.unix.md` are incorrect/not updated
- disable-wallet mode is not working without sqlite dependency installed? Using `cmake -B build -DENABLE_WALLET=OFF`, as per the instructions, will not work without sqlite installed. (error that sqlite is missing)