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

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662746393)
Should this function be checking that we haven't exceeded the capacity of the SetType?

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662824605)
Should this say "serializer" instead of "deserializer"?

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662822694)
I was really confused about why we need to use `CanAddDependency` here -- as far as I can understand, it's basically about compression, so that we get shorter serializations? And conceptually, the reason compression is helpful is in the context of serializations that are produced by a fuzzer being as meaningful as possible?

I guess compression doesn't hurt here, but just to verify my understanding: would all this logic work just fine if we dropped the use of `CanAddDependency` in the seriali
...

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1664704882)
nit: if you touch up this commit, I think the commit message has a couple typos:

`This is a class that encapsulated precomputes ancestor set feerates, and`

should read "encapsulates precomputed ancestor set feerates"

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662966890)
Nit: The fee/size values in this comment don't seem to match the fee/size values in the test below.