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

💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1749254225)
nit: Seems like this isn't needed so I would have preferred if that was left out or a separate commit because the file isn't touched at all otherwise.

💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#discussion_r1749385939)
If this is self-contained and you need to wipe everything this could have been done in a separate test line `p2p_assumeutxo.py` for example and you copy over the setup from this file here but it seems like there isn't much needed.

🤔 tdb3 reviewed a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2288595901)
Left a couple more comments.

💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1749344034)
nit: instead of making this an empty string if there is no address, could make the "address" key optional and omit it if nullopt.

💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1749385219)
A test could be added for this error condition (or could be added in a follow-up PR). Here's a rough idea (that still needs troubleshooting) (https://github.com/tdb3/bitcoin/commit/ae650c800f498c5df7f89696408c8b9c44801338)