💬 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
...

📝 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
...

💬 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

💬 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

💬 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...

🤔 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

💬 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 /
...

💬 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.

💬 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
};
```

💬 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
...

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760378255)
`Clean` means not dirty and not fresh. `NO_ENTRY` is a special value that is not a possible flag but signals to the tests that that entry does not exist. With proper types, that would correspond to a `std::nullopt`.
We don't have to name the enum `Flags`, we can name it `EntryState`. That way we don't have to validate the `flags` variable to see if it's one of the values we will accept. That is better served by using an enum and std::optional.

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760380040)
I don't think replacing with true and false is the right approach. Using enums, tests would be like
```
CheckAccessCoin(VALUE1, SPENT , SPENT , DirtyAndFresh, DirtyAndFresh);
CheckSpendCoins(ABSENT, SPENT , ABSENT, Fresh , std::nullopt );
```

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1760380094)
> is a special value that is not a possible flag but signals to the tests that that entry does not exist

> That is better served by using an enum and std::optional.

Thanks, I'll investigate tomorrow.
Please leave some comments about the overall direction as well, do you think this new test layout is more readable or less?

👍 theStack approved a pull request: "streams: cache file position within AutoFile"
(https://github.com/bitcoin/bitcoin/pull/30884#pullrequestreview-2305633593)
Code-review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35

Didn't run any benchmarks to compare with master.

💬 theStack commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1760381984)
Possible follow-up material: now that that the read operation above (std::fgetc) has been replaced by only fseek/ftell calls on the file object, I think this condition can't be true anymore and hence this else-branch could be removed.

📝 l0rinc converted_to_draft a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906)
Similarly to https://github.com/bitcoin/bitcoin/pull/30849, this cleanup is intended to de-risk https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1739909068 by simplifying the coin cache public interface.

`CCoinsCacheEntry` provided general access to its internal flags state, even though, in reality, it could only be `clean`, `fresh`, `dirty`, or `fresh|dirty` (in the follow-up, we will remove `fresh` without `dirty`).

Once it was marked as `dirty`, we couldn’t set the state back t
...

💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1760397936)
aha, that makes much more sense!

✅ glozow closed an issue: "Mempool + Validation Code Organization"
(https://github.com/bitcoin/bitcoin/issues/25584)

💬 glozow commented on issue "Mempool + Validation Code Organization":
(https://github.com/bitcoin/bitcoin/issues/25584#issuecomment-2351846451)
closing as (1) my main gripe with the fee estimator has been resolved, and (2) there isn't 1 correct actionable plan here. There are some general ideas of separation that we should incorporate into e.g. cluster mempool stuff, but I don't think there's any point in keeping this issue open.

💬 edilmedeiros commented on issue "contrib/signet/miner: grind will fail for high difficulty chain":
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2351881650)
Confirming this behavior is still happening after #28417. I'll rework #30130 on top of it.

```
2024-09-15 12:39:41 INFO Mined block at height 10078; next in -5784h44m40s (mine)
2024-09-15 12:39:58 INFO Mined block at height 10079; next in -5784h42m28s (mine)
2024-09-15 12:41:01 INFO Mined block at height 10080; next in -5784h41m0s (mine)
2024-09-15 12:41:43 INFO Mined block at height 10081; next in -5784h39m12s (mine)
Traceback (most recent call last):
File "/Users/jose.edil/2-develop
...