💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760052629)
This assertion is obviated now.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760052629)
This assertion is obviated now.
💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760053494)
`But` can be removed now.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760053494)
`But` can be removed now.
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2351617058)
My graph on a log scale, looks similar to sipa's:
Results txt: [2024-09-14-bench.txt](https://github.com/user-attachments/files/17006092/2024-09-14-bench.txt)
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2351617058)
My graph on a log scale, looks similar to sipa's:
Results txt: [2024-09-14-bench.txt](https://github.com/user-attachments/files/17006092/2024-09-14-bench.txt)
💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2351617378)
> I could reproduce the macOS 2h timeout issue on GHA yesterday. However, today I also fail to reproduce them.
Actually, it looks like the GHA macOS CI timeouts remain today, for example: https://github.com/bitcoin/bitcoin/actions/runs/10833854937/job/30163805645?pr=30866 or https://github.com/bitcoin/bitcoin/actions/runs/10829942238/job/30163952659?pr=30856
I am not really familiar with GHA, nor with macOS, so it would be good if someone else checked:
* Does the macOS 13 GHA task succ
...
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2351617378)
> I could reproduce the macOS 2h timeout issue on GHA yesterday. However, today I also fail to reproduce them.
Actually, it looks like the GHA macOS CI timeouts remain today, for example: https://github.com/bitcoin/bitcoin/actions/runs/10833854937/job/30163805645?pr=30866 or https://github.com/bitcoin/bitcoin/actions/runs/10829942238/job/30163952659?pr=30856
I am not really familiar with GHA, nor with macOS, so it would be good if someone else checked:
* Does the macOS 13 GHA task succ
...
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760072626)
Because class declarations don't need semicolons, but I'll revert if you find it distracting
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760072626)
Because class declarations don't need semicolons, but I'll revert if you find it distracting
💬 furszy commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1760070775)
Isn't it a bit aggressive to execute the provided predicate every 0.05 seconds for the entire duration?
Beyond flooding the logs, the predicate likely calls an RPC command that locks `cs_main` or some other important mutex, which slows down the node.
For instance, the `feature_assumeutxo.py` one, will call `getpeerinfo()` 60 times.
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1760070775)
Isn't it a bit aggressive to execute the provided predicate every 0.05 seconds for the entire duration?
Beyond flooding the logs, the predicate likely calls an RPC command that locks `cs_main` or some other important mutex, which slows down the node.
For instance, the `feature_assumeutxo.py` one, will call `getpeerinfo()` 60 times.
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760102028)
Because class declarations don't need semicolons, but I'll revert if you find it distracting
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760102028)
Because class declarations don't need semicolons, but I'll revert if you find it distracting
💬 fjahr commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1760114937)
> I guess fs::is_empty(opts.blocks_dir) could be replaced by not fs::exists("blk0000.dat")?
I think that alone wouldn't work if the user upgrades a pruned node to v28?
Anyway, I was going to revisit this today but I simply don't see the failures anymore currently 🤷♂️ I will get back to it if I see them occur again.
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1760114937)
> I guess fs::is_empty(opts.blocks_dir) could be replaced by not fs::exists("blk0000.dat")?
I think that alone wouldn't work if the user upgrades a pruned node to v28?
Anyway, I was going to revisit this today but I simply don't see the failures anymore currently 🤷♂️ I will get back to it if I see them occur again.
💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1760138462)
I simply took the same interval as `wait_until` uses. If the slowdown was an issue it should be much more apparent from the use there, so I did a simple benchmark and increasing the sleep from `0.05` to `0.2` lead to a considerable slowdown of the test suite overall (8min to 10:45min), so I don't think this would be an issue here either.
We could still lower the interval. Since we generally don't expect the predicate to become false it shouldn't hurt performance as much as changing it in `wai
...
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1760138462)
I simply took the same interval as `wait_until` uses. If the slowdown was an issue it should be much more apparent from the use there, so I did a simple benchmark and increasing the sleep from `0.05` to `0.2` lead to a considerable slowdown of the test suite overall (8min to 10:45min), so I don't think this would be an issue here either.
We could still lower the interval. Since we generally don't expect the predicate to become false it shouldn't hurt performance as much as changing it in `wai
...
💬 naiyoma commented on pull request "p2p: Increase inbound capacity for block-relay only connections":
(https://github.com/bitcoin/bitcoin/pull/28463#issuecomment-2351678725)
Concept ACK to increase the number of maximum connections to **200**, enabling an increase in outgoing block relay connections later on.
(https://github.com/bitcoin/bitcoin/pull/28463#issuecomment-2351678725)
Concept ACK to increase the number of maximum connections to **200**, enabling an increase in outgoing block relay connections later on.
💬 vostrnad commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2351700413)
I've benchmarked commit e624a9bef16b6335fd119c10698352b59bf2930a, both in this branch and cherry-picked onto a few selected commits (fresh sync to block 400,000 from a local node, using default settings, all binaries self-built using Mingw-w64, around 10 runs for each version).
Summary: some slowdown due to #28052 definitely remains, but it's under 5%.
|version|result|note|
|-|-|-|
|fa7f7ac040a9467c307b20e77dc47c87d7377ded|2112 ± 17 seconds|First commit of #28052, and last commit before
...
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2351700413)
I've benchmarked commit e624a9bef16b6335fd119c10698352b59bf2930a, both in this branch and cherry-picked onto a few selected commits (fresh sync to block 400,000 from a local node, using default settings, all binaries self-built using Mingw-w64, around 10 runs for each version).
Summary: some slowdown due to #28052 definitely remains, but it's under 5%.
|version|result|note|
|-|-|-|
|fa7f7ac040a9467c307b20e77dc47c87d7377ded|2112 ± 17 seconds|First commit of #28052, and last commit before
...
📝 pablomartin4btc opened a pull request: "Fix both ipv6 proxy setup and reachable networks display issues in Options Dialog (UI only, no functionality impact)"
(https://github.com/bitcoin-core/gui/pull/836)
Currently, setting up a proxy (whether SOCKS5 or Tor) with an IPv6 address works correctly via the command line or configuration file in both `bitcoind` and `bitcoin-qt` (also from the UI the ipv6 address gets saved properly in `settings.json`). However, the UI does not reflect this properly, which can create confusion. Since some ISPs and VPNs still experience issues with IPv6, users may mistakenly think there is a problem with Bitcoin Core, when in fact the proxy setup is functioning as expect
...
(https://github.com/bitcoin-core/gui/pull/836)
Currently, setting up a proxy (whether SOCKS5 or Tor) with an IPv6 address works correctly via the command line or configuration file in both `bitcoind` and `bitcoin-qt` (also from the UI the ipv6 address gets saved properly in `settings.json`). However, the UI does not reflect this properly, which can create confusion. Since some ISPs and VPNs still experience issues with IPv6, users may mistakenly think there is a problem with Bitcoin Core, when in fact the proxy setup is functioning as expect
...
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760258646)
Replaced it with a flag validation
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760258646)
Replaced it with a flag validation
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760258689)
Done
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760258689)
Done
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#issuecomment-2351740285)
> Can we remove any other tests that are trying to add flags that are not FRESH or DIRTY and so are now useless?
I've merged `NO_ENTRY` and `char(0)` values - this simplifies the cases somewhat.
Do you think we can remove any of the lines? There aren't any duplicates, even after the above change...
(https://github.com/bitcoin/bitcoin/pull/30906#issuecomment-2351740285)
> Can we remove any other tests that are trying to add flags that are not FRESH or DIRTY and so are now useless?
I've merged `NO_ENTRY` and `char(0)` values - this simplifies the cases somewhat.
Do you think we can remove any of the lines? There aren't any duplicates, even after the above change...
🤔 glozow reviewed a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2305615852)
reACK 9ad2fe7e69e, just have a question about the docs
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2305615852)
reACK 9ad2fe7e69e, just have a question about the docs
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1760336690)
Are these comments in the format timestamp, tx count, fee, size (or similar)? Or are they actually something like iteration count? I added a print statement to get feerates of clusters but got something slightly different.
```
total feerate: 4818977 / 54585, 71 txns
total feerate: 15029636 / 99008, 81 txns
total feerate: 1288344 / 98145, 90 txns
total feerate: 15755611 / 77992, 87 txns
total feerate: 429736 / 8245, 35 txns
total feerate: 695118 / 60056, 60 txns
total feerate: 941605 /
...
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1760336690)
Are these comments in the format timestamp, tx count, fee, size (or similar)? Or are they actually something like iteration count? I added a print statement to get feerates of clusters but got something slightly different.
```
total feerate: 4818977 / 54585, 71 txns
total feerate: 15029636 / 99008, 81 txns
total feerate: 1288344 / 98145, 90 txns
total feerate: 15755611 / 77992, 87 txns
total feerate: 429736 / 8245, 35 txns
total feerate: 695118 / 60056, 60 txns
total feerate: 941605 /
...
💬 sdaftuar commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1760345789)
The format was timestamp (from my simulation), tx count, min iterations when running Linearize 10 times with different rng seeds and then once more when running it woth LIMO using the optimal linearization as input, and then max iterations over the same set of 11 runs.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1760345789)
The format was timestamp (from my simulation), tx count, min iterations when running Linearize 10 times with different rng seeds and then once more when running it woth LIMO using the optimal linearization as input, and then max iterations over the same set of 11 runs.
💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760349038)
Since we don't have pass a bitmap as flags anymore, does it make sense to get rid of the `char` type for flags in `coins_tests` and replace with a more descriptive enum?
```C++
enum Flags
{
Clean,
Dirty,
Fresh,
DirtyAndFresh
};
```
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760349038)
Since we don't have pass a bitmap as flags anymore, does it make sense to get rid of the `char` type for flags in `coins_tests` and replace with a more descriptive enum?
```C++
enum Flags
{
Clean,
Dirty,
Fresh,
DirtyAndFresh
};
```
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760375960)
what's the difference between clean and NO_ENTRY? Does that difference still make sense after this change?
> get rid of the char type for flags in coins_tests and replace with a more descriptive enum
Since we don't have flags in the production code anymore I would prefer getting rid of them in the tests as well. Reintroducing an enum would kinda' defeat that purpose in my opinion.
To make sure the tests are still checking the same combinations, I have added a few scripted diffs to trans
...
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760375960)
what's the difference between clean and NO_ENTRY? Does that difference still make sense after this change?
> get rid of the char type for flags in coins_tests and replace with a more descriptive enum
Since we don't have flags in the production code anymore I would prefer getting rid of them in the tests as well. Reintroducing an enum would kinda' defeat that purpose in my opinion.
To make sure the tests are still checking the same combinations, I have added a few scripted diffs to trans
...