💬 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.
```

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721607971)
nit: these checks can be tidied up a bit:

<details>
<summary>git diff on 67fc994bed</summary>

```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 49bc4b1b50..792fdfde3e 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -217,17 +217,9 @@ BOOST_AUTO_TEST_CASE(consteval_hex_digit)

BOOST_AUTO_TEST_CASE(util_HexStr)
{
- BOOST_CHECK_EQUAL(
- HexStr(HEX_PARSE_OUTPUT),
- HEX_PARSE_INPUT);
-
- BOOST_CHECK_EQUAL(
-
...

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721609212)
nit: phantom newline

💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2296603677)
I'm using Debian. Here's my recent run (2 million iterations) using an initial corpus for both targets:


The new target is clearly faster. I think this is a more useful run because we generally fuzz from a corpus of inputs. I also reproduced the bug using only `-use_value_profile=1`. So overall, I would say this PR is good to go. I agree that it's probably not really wort
...

💬 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_r1721809523)
> but I think a much bigger refactor

Maybe, would it help if I did that in a different PR?

💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2296610487)
Let me know if you think there is anything more I can do to test. Looking at that flame graph is fun so I'm happy to keep fuzzing.