💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393168493)
Nice, this code is _much_ more straightforward now.

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393167335)
Same comment about default bool.

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393193011)
If version is no longer used for fileout in these ops, can't we `s/CAutoFile/AutoFile/g` ?

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393187848)
I guess this makes more sense than defining witness-ness for an empty block, but just to clarify...

This is a no-op and would've been a no-op independent of this PR as well, right?

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393181788)
Beautiful :)

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393251131)
I realize this isn't new, but it's pretty icky to go to `gArgs` for something as low-level as serialization. Makes me wonder if we're doing that in any tight loops. Nothing to do with this PR, but it be nice to cache this in a local bool.

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393249080)
These can be a `DataStream`s now?

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393197401)
Again, so much more clear. Nice.

I think we could probably drop `version.h` here now?

Edit: Oooh, we can drop it from some deep internal headers even (`hash.h` and `consensus/validation.h`). See https://github.com/theuni/bitcoin/commit/9986da90f604209c42dcc65a47455ea23b2f7a5a . I think I'd prefer to see that added to this PR so that we don't put it off.

💬 jonatack commented on pull request "[26.x] rc3 or finalize":
(https://github.com/bitcoin/bitcoin/pull/28872#issuecomment-1811250489)
Yes, the first suggested one in https://github.com/bitcoin/bitcoin/pull/28155#issuecomment-1785999512, i.e. https://github.com/bitcoin/bitcoin/commit/94e8882d820969ddc83f24f4cbe1515a886da4ea, is a bug fix.

Am updating https://github.com/bitcoin/bitcoin/pull/28248 to punt for later on two of the seven, in favor of the clearest ones and the regression.

👍 hebasto approved a pull request: "depends: remove `PYTHONPATH` from config.site"
(https://github.com/bitcoin/bitcoin/pull/28845#pullrequestreview-1730819522)
ACK 3b19100303e70315c31d7aa3a3fd38d58131246f, this PR effectively reverts no longer needed de619a37fd18a17225c8a10b828fc61958abe4cf.

My Guix builds:
```
x86_64
bf25f00a359238ccb479a780d3feca454a91503d11fafb5250bfba382451349f guix-build-3b19100303e7/output/aarch64-linux-gnu/SHA256SUMS.part
47a456af1b2fbd4233c7feb97a42313908e235a587991f654aa8fadc0e517a0d guix-build-3b19100303e7/output/aarch64-linux-gnu/bitcoin-3b19100303e7-aarch64-linux-gnu-debug.tar.gz
cf7007c15990a96c8a32155b6b96f9a1da
...

💬 theuni commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1811350334)
Btw, the `version.h` suggesed change above, while large, can be trivially reviewed using `git diff --stat`, as `#include <version.h>` is either added or removed in one line and that's it.

📝 hebasto converted_to_draft a pull request: "build: Pass sanitize flags to instrument `libsecp256k1` code"
(https://github.com/bitcoin/bitcoin/pull/28875)
This PR is a revived https://github.com/bitcoin/bitcoin/pull/27991 with an addressed [comment](https://github.com/bitcoin/bitcoin/pull/27991#discussion_r1252148488).

Fixes https://github.com/bitcoin/bitcoin/issues/27990.

Might be tested as follows:
```
$ ./autogen.sh && ./configure --enable-fuzz --with-sanitizers=fuzzer CC=clang-13 CXX=clang++-13
$ make clean > /dev/null && make
$ objdump --disassemble=secp256k1_xonly_pubkey_serialize src/test/fuzz/fuzz | grep __sanitizer_cov
1953bd0
...

💬 hebasto commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#issuecomment-1811514658)
Converted to a draft, as I'm not sure how to suppress new UBSan reports.

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393533653)
That argument is only needed to support `-rpcserialversion`, which is deprecated as of #28448, so presumably can/will be pulled out later.

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393535412)
Yes -- serializing an empty vector is just `WriteCompactSize(s, 0)` no matter the elements that would have been in the vector, but to serialize a vector of blocks (even an empty one) you need to decide what to do about witness data (even though there is no witness data in an empty block), whereas you don't need to do that for block headers.

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393542569)
Might be better to do that after dropping version from `GetSerializeSize` ? see https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378757410

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393566128)
None of the psbt bits can switch to `DataStream` until `CVectorWriter` is updated.

📝 ajtowns opened a pull request: "Remove version field from GetSerializeSize"
(https://github.com/bitcoin/bitcoin/pull/28878)
Depends on #28438.

Drops the version field from `GetSerializeSize()`, simplifying the code in various places. Also drop `GetSerializeSizeMany()` (as just removing the version parameter could result in silent bugs) and remove unnecessary instances of `#include <version.h>`.

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1393569854)
See #28878. That seem okay?

💬 BenWestgate commented on issue "MuSig2 support":
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1811731481)

> Is it possible to support descriptors that include only the "aggregated xpub" and not every key as in the proposed `agg()` expression, leading to a O(1) size instead of O(number-of-keys)?

You should be able to use Shamir secret sharing between the private key data so that a spending threshold can recover all pubkey data needed to reconstruct the agg() expression.