📝 ryanofsky opened a pull request: "multiprocess: Add basic type conversion hoois"
(https://github.com/bitcoin/bitcoin/pull/28921)
Add type conversion hooks to allow `UniValue` objects, and objects that have `CDataStream` `Serialize` and `Unserialize` methods to be used as arguments and return values in Cap'nProto interface methods. Also add unit test to verify the hooks are working and data can be round-tripped correctly.

The non-test code in this PR was previously part of #10102 and has been split off for easier review, but the test code is new.

---

This PR is part of the [process separation project](https://gith
...

💬 brunoerg commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819906879)
> I don't think that hide issues under the carpet is a good path to follow.

Correct me if I'm wrong. In https://github.com/bitcoin/bitcoin/pull/28395#pullrequestreview-1651973742, isn't the solution that we will adopt to disable BnB when SFFO is enabled?

💬 kevkevinpal commented on pull request "bugfix: throw an error if an invalid parameter is passed to getnetworkhashps RPC":
(https://github.com/bitcoin/bitcoin/pull/28554#issuecomment-1819926688)
reACK [9ac114e](https://github.com/bitcoin/bitcoin/pull/28554/commits/9ac114e5cd9d8ade3a1d9f3d76a08ff59a3f1658)

💬 furszy commented on issue "fuzz, coinselection: Assertion 'result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed":
(https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1819948545)
> > I don't think that hide issues under the carpet is a good path to follow.
>
> Correct me if I'm wrong. In [#28395 (review)](https://github.com/bitcoin/bitcoin/pull/28395#pullrequestreview-1651973742), isn't the solution that we will adopt to disable BnB when SFFO is enabled? I was wondering whether we would end up touching the fuzz doing that.

Yes. We need to tackle both aspects in the same PR (wallet and fuzz test). Skipping BnB only in the test side would hide the unexpected behavior
...

💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1399855267)
One possible problem I encountered when running the `p2p_invalid_messages.py` `test_resource_exhaustion` subtest with v2 that but could affect many tests intermittently, also this spot:

I think that there could be a race between the python main thread of functional tests and the p2p thread:
For example, this command 1) builds messages, 2) encrypts them and 3) sends them, all within the main thread.
If during step 2) the bitcoind node would decide to send out a ping, the p2p thread will also
...

👍 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
|--------------------:|-------------------
...