💬 fanquake commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2079415082)
This is an interesting bug

💬 laanwj commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#issuecomment-2862518703)
> This probably applies to testnet and regtest too.

Yes, for quick testing, not having to use an external script to create credentials is more convenient too. Usually cookie-based auth will suffice, but not always when dealing with external applications that need a username/password.

💬 fanquake commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2862534081)
No movement in #31507, but could rebase & update this with the one new commit?

👍 rkrux approved a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2824665753)
re-ACK f09e1f9

```
git range-diff b6101b1...f09e1f9
```
Range diff contains changes from [this review](https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2818667962) & rebase now that #28710 is merged that has led to the removal of `LegacyScriptPubKeyMan` and naturally all the changes in it from this PR - the overall diff has become marginally smaller.

💬 fanquake commented on pull request "build: simplify *ifaddr handling":
(https://github.com/bitcoin/bitcoin/pull/32446#issuecomment-2862600446)
Guix Build:
```bash
876f676d8a0f348d8a8146b05d9206e8dffbd050269e27fcc41cf09273dfbc51 guix-build-c983612fb43b/output/aarch64-linux-gnu/SHA256SUMS.part
f47bfe426d480a0b7229be7b99f588bad1a5073cc9415cc99f62dd1aac560acf guix-build-c983612fb43b/output/aarch64-linux-gnu/bitcoin-c983612fb43b-aarch64-linux-gnu-debug.tar.gz
cda94cfa1ed1db52b077744fbaed9f4cd4f9c732035c5058634bdf36a4f5fa3d guix-build-c983612fb43b/output/aarch64-linux-gnu/bitcoin-c983612fb43b-aarch64-linux-gnu.tar.gz
f80800376c204970
...

👍 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).