💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664547028)
does the naming inconsistency apply here as well - given that we ignore the first element with instant `.Next()` as a first step?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664597181)
Yeah I suppose we can rename it `m_flagged_sentinel` or something? Do you have a suggestion that would have made this clearer if that was the name originally?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2206960214)
> I ran the benchmarks without prune (on the previous push), with just a lazy -reindex or -reindex-chainstate, but couldn't measure any difference - do I need more than 200k blocks? Or is prune critical here? Or is lazy reindexing not enough?

It shouldn't have much effect on non-pruning nodes, so it's good that there's no regression.
You will need pruning enabled to see the performance benefits. The higher the dbcache and lower the prune settings the better. The higher the block height the b
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2206961708)
> You will need pruning enabled to see the performance benefits

That requires a bit more setup to be able to do it offline

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664601643)
we have "sentinels" on both ends now, right? A dummy at the beginning and a `nullptr` on the end. This is what I found confusing in the test, why not store the actual head and have a `nullptr` or `sentinel` at the end only? I haven't investigated this, maybe I'm completely off again...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664604753)
Neat! Could also add `[[unlikely]]` here https://github.com/bitcoin/bitcoin/pull/28280/files#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR189

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664606357)
Hmm but accessing a null pointer would be a segfault and not an exception right? Sorry I don't follow how do you think this method could throw?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2206973129)
> That requires a bit more setup to be able to do it offline

I don't think it's possible to reindex the chainstate of a pruned node, since that requires having all blocks. So you will need to redownload blocks to benchmark this. In my setup I have another fully synced non-pruned node on a separate machine in my local network and I only connect the node being benchmarked to that other node.

💬 ryanofsky commented on issue "Use C++20 consteval to verify translation strings":
(https://github.com/bitcoin/bitcoin/issues/30379#issuecomment-2207003419)
> So I guess this needs a clang-tidy plugin.

Clang-tidy plugin is not actually required because #30383 can catch the errors, IIUC.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664633347)
> we have "sentinels" on both ends now, right? A dummy at the beginning and a nullptr on the end.

Yes, that sounds right.

> why not store the actual head and have a nullptr or sentinel at the end only?

I found it was more code to keep track of a changing head in the `CCoinsViewCache`, instead of a dummy sentinel I called `m_flagged_head` that remained static and all insertions happened after the sentinel.
If we tracked the actual head, we would have to assign the latest iterator added
...

👋 mzumsande's pull request is ready for review: "assumeutxo: Don't load a snapshot if it's not in the best header chain"
(https://github.com/bitcoin/bitcoin/pull/30320)

💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1664699548)
In 7461d0c006c92:

At this point, just a non-blocking nit but you are locking `cs_KeyStore` twice. On here, and another one inside `LegacyDataSPKM::LoadKeyMetadata`.

Same for `LoadScriptMetadata`.

💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1664685526)
Well, if you re-touch it again. Could:
```c++
LegacyDataSPKM* CWallet::GetLegacyDataSPKM() const
{
if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { return nullptr; }
return dynamic_cast<LegacyDataSPKM*>(GetScriptPubKeyMan(OutputType::LEGACY, /*internal=*/true));
}
```

🤔 furszy reviewed a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2157260912)
Code review ACK 8ce3739edbcf6437bf2695087e0ebe8c633df19b

💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1664702987)
In https://github.com/bitcoin/bitcoin/commit/7461d0c006c92ede2f2595b79a5509eaf3509fb7:

`FillableSigningProvider::AddKeyPubKey` already locks `cs_Keystore` internally.

💬 mzumsande commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2207155479)
After merge of #30267: Rebased, addressed nit by fjahr, and marked as ready for review.

🤔 sdaftuar reviewed a pull request: "cluster mempool: cluster linearization algorithm"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2151679554)
Just reviewed through the first 3 commits so far.

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1661222360)
Nit: noticed a couple typos in the commit message, if you end up touching this again:
```
This primarily adds the DepGraph class, which encapsulated precomputed
```
encapsulate**s**

```
number of a utility features (inspectors for set feerates, computing
```
~~a~~

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662798441)
nit: "satisyfy" --> "satisfy"

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1661239839)
Is this only used for verifying whether two DepGraph objects are identical, eg in the serialization/deserialization tests in the next commit?

It occurred to me that there might be some contexts where an equality operator for two Entry objects might want to be defined differently -- eg because you could have two Entry's in the same DepGraph with identical feerate and ancestor/descendant state, but still treat them as different objects -- but I'm guessing that is not the case for any of the use
...