👍 rkrux approved a pull request: "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor"
(https://github.com/bitcoin/bitcoin/pull/32344#pullrequestreview-2824722022)
ACK 97d383a

💬 rkrux commented on pull request "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor":
(https://github.com/bitcoin/bitcoin/pull/32344#discussion_r2079473302)
Nice, I did consider removing the loop in the previous review but decided against it so as to simulate a similar flow like this one in a loop: https://github.com/bitcoin/bitcoin/blob/64697529523705f5a411ca893060429afcd5cd47/src/wallet/wallet.cpp#L4131-L4132

However, I did check now that in `GetDescriptorScriptPubKeyMan` the `ScriptPubKeyMan` is gotten via `descriptor.id`, which would remain same whether the `w_desc` reference is same or not.
https://github.com/bitcoin/bitcoin/blob/6469752952
...

💬 purpleKarrot commented on issue "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install":
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2862620147)
>> We may do that with a [dependency provider](https://cmake.org/cmake/help/latest/command/cmake_language.html#set-dependency-provider). This will put find_package() completely under our control, so restricting CMake's builtin find logic becomes unnecessary.
>
> Does this require a minimum CMake version of 3.24?

Yes, but it does not require updating the minimum required version of the project. That means building with system packages would be possible with version 3.22, while building with depe
...

💬 fanquake commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#discussion_r2079487982)
Added the `UTC` option.

💬 fanquake commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2862624796)
> It would be useful to mention in the commit message / PR description a note from the CMake docs:

Added the relevant docs to the commit message.

👋 fanquake's pull request is ready for review: "build: let CMake determine the year"
(https://github.com/bitcoin/bitcoin/pull/32445)

📝 laanwj opened a pull request: "[RFC] dbwrapper: Set global leveldb mmap limit"
(https://github.com/bitcoin/bitcoin/pull/32447)
Set the default leveldb mmap limit to 4096 files from dbwrapper, before creating the first leveldb context.

The motivation here is to remove the need for a custom leveldb patch, see google/leveldb#1265.

After looking into this i'm not as sure whether this is what we want to do:

- The interface here is really ludicrous.
- Why is 4096 a good number?
- After #30039, the number of ldb files created is 16 times smaller. 1000 files with the new default of 32MB is 32GB of database. Maybe the
...

💬 rkrux commented on pull request "refactor: Removals after bdb removal":
(https://github.com/bitcoin/bitcoin/pull/32438#discussion_r2079507501)
Indeed, the second node here is v28 and the `wallet_migration` test fails in the CI with this error:

```python
test_framework.authproxy.JSONRPCException: BDB wallet creation is deprecated and will be removed in a future release. In this release it can be re-enabled temporarily with the -deprecatedrpc=create_bdb setting. (-4)
```

💬 laanwj commented on pull request "[RFC] dbwrapper: Set global leveldb mmap limit":
(https://github.com/bitcoin/bitcoin/pull/32447#issuecomment-2862682521)
Yeah no even "when to call this initialization" isn't clear, this breaks the tests as-is.

✅ laanwj closed a pull request: "[RFC] dbwrapper: Set global leveldb mmap limit"
(https://github.com/bitcoin/bitcoin/pull/32447)

💬 purpleKarrot commented on pull request "depends: Avoid using helper variables in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2862686579)
Ack 7343a1846cebf74ffef3c54e05b633df629510a1

I am not particularly happy with logic in toolchain files. Removing helper variables is a step in the right direction.

💬 laanwj commented on pull request "[RFC] dbwrapper: Set global leveldb mmap limit":
(https://github.com/bitcoin/bitcoin/pull/32447#issuecomment-2862713589)
See bitcoin-core/leveldb-subtree#52 instead.

🤔 janb84 reviewed a pull request: "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory"
(https://github.com/bitcoin/bitcoin/pull/32423#pullrequestreview-2824869529)
tACK [3acfc07](https://github.com/bitcoin/bitcoin/commits/3acfc071c3445e943069b2778bbc5c74f702ef36)

- code review ✅
- compiled, run all tests ✅
- tested on regtest with user/pass & did remote curl call to test rpc user&pass func. ✅

💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2862815509)
The feedback from @fanquake has been addressed.

💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2079588558)
> I think for now we could just assert `binary.resources_manager.has_manifest`, if we care that this is being added? Could save anything else for if/when it's actually being used?

Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2862815509).

💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2079589157)
> > What alternatives are you suggesting for configuring the Developer Command Prompt?
>
> Directly running the command that somebody who owns a Windows computer would run, that doesn't require installing NPM, and then executing somebody else's JavaScript.

Thanks. [Reworked](https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2862815509).

💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2079591061)
I can add a note if I touch the PR again

💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2079591354)
Leaving as-is for now

👍 hebasto approved a pull request: "build: let CMake determine the year"
(https://github.com/bitcoin/bitcoin/pull/32445#pullrequestreview-2824945378)
re-ACK dfeac76b951fc2d28e0832b176f3c8ad595697f1.

💬 janb84 commented on pull request "doc: swap "Docker image" for "container image"":
(https://github.com/bitcoin/bitcoin/pull/32444#issuecomment-2862868108)
ACK https://github.com/bitcoin/bitcoin/commit/1372eb09c5d031315bc6cde34f7a15297e96f4cc