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

💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662836077)
Left another comment down below regarding `CanAddDependency()` in the serializer -- it makes sense to me to use it in the deserializer in the context of having a fuzzer produce random bytes which we try to deserialize into a graph, but I don't follow why it's necessary in the serializer.

💬 TheCharlatan commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2207195276)
I checked if there is anything we could do in the testing setup to make the test faster. With this patchset, the test takes around 1:15 min for 10k runs on my machine. Removing the validation interface from the test setup cuts it down to 1:05 min , if I remove the networking setup, it takes 35 sec to complete.

I also removed the mempool and it crashed when reaching validation.cpp:5684. Is it a hard requirement when using assumeutxo that a mempool exists, or is this a bug?

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1664822525)
Yes, it is needed, for the simple reason that the compressor needs to predict what the decompressor state will be, or they will go out of sync. If they disagree about which dependencies are possible, the delta-encoded differences between them will be off, resulting in possibly widely different DepGraphs on decoding. An example where this matters is in the 4th unit test (specifically constructed for that, suggested by @instagibbs).

I am considering a simplification for the encoding format howe
...

🤔 furszy reviewed a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2157536398)
> In ApplyMigrationData, it looks like we're creating batchs for the watchonly and solvable wallets multiple times. Those could be consolidated in this PR as well.

Done. Thanks.

💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1664854646)
Done

💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1664855357)
Done