💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2359202120)
looked at `git range-diff master 9f67d6dcdcf7e9c75a93ad78f8902537599f4f6d 4476915bc76b48eff9b5e67fd6bd7647cc12793f`

LGTM, though I haven't re-validated the serializer, trusting the tests of tests. I want to think more about the fuzz targets to see if there are any other easy pickups in coverage.

📝 LuizWT opened a pull request: "Optimize: convert trusted keys list to a set for better performance"
(https://github.com/bitcoin/bitcoin/pull/30925)
# Motivation

This pull request optimizes the handling of trusted keys by converting lists to sets. This change improves performance for membership checks, making them faster.
Changes Made

Changed trusted keys storage from lists to sets for faster access.

# Tests

No new tests were added, as this is a refactor that does not change the business logic.

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