💬 theuni commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1765609266)
Arguably the issue here is that we have debug symbols for secp at all.

💬 laanwj commented on pull request "Optimize: convert trusted keys list to a set for better performance":
(https://github.com/bitcoin/bitcoin/pull/30925#issuecomment-2359296620)
Have you benchmarked the performance difference?

💬 laanwj commented on pull request "Optimize: convert trusted keys list to a set for better performance":
(https://github.com/bitcoin/bitcoin/pull/30925#discussion_r1765643226)
Have you even tested this? You can't subscript index into a set, and it makes no sense.

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765576120)
AddDependency doesn't exist anymore

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765664723)
if not erroneous, I think a bit stronger
```Suggestion
Assume((m_used.None() && pos_range == 0) || m_used.Last() == pos_range - 1);
```

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765657306)
is this not still true?

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765667807)
Haven't looked close enough to the new format, but pushing another element to `reordering` after hitting `SetType::Size` already seems to make the DepGraph construction later odd? Couldn't this make the mapping argument too large?

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765572322)
I think this gives additional coverage since no deps added in a `AddDependencies` call is allowed
```Suggestion
const auto parents_mask = provider.ConsumeIntegralInRange<uint64_t>(0, (uint64_t{1} << num_tx) - 1);
```

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765598717)
Could use some documentation on how to use `mapping` and `pos_range`, with holes it's not immediately obvious.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2359409218)
> 1. Child`MempoolRejectedTx`, put in orphanage
>
> 2. Child `MempoolRejectedTx` for other reason, now in reject filter

On first glance that sounds impossible, `MempoolRejectedTx` should have erased it from orphanage? Will look into the error

🤔 mzumsande reviewed a pull request: "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress"
(https://github.com/bitcoin/bitcoin/pull/30909#pullrequestreview-2313807588)
> Whereas calling a rescan on the (missing) background blocks seems like a bug that should be avoided at the call site, no?

Well, the approach seems to be similar with pruning:
`importdescriptors` tries to scan as many blocks as possible (no stopping after a failure due to missing block data), and then returns a failure / warns the user that the result may be incomplete. It could probably have been implentend differently, checking if we are missing blocks due to pruning and refuse to import
...

💬 mzumsande commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1765732245)
I think this should only be possible to trigger with the rescan, the various RPCs are always be called with a block (usually the tip) that is guaranteed to have `m_chain_tx_count` set.

💬 SanAndreyas commented on pull request "Optimize: convert trusted keys list to a set for better performance":
(https://github.com/bitcoin/bitcoin/pull/30925#discussion_r1765732956)
This is not even an equivalent expression. When you convert a list to a set, the order of the lines is lost. Therefore, accessing the first element with [0] could give you any of the lines randomly, not necessarily the first line from the original file. In this case you needed index 0 ( root ).

🤔 hodlinator reviewed a pull request: "test: generalize HasReason and use it in FailFmtWithError"
(https://github.com/bitcoin/bitcoin/pull/30921#pullrequestreview-2313849581)
As [previously stated](https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1765022377) I would *most* prefer the parameters to `FailFmtWithError` were reverted so it took a `string_view`. The current version (regrettably suggested as an alternative by yours truly) uses `HasReason` "out of context" just to be more friendly to memory when using `HasReason` repetitively. I wouldn't blame others for finding it a step back from a readability standpoint.

The whole reason of the PR was to use
...

💬 l0rinc commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#issuecomment-2359468637)
Thanks for your detailed explanation @hodlinator.

> optimal than the current version

We're talking about something extremely trivial, inside tests, it could be a million times more expensive and still wouldn't matter...

> It requires changing the HasReason constructor to take a string_view instead of const string& which is arguably a step in the right direction anyway (+ relies on your other changes to that type).

I had a `string_view` constructor for `HasReason` before, but it was
...

💬 l0rinc commented on pull request "Optimize: convert trusted keys list to a set for better performance":
(https://github.com/bitcoin/bitcoin/pull/30925#issuecomment-2359475211)
Approach NACK - no explanation, no benchmarks, no explanation for how to test this, etc

💬 fjahr commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1765784323)
Point taken that this is about backups and not assumeutxo primarily, so I have added another test for this in`wallet_backup.py`.

> It would be beneficial to have this in a separate test method so that we can add more test cases cleanly without associating it with the assume UTXO test.

I don't fully get this comment though. I don't know how I could make a method that would make this code reusable. The code in `wallet_assumeutxo.py` is spread across the test and pretty specific to the assume
...

🤔 hodlinator reviewed a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2312129363)
Concept ACK.

Really like the move to put restrictions on how flags are used.

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1765771596)
Types should for the most part be nouns IMO. Seldom verbs in present participle like `Caching`. `CoinStruct` feels a bit WIP too.

Suggestions:
```suggestion
struct CoinState {
const CAmount value;
const CacheState cache;
```
```suggestion
struct CoinCache {
enum State {
CLEAN,
DIRTY,
FRESH,
DIRTY_FRESH,
};
const CAmount value;
const State state;
```

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1764709955)
nit: terseness++
```suggestion
assert(flags & (DIRTY | FRESH));
```