💬 Sjors commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2309750697)
re-utACK ac9d53d0dee26db58ab125aa369f476c232da660

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1730971231)
Ok, I've decided to pull in the doc update to the line of codes that I am modifying, to avoid having to touch them again. But the other refactoring and documentation will be done in the follow-up, as it needs a new commit anyway and I have a few follow-up queued up anyway.

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2309766315)
Pushed trivial doc-only update. Should be easy to re-review.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2309786222)
I incorporated https://github.com/TheCharlatan/bitcoin/commit/3c665065f132fd80ea3b1733785b98e45c8b371e so the `waitTipChanged()` implementation uses the kernel signal instead of `g_best_block`. Also rebased.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1730997386)
Done

💬 vasild commented on issue "-proxy does not work for addresses like 10.x.y.z":
(https://github.com/bitcoin/bitcoin/issues/25684#issuecomment-2309809495)
> Would it be enough just to document this change?

There is no central decision maker to answer that question. My advice would be to pick the solution you think is best, open a pull request and see if there will be a rough consensus around it (enough people to review and approve it and no objections).

💬 maaku 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#issuecomment-2309834241)
@Sjors I have brought this up on the mailing list, 4 months ago: https://groups.google.com/g/bitcoindev/c/CAfm7D5ppjo/m/Jiurx8AiAgAJ

🤔 ismaelsadeeq reviewed a pull request: "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface"
(https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2260433636)
This force-push [diff](https://github.com/bitcoin/bitcoin/compare/245dfa20bd37f78783adbd03897b7f144d71a7b8..528d4e67e68350f451180a2356a635f67f02287f) is a complete change in approach from the previous one, which was simpler and more straightforward 245dfa20bd37f78783adbd03897b7f144d71a7b8 (i.e., locking the mutex before starting the settings update sequence). The commit was reviewed and acked by @stickies-v thanks

The new approach was written by @furszy which modifies the chain interface’s `
...

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