👍 BrandonOdiwuor approved a pull request: "script, assumeutxo: Enhance validations in utxo_snapshot.sh"
(https://github.com/bitcoin/bitcoin/pull/28852#pullrequestreview-1741060818)
tested ACk 11b7269d83a56f919f9dddb7f7c72a96d428627f

1. Successfully tested error handling added and chainstate can be automatically recovered if an error occurs e.g when the snapshot file already exists or user interruption by `ctrl + c`
2. Successfully tested the option to enable or disable network activity

💬 maflcko commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1820392508)
lgtm ACK 47e5c9994c087d1826ccc0d539e916845b5648fb 🏓

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 47e5c9994c087d18
...

💬 maflcko commented on pull request "lint: Report all lint errors instead of early exit":
(https://github.com/bitcoin/bitcoin/pull/28862#issuecomment-1820396967)
> tested on my own fork and saw multiple lint errors

Yeah, I guess linters should enforce their rules globally, instead of only on the diff. Otherwise, one gets different results, depending on how the diff is calculated.

Though, this is unrelated to this pull request and can be improved in the future, if there is need and interest.

💬 maflcko commented on pull request "multiprocess: Add basic type conversion hooks":
(https://github.com/bitcoin/bitcoin/pull/28921#discussion_r1400158381)
```suggestion
DataStream stream{};
```

The values are ignored either way, so it seems better to remove them. `CDataStream` will go away anyway, soon.

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#issuecomment-1820479487)
Concept ACK

You could also add a `oldest_timestamp` (or `birth`) field to `getwalletinfo` which would contain the birth date.

🤔 Sjors reviewed a pull request: "wallet: birth time update during tx scanning"
(https://github.com/bitcoin/bitcoin/pull/28920#pullrequestreview-1741418506)
I checked that the test fails if you drop everything but 134960c3828abe658d565af52e3c6c5af03019f2.

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1400220704)
4bfb243ac41cf1396cf55f8b24de2ba52186c7a4: alternative name: `MaybeUpdateBirthTime( )`

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1400226671)
4bfb243ac41cf1396cf55f8b24de2ba52186c7a4: this seems simpler:

```cpp
TryUpdateBirthTime(spkm_man->GetTimeFirstKey());
m_spk_managers[id] = std::move(spkm_man);
```

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1400224214)
4bfb243ac41cf1396cf55f8b24de2ba52186c7a4: I'm a bit puzzled why this function ever accepted an argument that it didn't use.

💬 Sjors commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1400248376)
4bfb243ac41cf1396cf55f8b24de2ba52186c7a4: you can use `std::placeholders::_1` here, by dropping `this` from `NotifyFirstKeyTimeChanged` calls and changing:

```diff
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -255,11 +255,11 @@ public:

/** Keypool has new keys */
boost::signals2::signal<void ()> NotifyCanGetAddressesChanged;

/** Birth time changed */
- boost::signals2::signal<void (const ScriptPubKeyMan* spkm, int64_t new_birth_time)>
...

💬 Sjors commented on pull request "doc: Add release note for coinstatsindex in pruned mode":
(https://github.com/bitcoin/bitcoin/pull/28909#issuecomment-1820535005)
Changing the historical release notes won't change the release notes that come with the actual v24.* binaries, so this won't work.

The v24 release will also be [end-of-life](https://bitcoincore.org/en/lifecycle/) as soon as we ship v26.0.

✅ maflcko closed a pull request: "doc: Add release note for coinstatsindex in pruned mode"
(https://github.com/bitcoin/bitcoin/pull/28909)

💬 Sjors commented on pull request "Drop CAutoFile":
(https://github.com/bitcoin/bitcoin/pull/28904#issuecomment-1820585297)
Looks like a nice cleanup, but I'm not familiar with the history of `CAutoFile` -> `AutoFile`. I did check that tests pass for all intermediate commits.

💬 maflcko commented on pull request "doc: Add release note for coinstatsindex in pruned mode":
(https://github.com/bitcoin/bitcoin/pull/28909#issuecomment-1820586422)
Closing for now, per previous comment

💬 maflcko commented on pull request "Drop CAutoFile":
(https://github.com/bitcoin/bitcoin/pull/28904#issuecomment-1820609651)
> Looks like a nice cleanup, but I'm not familiar with the history of `CAutoFile` -> `AutoFile`.

serialization flags embedded in the version-integer and type-integer are gone, serialization now uses params-serialization, so the ser-version and ser-type code can be deleted.

👍 Sjors approved a pull request: "wallet: batch all individual spkms setup db writes in a single db txn"
(https://github.com/bitcoin/bitcoin/pull/28894#pullrequestreview-1741582965)
ACK 1901360ca52c6563a40dc78633cc492a822ac7a9

Not sure if b85b75d6470841813c5844b2e9c883360f1ad94a is needed since we're getting rid of the legacy wallet anyway. But it looks correct.

Ran bench on MacBook Pro 2019 (2,3 GHz 8-Core Intel Core i9, SSD):

Before:

```
src/bench/bench_bitcoin -filter="WalletCreate[E|P].*" -min-time=30000
```

| ns/op | op/s | err% | total | benchmark
|--------------------:|-------------------
...

💬 Sjors commented on pull request "wallet: batch all individual spkms setup db writes in a single db txn":
(https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1400325124)
da133a80bae311373337a0d9c474667a5f73908d nit: `TopUpWithDB`

💬 Sjors commented on pull request "wallet: batch all individual spkms setup db writes in a single db txn":
(https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1400342212)
1901360ca52c6563a40dc78633cc492a822ac7a9: nit "is committed to disk"

💬 dergoegge commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1820666311)
I think this is rfm. Could a maintainer take a look?

✅ hebasto closed an issue: "ci: failure in Windows MSVC build"
(https://github.com/bitcoin/bitcoin/issues/28901)

🚀 hebasto merged a pull request: "ci: Avoid toolset ambiguity that MSVC can't handle"
(https://github.com/bitcoin/bitcoin/pull/28905)