🤔 Sjors reviewed a pull request: "Implement BIP 370 PSBTv2"
(https://github.com/bitcoin/bitcoin/pull/21283#pullrequestreview-2601346881)
Studied the other commits as well.

Perhaps there's an opportunity to split this PR by first introducing the internal refactoring that anticipates PSBTv2. But other that I don't think it can be made much smaller.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946295044)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", nit: calling this param `psbt_version` makes it clear that inputs and outputs don't have their own versioning. And consistent with the eventual variable name.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946298433)
In https://github.com/bitcoin/bitcoin/commit/2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", μnit `const`.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946310826)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version": somewhere later in the PR, we should add a target for version 2?

Or better: any version, which would detect that the `PSBTInput` constructor doesn't prevent version 1 (not currently a problem, but could creep in if we build functionality to add inputs to a PSBT).

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946376405)
In c6743bf7137615eb20145c6eb3e2da5c82903b1e "Enforce PSBT version constraints": this is hard to read. Would prefer two separate checks based on the PSBT version.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946305833)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", nit: `/*psbt_version=*/` would be useful here, but this goes away in a later commit (and before the default is changed to 2).

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946391356)
In 0c459e43ba1ce74d2e88dde352e28a63743f4746 "Add PSBT::CacheUnsignedTxPieces": this seems fine, but it would be good to have a method that verifies that for v0 PSBTs these two representations are in sync. You could then enable that method in debug builds and call it in a bunch of places.

The could e.g call `GetUnsignedTx()` (introduced later), skip its `if (tx != std::nullopt)` early return, and compare.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946401139)
In 162d0916774c719f1e3749b0d4d204e296ee4ac8 "Add PSBT::GetUniqueID": I think this should check the version number, and then `Assume(version == 0 || tx == std::nullopt)`

The way it's written now suggests `tx` is just a cache that might be stale.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946413388)
In aeb9e44bc5a221553d2010752c76f8f16205551c "Change PSBT::AddInput to take just PSBTInput": I think it's more readable to just check the PSBT version directly.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946386141)
In cef9de00f4c2d4c9c033e7dce2b7ffac2e694a26 "Call CacheUnsignedTxPieces in PSBT constructor": might as well do this in the previous commit. Ditto for the next commit.

⚠️ jsarenik opened an issue: "Memory-only wallet (for faucets)"
(https://github.com/bitcoin/bitcoin/issues/31816)
### Please describe the feature you'd like to see added.

Currently I do following regularly every couple of days in order to refresh faucet wallets and reclaim megabytes of disk space:

```sh
cd ~/.bitcoin/signet/wallets
bitcoin-cli -signet createwallet name false true
# blank with private-keys enabled
cd name
dt=../listdescriptors-true.txt
A=$(sed 's/"timestamp".*$/"timestamp":"now",/' $dt | jq -rc .descriptors)
bitcoin-cli importdescriptors $A
```

It works well for [me](https://alt.signetfau
...

💬 maflcko commented on pull request "ci: Bump fuzz task timeout":
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642789552)
Interesting. For context, the "speedup" after Jan 12 is likely due to fuzz input invalidation after commit 37af8bfb34d685bfd5d9a9ba6b35b4705e021535. And the "slowdown" after Dec 13th is likely due to adding fuzz inputs after commit https://github.com/bitcoin-core/qa-assets/commit/5c026e2a97b5f447a863734fb1c2566c33738d01.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946505928)
Maybe it is worth doing this: https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2485715952. It is a super good change and normally I would do it, but the reason I didn't is that it will increase the size of this PR which, I am afraid, would turn reviewers away.

I will proceed to other suggestions and give this some thought...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946520524)
Done, but used `uint8_t` because `ReceiveMsgBytes()` (in `master`) takes a span of that and conversion is not possible.

💬 maflcko commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946527624)
> conversion is not possible.

It should be possible to convert std::byte* to uint8_t* (and vice-versa). The two are the almost the same anyway (https://en.cppreference.com/w/cpp/types/byte). And a span is just a pointer+size, so conversion between the two span types should also be possible.

💬 dergoegge commented on pull request "ci: Bump fuzz task timeout":
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642944794)
Something we could also do is stop using libfuzzer's instrumentation in that task, as it makes things slower too. We aren't using instrumentation in the other fuzz jobs either.

We could still compile with libfuzzer but disable the instrumentation with `-fsanitize-coverage-allowlist=allowlist.txt` where the allow list is empty (i think).

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946537723)
That would make `addr_to` a pointer, so `*addr_to` would have to be used in the 6 places below. I slightly prefer the current "if holds alternative CService" because it is dump and easy to follow. Leaving it as it is.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946544583)
Yeah, `EventNewConnectionAccepted()` is generic used also for non-I2P connections and the flip will not do anything on I2P addresses.

> But maybe you should add a Assume(IsI2P(conn.peer));

In which function?

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946547841)
Removed!

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946550609)
Done.