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

💬 TheCharlatan commented on pull request "build: Fix regression in "ARMv8 CRC32 intrinsics" test":
(https://github.com/bitcoin/bitcoin/pull/28919#issuecomment-1820742685)
ACK 228d6a2969e4fcee573c9df7aad31550eab9c8d4

I checkd that the bianry now contains both crc and crypto intrinsic opcodes.

Guix build (x86_64 and aarch64):
```
2afd81f540c6d3b36ff305e88bafe935e4272cd3efef3130aa69d49a0522541b guix-build-228d6a2969e4/output/aarch64-linux-gnu/SHA256SUMS.part
6c704d6d30d495adb3fb86befdb500eb389a02c1167163f14ab5c3c3e630e6b3 guix-build-228d6a2969e4/output/aarch64-linux-gnu/bitcoin-228d6a2969e4-aarch64-linux-gnu-debug.tar.gz
e4419963c9c0d99adc4e38538900b648f
...

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1820750522)
Rebased now that #28905 is merged.