💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2348456444)
In https://github.com/bitcoin/bitcoin/commit/10b7148efa754169cc625e93c98fa54f7b375e5d "sign: Add CreateMuSig2PartialSig"

Might be opinionated:
I think having a `struct` for the `pubnonce` can add some structure in the overall MuSig signing code. The `66` size checks spread across `CreateMuSig2PartialSig` & `CreateMuSig2AggregateSig` functions seem distracting and IMO can go inside the constructor of the struct that internally can use `secp256k1_musig_pubnonce`.
At the moment, treatment of
...

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2348311867)
In 6b01d7379b9e29760abd7b549a04db43937a7b8d "sign: Add CreateMuSig2Nonce"

Can consider creating the constant now in the `musig.h` dedicated file, even though there is no pubnonce object yet - ref https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2049062217

```diff
- // Serialize nonce
+ // Serialize pubnonce
std::vector<uint8_t> out;
- out.resize(66);
+ out.resize(MUSIG2_PUBNONCE_SIZE);
```

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2348997742)
In 10b7148efa754169cc625e93c98fa54f7b375e5d "sign: Add CreateMuSig2PartialSig"

Same `s/hash/sighash` suggestion in both `key.cpp` and `key.h`.

💬 Crypt-iQ commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3292557230)
ACK 7f87cddd36f0456be52d795a53887d53294f824e

💬 purpleKarrot commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3292587091)
> Throwing an exception from the destructor of a class is a bad practice because the destructor will be called when an object of that type is alive on the stack and another exception is thrown, which will result in "exception during the exception". This would terminate the program without any messages.

This is not precisely what happens. If an exception is emitted from a function marked `noexcept(false)`, the current terminate handler will be invoked. It does not matter whether the destructor
...

🤔 Eunovo reviewed a pull request: "test: Prevent disk space warning during node_init_tests"
(https://github.com/bitcoin/bitcoin/pull/33391#pullrequestreview-3225126615)
Tested ACK https://github.com/bitcoin/bitcoin/pull/33391/commits/bdf01c6f61262cd6e211ead3c0dbc66ccb48b32f:

The message does not appear when you build https://github.com/bitcoin/bitcoin/pull/33391/commits/bdf01c6f61262cd6e211ead3c0dbc66ccb48b32f and run the unit tests, but it appears on master.

Since the test is focused on initialisation and shutdown logic, the effect of using REGTEST chaintype here is minimal.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3292695737)
Thanks for the review @stringintech

Updated e450549ae94736fc727d0d8a4a7a6a80e768ecd2 -> 0fc068b735d267c7ef4a3b23e32dab1771df2509 ([kernelApi_64](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_64) -> [kernelApi_65](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_65), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_64..kernelApi_65))

* Addressed @stringintech's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2346797667), removed asserti
...

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3292821103)
> Updated the map to that of the latest run: [asmap/asmap-data#30](https://github.com/asmap/asmap-data/pull/30)

FYI for reviewers, new maps are available in [asmap-data](https://github.com/asmap/asmap-data/) and will continue to be updated over the next few months but I don't plan to update the map data that is included here for at least a while. This is by choice: If this had been merged for v30 then this map data might have been included and it would have been the default until the release
...

💬 quapka commented on issue "BIP32: Maximal length of derivation paths and depth of extended keys":
(https://github.com/bitcoin/bitcoin/issues/32201#issuecomment-3292848589)
Linking https://github.com/bitcoinj/bitcoinj/issues/3683, to keep things in one place.

💬 janb84 commented on pull request "doc: Add documentation explaining different build types":
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3292868065)
Please squash your commits, like described in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 Sjors commented on pull request "test: Add submitblock test in interface_ipc":
(https://github.com/bitcoin/bitcoin/pull/33380#issuecomment-3292933671)
ACK 0a26731c4cc182e887ce959cdd301227cdc752d7

📝 musaHaruna opened a pull request: "rpc: add scan_utxoset option to getbalance(s) to verify wallet balance accuracy"
(https://github.com/bitcoin/bitcoin/pull/33392)
Fixes [#28898](https://github.com/bitcoin/bitcoin/issues/28898)
Users who import descriptors with incorrect birthdates may encounter inaccurate wallet balances. In such cases, the wallet may miss historical transactions that belong to it, resulting in underreported balances.
Currently, the only way to correct this is to manually run rescanblockchain or reimport descriptors with proper metadata. However, users may not realize that their wallet balance is incomplete.
This PR introduces a mechan
...

💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2349501608)
> But one thought is that maybe serializing whole block is not necessary, and this could just return `CBlockHeader`?

I think it's easier to use if we just return the block. Otherwise a client has the call `getBlock()`, then `applySolution()` and then swap out the header in the block they got. In which case they might as well just apply the solution themselves.

I think the two main use cases for this are:
1. Testing mining (pool) software, which doesn't need to be performant
2. Debugging
...

💬 D33r-Gee commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3293023636)
With [30d9bdb](https://github.com/bitcoin/bitcoin/commit/30d9bdb77324d234d4aacd155075b4dfc00930c7), the `loadtxoutset` RPC command has been refactored to use the snapshot interface, as suggested by @Sjors. Additionally, the interface now includes `getMetadata` and `getActivationResult` methods to ensure parity with the legacy RPC command.

💬 D33r-Gee commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#discussion_r2349555648)
> a good way to add test coverage to this

refactored `loadtxoutset` to utilize the new `snapshot` interface...

⚠️ Rev-9T opened an issue: "v30 Testing (BUG)"
(https://github.com/bitcoin/bitcoin/issues/33393)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

1. It has a very serious vulnerability by defaulting op_return to 100,00 bytes.
2. No op_return customisation options.

### Expected behaviour

op_return should be no more than 42 bytes.

### Steps to reproduce

Updated the `bitcoin.conf` file
Added `datacarriersize=42`

### Relevant log output

_No response_

### How did you obtain Bitcoin Core

Compiled from source

### What version of
...

💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2349650823)
thanks 👍

💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2349653359)
I don't think that's useful. The test is not much longer than others and `check_tx_counts` is separated in a function because it's called multiple times with different values of `final`. In this case there's no duplicated code.

💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#issuecomment-3293196760)
21fb4c949cebf3510dad3dc3827ccb9451e4828d correct typos

💬 ryanofsky commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3293213987)
Cap'n Proto has a policy of declaring destructors `noexcept(false)` and using `std::uncaught_exception()` to prevent throwing more than once under the assumption that "ff another exception is already active, the new exception is assumed to be a side-effect of the main exception, and is either silently swallowed or reported on a side channel." It seems like that could be a reasonable approach here. The DebugLogHelper destructor could continue to throw normally, but just log if another exception w
...