💬 Sjors commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2296430206)
Concept ACK.

In my testing of #30664 I keep seeing `share/rpcauth/__pycache__/` in `git status`, so that might need to be added to `.gitignore`.

🤔 paplorinc reviewed 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-2245093564)
The code became a lot simpler now - have a few suggestion I'd like us to consider, but I understand if you think it's out of scope.

Once we get to a state that is ACK-worthy, I'll enable hard assertions everywhere and run an IBD and check if it crashes anywhere.

💬 paplorinc 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_r1721733726)
```suggestion
Assume(IsDirty());
```

nit: it's a cheap call, why not make it an assert instead?

💬 paplorinc 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_r1721727681)
I was surprise to see that the method called `GetCoin` now:
* overwrites the dummy coin parameter with the value found in the cache
* returns a boolean indicating whether the coin exists in the cache and is not spent (the name doesn't have any hints about it being spent or not)

I don't mind if you think it's outside the scope of the current change, but checking the usages it seems to me we can now simplify `GetCoin` to actually return the coin and let the callsite check for spent and it mig
...

💬 paplorinc 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_r1721668812)
So basically if we lay out the allowed combinations we'd get:
```C++
assert((!entry.IsDirty() & !entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 0
(entry.IsDirty() & !entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 1
(entry.IsDirty() & entry.IsFresh() & !entry.coin.IsSpent()) | // attr = 3
(entry.IsDirty() & !entry.IsFresh() & entry.coin.IsSpent())); // attr = 5
```
Which could be simplified to
```C++
assert((entry.IsDirty() && !(entry.coin.IsSpen
...

💬 paplorinc 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_r1721738283)
is this is always the case, consider adding an assertion
```C++
assert(it->second.IsDirty());
```

(we can remove them in the last commit, but would be useful for the intermediary commits at least)

💬 paplorinc 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_r1721510656)
when do we throw and when do we `Assume` here?
> \* Assume is the identity function.
> \*
> \* - Should be used to run non-fatal checks. In debug builds it behaves like
> \* Assert()/assert() to notify developers and testers about non-fatal errors.
> \* In production it doesn't warn or log anything.
> \* - For fatal errors, use Assert().

Wouldn't this be a fatal error?

Alternatively, if we want to separate the test runs from production runs, would it maybe make sense to call Sani
...

💬 paplorinc 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_r1721740165)
same, can we add a (temporary) assertion for the removed value?

👍 ryanofsky approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2245487748)
Code review ACK d880419cb65fd9d446057ea05384dad55da0f292. Only changes since list review are documentation improvements and a small code simplification.

Overall this PR makes nice improvements to the waitfor* RPCs and simplifies the code getting rid of boost signals and a global variable.

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721752519)
In commit "Replace RPCNotifyBlockChange with waitTipChanged()" (bf3377f4bf988f2477944ca54c4dee33b9b45b6e)

`now` variable is unused if timeout is zero, so this line could be moved inside the `if (timeout)` block below.

💬 instagibbs commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2296543993)
@gmaxwell
>But, perhaps if there were padding traffic and a tiny amount of broadcast of third-party txn as if they were private broadcast then the weakness under the traffic analysis case would be improved to be no worse than you'd have from just running ordinary P2P across tor?

Assuming a switch to INV-based private relay, you might need to take some care to make sure the decoy tx gets requested at high rates, since the real private relay will tend to result in the getadata for the tx?

💬 paplorinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2296577863)
I'll review this in more detail later, but I have a question first:
Whenever a production change is made without corresponding test changes (yet the build passes), it suggests to me that there might be a gap in our coverage.
Do you think it would make sense to add a test that reproduces some aspect of the old behavior before making the change—one that would need to be updated in the commit that introduces the change?

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2296587174)
> it would make sense to add a test that reproduces some aspect of the old behavior before making the change—one that would need to be updated in the commit that introduces the change?

Good point, I can add a test that that expects flush every 24 hours, then reduce it to 1 hour after this change.

✅ furszy closed a pull request: "bugfix: update chainman best_header after block reconsideration"
(https://github.com/bitcoin/bitcoin/pull/29913)

💬 furszy commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2296594476)
closing in favor of https://github.com/bitcoin/bitcoin/pull/30666.

💬 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_r1721800990)
This behavior is documented [here](https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L306-L310), so not sure why test code bothered to do this. Indeed, switching to `std::optional` would be cleaner, but I think a much bigger refactor. This is also suggested in the original PR [here](https://github.com/bitcoin/bitcoin/pull/18746#issuecomment-842393167).

💬 paplorinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1721803828)
To simplify review, could you please separate the concerns - having low-risk changes in separate (moves, renames, inlines, rewording) commits from high risk ones?

👍 stickies-v approved a pull request: "refactor: Replace ParseHex with consteval HexLiteral"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2240152108)
ACK 67fc994bedf14e360b3e51fa1a71dc6c1684b532, but

> I think it would be better to remember the commit and then just update the called sites to accept `std::byte` (and then use that as an excuse to change the tests one-by-one to use the new HexLiteral function) in a follow-up. Otherwise, the tests will be changed again anyway for that reason (to replace `HexLiteral<uint8_t>` with `HexLiteral`).

I agree with this approach, although I also wouldn't object to keeping the PR as-is.

Otherwise
...

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721597256)
nit: unnecessary `std::begin`
```suggestion
std::vector<unsigned char> expected(HEX_PARSE_OUTPUT, std::end(HEX_PARSE_OUTPUT));
```

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721561190)
rename nit
```suggestion
// Limiting the value of rounds since it is otherwise uselessly expensive and causes a timeout when fuzzing.
```