💬 fjahr commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + test":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2214762372)
nit: Could mention #28648 as related in the PR description.

💬 mzumsande commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1669015915)
Removed both TODOs.

💬 mzumsande commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2214767684)
[29ab88d ](https://github.com/bitcoin/bitcoin/commit/29ab88dfe1c43af3620480c99cba844aa414023c)to [ab478c5](https://github.com/bitcoin/bitcoin/commit/ab478c5fa16427b496e8a93e4780770d4c270214):
Addressed feedback by @fjahr (thanks!) and rebased.

👋 stickies-v's pull request is ready for review: "C++20 std::views::reverse"
(https://github.com/bitcoin/bitcoin/pull/28687)

💬 stickies-v commented on pull request "C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-2214772241)
With https://github.com/bitcoin/bitcoin/pull/30263 merged, this PR is now ready for review 🥳

📝 achow101 converted_to_draft a pull request: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328)
The legacy wallet `IsMine` code will be removed with the legacy wallet, but is still partially needed for migration. Instead of using `IsMine` directly in migration, equivalent checks are performed by migration.

Builds on #26596

👍 instagibbs approved a pull request: "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2163961806)
reACK 17d41e7a547f16f0dd3802dac73a63772ca000b2

biggest change is changing the new function to `ActiveTipChange` and making ibd a no-op

reviewed via `git range-diff master 0e0c422 17d41e7`

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1669031104)
fixed

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1669032119)
thanks accepted your suggestion with a slight modification.

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1669032477)
yes, this is gone now.

💬 theStack commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2214797055)
Thanks for the reviews! I force-pushed following [@dergoegge's suggestion](https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789) of doing the version message push in `SendMessages` instead, with the advantage of not needing to change the `NetEventsInterface` (sorry for invalidating the ACK @maflcko).
The previous version is still available on the following branch: https://github.com/theStack/bitcoin/tree/202407-p2p-fix_selfdetection_racecond_oldversion

💬 theStack commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1669035582)
Done as suggested.

💬 mzumsande commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2214820310)
> maybe someone can confirm that everything is deterministic and the snapshot can be loaded now (e.g. by adding an assert and setting `base_blockheight` and `m_coins_count` to 200 it should crash almost immediately).

FYI, confirmed for myself on another computer that this was just an error, and the snapshot can be loaded by the fuzzer.

🚀 ryanofsky merged a pull request: "psbt: Check non witness utxo outpoint early"
(https://github.com/bitcoin/bitcoin/pull/29855)

🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2164003242)
nice, I like this version a lot more.

I haven't investigated it yet, but now that we have a circular list, does the double linking still offer a measurable advantage or could we do the same with a single link and maybe 2 iterations instead (and would it be worth it, if it's possible)?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1669052113)
it seems to me that `next` remains unchanged after clear:
```C++
auto next{node->second.Next()};
node->second.ClearFlags();
assert(next == node->second.Next());
node = next;
```
so if that's intentional, we can simplify to:
```C++
node->second.ClearFlags();
node = node->second.Next();
```

💬 mzumsande commented on issue "RPC `getblock` resulted in 500 and ReadBlockFromDisk: OpenBlockFile failed for FlatFilePos(nFile=-1, nPos=0)":
(https://github.com/bitcoin/bitcoin/issues/20978#issuecomment-2214874858)
If the block is not available anymore, we already throw a specific error: "Block not available (pruned data)": [rpc](https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/rpc/blockchain.cpp#L586-L588) / [rest](https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/rest.cpp#L310-L312)

So I think it would only be consistent to add a check for `BLOCK_HAVE_DATA`, not call `ReadRawBlockFromDisk` and return a similar error (e.g. "Block n
...

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214881383)
@paplorinc With only a single link, how would you delete arbitrary map entries? Barring large hacks like putting the data in the next entry rather than the entry which the key points to, I don't see how you could delete entries in less than $\mathcal{O}(n)$ time.

💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1669095380)
this was unintentional. Disallowed it and added coverage.

💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214900126)
@theStack took test coverage suggestion, and yes "fees" is intentional since it will be in any net's address

@luke-jr I think @petertodd has stated what I would say. It's a pattern people can already do, and it's trivial and cheap to clean up.