👍 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
...
(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
...
(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
...
(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
(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.
```
(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
```
(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()))
```
(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()))
```
(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.
(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.
(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.
(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.
(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)
(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)
💬 davidgumberg commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2336867846)
@andrewtoth
>> 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 sho
...
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2336867846)
@andrewtoth
>> 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 sho
...
💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1749642423)
> made the test robust for me: [fjahr@ef00b8e](https://github.com/fjahr/bitcoin/commit/ef00b8eab48cf7ebcc8a0539f237175b8f5a0373)
Thanks for checking. Using a clean chain means that the cache won't be used, which normally lives on the storage unit where the build dir lives, so only the temp dir should be used. However, given the prior result that a non-tmp folder was generally fine, I would have expected the opposite result (Test would be less fragile by using the persisted cache on a non-tmp
...
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1749642423)
> made the test robust for me: [fjahr@ef00b8e](https://github.com/fjahr/bitcoin/commit/ef00b8eab48cf7ebcc8a0539f237175b8f5a0373)
Thanks for checking. Using a clean chain means that the cache won't be used, which normally lives on the storage unit where the build dir lives, so only the temp dir should be used. However, given the prior result that a non-tmp folder was generally fine, I would have expected the opposite result (Test would be less fragile by using the persisted cache on a non-tmp
...
⚠️ maflcko opened an issue: "CI timeouts"
(https://github.com/bitcoin/bitcoin/issues/30851)
Looks like some CI tasks are timing out, starting a few days ago.
It would be good to investigate and fix the problem.
A quick glance on the ccache stats may indicate that ccache stopped working for some tasks.
However, CI should also pass with a clean ccache, so the issue may be a different one, or there may be more than one issue.
(https://github.com/bitcoin/bitcoin/issues/30851)
Looks like some CI tasks are timing out, starting a few days ago.
It would be good to investigate and fix the problem.
A quick glance on the ccache stats may indicate that ccache stopped working for some tasks.
However, CI should also pass with a clean ccache, so the issue may be a different one, or there may be more than one issue.
💬 maflcko commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2337273254)
It could be a ccache issue, but another look is needed, see https://github.com/bitcoin/bitcoin/issues/30851
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2337273254)
It could be a ccache issue, but another look is needed, see https://github.com/bitcoin/bitcoin/issues/30851
💬 davidgumberg commented on issue "docs: Windows build intructions result in a large binary":
(https://github.com/bitcoin/bitcoin/issues/30593#issuecomment-2337273374)
+1 to adding a note in `docs/build-windows.md`
(https://github.com/bitcoin/bitcoin/issues/30593#issuecomment-2337273374)
+1 to adding a note in `docs/build-windows.md`
✅ maflcko closed a pull request: "test: use assert_greater_than util"
(https://github.com/bitcoin/bitcoin/pull/30019)
(https://github.com/bitcoin/bitcoin/pull/30019)
💬 maflcko commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#issuecomment-2337298742)
Closing for now due to inactivity on a test-only change for more than 6 months. Leave a comment below, if you want this re-opened.
(https://github.com/bitcoin/bitcoin/pull/30019#issuecomment-2337298742)
Closing for now due to inactivity on a test-only change for more than 6 months. Leave a comment below, if you want this re-opened.