💬 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));
```

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1765112447)
While "clean" is the opposite of "dirty", it doesn't involve how *fresh* something is. It's on another dimension.

I actually prefer `ClearFlags()`, `ClearState()` or possibly `Reset-something()`.

Same goes for `Caching::CLEAN`. Would prefer `VOID`/`EMPTY`/`INIT`/`NULL`.

💬 fjahr commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2359486900)
Sorry for the long wait. I added the following changes:

- rebased to get rid of autotools here
- added commit from @furszy for the migration context as suggested here: https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2260723558
- added a separate test for this in `wallet_backup.py` to make sure this is covered not only in the assumutxo context but also the pruning context

💬 fjahr commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#issuecomment-2359493766)
> Maybe we should have a custom locator with the last-change block, and last-scanned block?

Seems like a bigger change, I will leave this to be explored in a follow-up. It should probably be weighed against the potential follow-up changes proposed in #30221.

🤔 sipa reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2313906901)
@instagibbs I noticed that your extending of `clusterlin_cluster_serialization` makes the fuzz test ~10x slower (when sanitizers are enabled), and I don't understand why.

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765788553)
I have added some documentation and `Assume`s.

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765789292)
Good idea, done (in `clusterlin_cluster_serialization`).