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

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765788969)
No; `mapping` should be equal to `depgraph.PositionRange()` though, which I've clarified and added an `Assume` for.

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765790977)
`reordering` has a size equal to the number of transactions (excluding holes) in the reconstructed `DepGraph`, and can indeed not over `SetType::Size()` (this is also asserted above). However, `total_size` does include holes, and can reach `SetType::Size()` (that's exactly what this branch is for, and it does not increment `total_size` anymore).

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765791761)
Indeed, done.

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765791238)
I made it even more explicit with `Assume(m_used.None() ? (pos_range == 0) : (pos_range == m_used.Last() + 1));`.

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765791639)
Gone.

💬 hodlinator commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#issuecomment-2359497764)
> I had a `string_view` constructor for `HasReason` before, but it was regarded as [premature optimization](https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1764912413), so I've reverted that part.

Is there a way to see complete force-push history? Since you force-pushed twice in a short timespan I'm unable to see your first version of the PR. I suspect from maflcko's comment that you may have been changing `HasReason::m_reason` itself into a `string_view` as well(?), which I agree w
...

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2359531099)
Ah, I think it was bugs in the if/else logic in `MempoolRejectedTx`. The flow should be:

```
if (missing inputs) {
if (first time and not already rejected) {
consider keeping orphan...
}
return
} else if (error A) {
special A stuff
} else if (error B) {
special B stuff
} else {
cache rejections, etc
}
```

But we (1) dropped the return early and (2) didn't have nested `if`s, so we started caching orphans in recent rejects. I'll work on writing more unit tests. I
...

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1765803669)
*fresh* is actually a substate of *dirty*. Being *fresh* implies *dirty*, so cleaning implies no longer fresh..

🤔 tcharding reviewed a pull request: "Implement BIP 370 PSBTv2"
(https://github.com/bitcoin/bitcoin/pull/21283#pullrequestreview-2314013431)
Caveat: I'm not a C++ dev and don't know much about Core development.

I did however check the changes here against my Rust implementation of the same, and found some bugs in mine, so cheers!