💬 andrewtoth commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1749352045)
No, but without this the next commit would break.

💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1749355280)
Added https://github.com/bitcoin/bitcoin/pull/30849 to somewhat mitigate the `GetCoin` mutations in tests - hoping this will get us closer to excluding invalid tests states.

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749356667)
nit: don't remove space before `:`.

🤔 andrewtoth reviewed a pull request: "refactor: migrate `bool GetCoin` to return `optional<Coin>`"
(https://github.com/bitcoin/bitcoin/pull/30849#pullrequestreview-2288602488)
Concept ACK

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749357863)
For clarity this comment should go above where we use `m_rng`. So it should go a few lines up and probably be changed to `Randomly return spent coins`.

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749358772)
In commit 6243ceb65b83ea8dc3a689fdcc868a3a94b86e80 ` refactor: Remove unrealistic simulation state `

The comment above should be removed in this commit as well, since it is no longer accurate.

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749359238)
I'll just remove it

📝 klein818 opened a pull request: "doc: fix minor typo "
(https://github.com/bitcoin/bitcoin/pull/30850)
Fix typo in doc/build-windows-msvc.md:
- "Micsrosoft" -> Microsoft

No test required.

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749359555)
Already done, forgot to push

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749359570)
Done

💬 l0rinc commented on pull request "doc: fix minor typo":
(https://github.com/bitcoin/bitcoin/pull/30850#issuecomment-2336825778)
ACK 7a669fde183b2f98c4faeff3eedaf95a1cba3b09

💬 davidgumberg commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2336837979)
> > This has uncovered a bug in #28052, InitBlocksdirXorKey does not return an empty std::vectorstd::byte when the -blocksxor=0 argument is used, it instead [returns](https://github.com/bitcoin/bitcoin/blob/fa460884406b35b0dee75af23f42e8b4c4acbebc/src/node/blockstorage.cpp#L1185) a [vector with 8 bytes equal to zero](https://github.com/bitcoin/bitcoin/blob/fa460884406b35b0dee75af23f42e8b4c4acbebc/src/node/blockstorage.cpp#L1152):
>
> I may be missing something because I am looking at the bloc
...

👍 theStack approved a pull request: "test: Add coverage for dumptxoutset failure robustness"
(https://github.com/bitcoin/bitcoin/pull/30817#pullrequestreview-2288612539)
ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7

Further potential follow-up material (maybe a "good first issue" candidate): could go one step further and even test that if the network was already disabled (via `setnetworkactive False`), it won't be enabled after a `dumptxoutset` call. Right now the following patch passes all tests:
```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index b94b3a64e1..b1267bf808 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.c
...

💬 sipa commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2336842276)
@davidgumberg Well from the (apparently mistaken) assumption that the XOR logic comes with no material performance cost, it made sense to always want to use it (even with a pattern of zero), just from a perspective of minimizing different code branch combinations.

If the XOR logic is too slow (on some systems), as it appears to be, we have two options:
* Disabling it when (nonzero) XORing is not enabled, but that leaves us with the uncomfortable choice to make of (perhaps selectively) disabling
...

💬 andrewtoth commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2336842376)
> My bench logs indicate that the regression is coming from writing block undo data, the difference between the (reported) total block connection time between v27.1 and v28.0rc1 is +1,337.85 seconds for v28, and the difference in (reported) time spent writing undo block data is +1,579.31s for v28:

This is saying that the total time for writing undo block data exceeds total block connection time. But, writing undo block data is a subsection of block connection time, so it should always be less
...

🤔 fjahr reviewed a pull request: "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD"
(https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2288547896)
utACK bb08c227de158dbd88a80d904edb209e1734ab91

If we are in a hurry for v28 this is fine to merge as is, my comments can be kept for a follow-up.

Aside from the inline comments I would suggest to modernize the naming of `nLocalServices`. I prepared a commit to cherry-pick or I can open the follow-up: https://github.com/fjahr/bitcoin/commit/6cc25ffb1411285d8191bc932eb37b96348f022c

💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1749251951)
```suggestion
# Even though n2 is a full node, it will unset the 'NETWORK' service flag during snapshot loading.
```

💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1749253132)
Since this check is repeated six times it could be a reusable function

```python
def check_network_limited(node):
node_services = node.getnetworkinfo()['localservicesnames']
assert 'NETWORK' not in node_services
assert 'NETWORK_LIMITED' in node_services
```

💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1749385549)
see above

```suggestion
self.wait_until(lambda: any(tip['hash'] == snapshot_block_hash and tip['status'] == "headers-only" for tip in ibd_node.getchaintips()))
```

💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1749268034)
nit: I think this does the same and is a bit simpler? Allows to get rid of the `default_value` when applied to the `ibd_node` line below as well.

```suggestion
self.wait_until(lambda: any(tip['hash'] == headers_tip_hash and tip['status'] == "headers-only" for tip in snapshot_node.getchaintips()))
```