💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664558577)
Added `noexcept` everywhere.

💬 ajtowns commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2206898641)
ACK df2422c5f91e5b1b42d6b718cc2527dc413712fc ; looks like it behaves correctly to me

💬 stickies-v commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1664572723)
Is there a reason this can't be simplified to:

```suggestion
if (nHeight % consensusParams.DifficultyAdjustmentInterval() == 0) {
```

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664545575)
what happens if this ends up throwing, it's not a trivial function (e.g. `m_prev` could become nullable in the future and cause havoc)?

🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2157030164)
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?

```bash
sudo ./src/bitcoind -stopatheight=200000

sudo hyperfine \
--warmup 1 --runs 5 \
--show-output \
--parameter-list commit master,670084adc53e3d661ea5b4a19743659609a42d5e \
--prepare 'git checkout {commit} && git reset --hard && make -j10
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1664560161)
maybe it could make sense to document these rare conditions with something like `} else if ([[unlikely]] fOk) {`

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