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

💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765886440)
In 7d46c2488679ccca70cdd1dc71100724347a47e8 this is a pretty big change behavour, right? I would have expected more in the patch that just a one line change (eg docs somewhere or perhaps a test somewhere that checks default behavour).

💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765883458)
Whitespace is different here compared to below (`mtx, /* version=*/ 2`). I don't know if you guys worry about that sort of thing.

💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765869737)
Is there a reason to compute the locktime again? It is done in the call to `GetUnsignedTx` just above.

(In 0cf1a9febb0205dadc03cd87a7af074e7b4cfd8c)

💬 tcharding commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1765876840)
Why change to a signed int? IIUC this is checked against the version field in transaction which is unsigned.

💬 Ajeetprajapati315 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/e49d858aab9bfae2455d87226d9ebebbdf2c40c8#commitcomment-146902742)
crypto.com
8700755231