💬 davidgumberg commented on issue "CI: Cmake warnings should be errors":
(https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-2855503431)
Is it possible that instead it is the case that we have some warnings which should in fact be errors? For example, having a python version below minimum? I'm unclear where people stand conceptually on this after #31593 was merged, fixing the python warning.

🤔 janb84 reviewed a pull request: "docs: clarify RPC credentials security boundary"
(https://github.com/bitcoin/bitcoin/pull/32424#pullrequestreview-2819238684)
ACK [ed86201](https://github.com/bitcoin/bitcoin/pull/32424/commits/ed862012f747d4e5248f08ff25183dc666c3de6e)

The updated documentation provides greater clarity on the security consequences of sharing RPC credentials.

💬 janb84 commented on pull request "docs: clarify RPC credentials security boundary":
(https://github.com/bitcoin/bitcoin/pull/32424#discussion_r2076031813)
```suggestion
the node and the same system privileges as the operating system user account running bitcoind. When
```
Small NIT, but fine to ignore

💬 mzumsande commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2855558032)
I prefer #32406.
While it won't change the outcome if some nodes (or even a moderate majority of nodes) don't accept these transactions due to the high connectivity of the p2p network, it does matter how many miners do.

> These limits are easily bypassed by both direct submission to miner mempools (e.g. MARA Slipstream)

MARA has 5% of the hashrate, and it often makes a difference how long the expected time to mining is - for usability and time-sensitive applications.
If it turns out that
...

💬 maflcko commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2855566064)
lgtm ACK 5155dbd5f8202b7c54b45c53d3cea5e57e8e53ad

My nit about the warning isn't a blocker.

💬 maflcko commented on issue "CI: Cmake warnings should be errors":
(https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-2855566246)
> Is it possible that instead it is the case that we have some warnings which should in fact be errors? For example, having a python version below minimum?

I think this was implemented in https://github.com/bitcoin/bitcoin/pull/31669, but for some reason it was closed.

💬 davidgumberg commented on pull request "docs: clarify RPC credentials security boundary":
(https://github.com/bitcoin/bitcoin/pull/32424#issuecomment-2855566965)
lgtm ACK https://github.com/bitcoin/bitcoin/pull/32424/commits/ed862012f747d4e5248f08ff25183dc666c3de6e

I opened #32274 because I wasn't sure if this status quo is desirable or not, but I believe this documentation note reflects the present expectations for `bitcoind` RPC servers.

💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2855591507)
Rebased to fix merge conflict, this needs conceptual review, it seems to me the main question is: Should it be possible to make config warnings fatal? Or is it the case that any time you want to make a warning fatal, you have a warning that should probably always be an error?

🤔 jamesob reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2819295707)
Concept ACK

Leaving the knob in but changing its default is a good approach.

💬 jamesob commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2076064182)
In a vacuum it might be nice as a `std::optional<unsigned int>`, but that probably causes too much downstream churn.

💬 maflcko commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2076086158)
However, if some people prefer the option to not be deprecated (for whatever reason), it seems pretty minimal maintenance to keep it around without deprecation for now. There is no rush for this pull request, as the 30.x release is months away and also no rush to remove or deprecate the setting. Even if it is only for the extremely unlikely case to avoid having to undeprecate it later on (see also https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2855558032). The option has existed for
...

💬 iFadi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2855633720)
Concept NACK

💬 ryanofsky 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-2855636472)
> The thing I don't understand is why setting CMAKE_PREFIX_PATH as well as CMAKE_FIND_ROOT_PATH seems to be necessary on nix while setting CMAKE_FIND_ROOT_PATH alone seems to work on other platforms.

Reason for this is that nix is patching cmake to remove the root path `/` from `CMAKE_SYSTEM_PREFIX_PATH` in https://github.com/NixOS/nixpkgs/blob/df545660856d8252b4856bede70d424ef4b47c64/pkgs/by-name/cm/cmake/001-search-path.diff

Because of this `CMAKE_FIND_ROOT_PATH` is not searched directly for
...

💬 willcl-ark 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-2855642758)
Yes I agree @ryanofsky, thanks for double-checking!

I added the `if()` guard because I heard from @hebasto that we call the toolchain multiple times, and the `if` guard therefore avoids adding the path repeatedly.

📝 w0xlt opened a pull request: "docs: Improve `keypoolrefill` RPC docs"
(https://github.com/bitcoin/bitcoin/pull/32429)
Update `keypoolrefill` RPC docs to make it clear that descriptor wallets have 4 ScriptPubKeyManagers by default and each of them is updated in this command, as suggested https://github.com/bitcoin/bitcoin/issues/29924#issuecomment-2849321859

Closes https://github.com/bitcoin/bitcoin/issues/29924

💬 w0xlt commented on issue "`keypoolrefill` doesn't fill keypool to specified parameter":
(https://github.com/bitcoin/bitcoin/issues/29924#issuecomment-2855652872)
> it could make sense to clarify the RPC docs

Done in https://github.com/bitcoin/bitcoin/pull/32429

💬 ryanofsky 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-2855669606)
> I added the `if()` guard because I heard from [@hebasto](https://github.com/hebasto) that we call the toolchain multiple times, and the `if` guard therefore avoids adding the path repeatedly.

I think this is half-true. I noticed by adding prints to the cmake file that it was called multiple times, but each time CMAKE_PREFIX_PATH was always set back to the original value, so I don't think the `if` guard did anything.

Also just to be sure another difference isn't glossed over: I think it proba
...

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2076116744)
Done both.

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2076119234)
Removed:
* `importmulti' in doc/descriptors.md
* bdb.cpp in linter
* `WriteHDCHain` and WriteCScript`

I would prefer to leave further cleanups for follow up RPs as I am sure there are plenty of things that were missed and trying to catch them all in this one PR is going to be very annoying and just delay this even more.

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2076119393)
Done