💬 hebasto commented on pull request "build: remove `--enable-lcov-branch-coverage`":
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2151924530)
CMake's https://github.com/hebasto/bitcoin/pull/191 is built on top of this PR change.

🤔 hebasto reviewed a pull request: "build: re-enable deprecated warning copy"
(https://github.com/bitcoin/bitcoin/pull/30236#pullrequestreview-2101568286)
Post-merge ACK c3a5e8a0639ff2505adb4a4e7776db87d5ebafd3.

💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151947313)
> > An alternative/addition would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)
>
> Can you clarify how this PR is related to C++23? I don't see CMake as a blocker, if that's something we want to do.

Ah sorry, looks like they didn't change anything about the deprecation in C++23 and it is kept as-is for now.

However, I found that this patch seems incomplete. For example, no warning is printed to catch https://eel.is/c++draft/depr.
...

💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151951796)
For reference https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2139r2.html#3.8 said:

> Strong recommendation: take decisive action early in the C++23 development cycle, so early adopters can shake out the remaining cost of updating old code. Note that this would be both an API and ABI breaking change, and it would be good to set that precedent early if we wish to allow such breakage in C++23.

But that didn't happen.

💬 hebasto commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151952891)
CMake includes dependencies with `-isystem`, which is equivalent to `suppress_external_warnings = yes`. Therefore, the block removed in this PR has never been a part of the CMake staging branch.

💬 fanquake commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151955730)
> unless -Wdeprecated-copy-dtor is passed.

I think we can turn on additional warning flags, if you think that is valuable.

> CMake includes dependencies with -isystem, which is equivalent to suppress_external_warnings = yes.

We may still need a way to disable that functionality, so all warnings can be observed, so I think this block is still relevant.

💬 fanquake commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151958943)
See comments starting here: https://github.com/bitcoin/bitcoin/pull/30189#issuecomment-2137467259. cc @maflcko.

💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151968101)
> > unless -Wdeprecated-copy-dtor is passed.
>
> I think we can turn on additional warning flags, if you think that is valuable.

Yeah, not sure how useful that'd be, given that the deprecation apparently won't result in a deletion (for now).



> > CMake includes dependencies with -isystem, which is equivalent to suppress_external_warnings = yes.
>
> We may still need a way to disable that functionality, so all warnings can be observed, so I think this block (or atleast the function
...

💬 fanquake commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2152016135)
> Yeah, could open a tracking issue in the cmake repo (hebasto/bitcoin)?

Opened one here so it isn't forgotten: https://github.com/hebasto/bitcoin/issues/223.

💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629164462)
```suggestion
// Implements the full legacy wallet behavior
// Slated for deletion once Berkeley DB library dependency is removed.
class LegacyScriptPubKeyMan : public LegacyDataSPKM
```

💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629293058)
Haven't fully grokked the code yet, so I understand if you'd rather not answer my questions, but if you want:

> I don't think this needs to have any additional explanation. keypool_size only matters for ranged descriptors. Non-HD keys do not make ranged descriptors, so the value here is ignored.

We're in an HD chain `for`-loop, how is this not involving HD keys?
Surely legacy wallets could be HD wallets.

> The loop below is for an entirely different set of descriptors and is unrelated
...

💬 maflcko commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1629294986)
This should also check that the background validation succeeds. Otherwise there could be a bug where the diverging chain is not rewound?

📝 AngusP opened a pull request: "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`"
(https://github.com/bitcoin/bitcoin/pull/30237)
This test uses the `extra_txn` (`vExtraTxnForCompact`) vector of optional orphan/conflicted/etc. transactions to provide transactions to a PartiallyDownloadedBlock that are not otherwise present in the mempool, and check that they are used.

This also covers a former nullptr deref bug that was fixed in #29752 (bf031a517c79cec5b43420bcd40291ab0e9f68a8) where the `extra_txn` vec/circular-buffer was null-initialized and not yet filled when dereferenced in `PartiallyDownloadedBlock::InitData`.

👍 hebasto approved a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2101695692)
re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4.

My Guix build:
```
x86_64
f33ae90e6d190245d34712f544cd458f282a2c90890029dfbd88d74e8823fb50 guix-build-c7376babd19d/output/aarch64-linux-gnu/SHA256SUMS.part
ee444d54e0b9cebfa6ab0c91fc8b543d414a98338d46d299f3b302273139c81f guix-build-c7376babd19d/output/aarch64-linux-gnu/bitcoin-c7376babd19d-aarch64-linux-gnu-debug.tar.gz
fdc7400c04fd6bfec5d0ab3148b0f4da110974ecda0859184adad29f38422f6f guix-build-c7376babd19d/output/aarch64-linux-gnu/bitc
...

💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2152172999)
Hi all,
an early alpha of the Ledger Bitcoin Testnet app with MuSig2 support is available for testing. (NB: the app is called `Bitcoin Test Musig` and not `Bitcoin Test`). It should be compatible with the [latest draft of the specs](https://github.com/bitcoin/bips/pull/1540).

Instructions and an easy end-2-end script for MuSig signing to play with it is available here for anyone interested in trying it out:

https://github.com/bigspider/moosig

It works for both keypath and script path s
...

💬 rocholojavinar commented on issue "Make Transport independent of CNetMessage and CSerializedNetMsg":
(https://github.com/bitcoin/bitcoin/issues/30209#issuecomment-2152186980)

💬 glozow commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1629407063)
Sure, can be in another PR

🤔 glozow reviewed a pull request: "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2101775625)
rebased

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1629413337)
fixed, thanks

💬 hebasto commented on pull request "feefrac: 128-bit multiply support in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29758#issuecomment-2152268436)
@sipa
> Would you mind benchmarking with smaller realistic numbers (say fee and size both between 0 and 2^30)?

I switched to `FastRandomContext::randbits(30)`.

> If the top limb is equal, it's possible the naive code does worse.

Numbers have not changed:
- the master branch:
```
> build_msvc\x64\Release\bench_bitcoin.exe -filter=FeefracMultipication

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