💬 i5hi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857496855)
The reason why Bitcoin is the best monetary network IMHO is because we have maintained conservative values on what the chain is specialized to do.

📝 maflcko opened a pull request: "lint: Remove string exclusion from locale check"
(https://github.com/bitcoin/bitcoin/pull/32434)
The exclusion isn't needed. In fact, it prevents detection of `"bla" + wrong()`.

For example, the following is not detected:

```diff
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index 1c2951deee..c1209013e5 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -336,7 +336,8 @@ RPCHelpMan addmultisigaddress()
RPCHelpMan keypoolrefill()
{
return RPCHelpMan{"keypoolrefill",
- "\nFills the keypool."+
+
...

💬 maflcko commented on pull request "docs: Improve `keypoolrefill` RPC docs":
(https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2077012046)
```suggestion
"Refills each descriptor keypool in the wallet up to the specified number of new keys.\n"
```

nit: This is trimmed anyway.

💬 maflcko commented on pull request "docs: Improve `keypoolrefill` RPC docs":
(https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2077039662)
I don't think to_string is allowed, see https://github.com/bitcoin/bitcoin/pull/32434

Could use ToString?

💬 laanwj commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2857505743)
GUIX build:
```
db8daad79781d10e52b1b225343fd72081ba717b484b44068ad10d646bdcb89c guix-build-345944bd6cf8/output/dist-archive/bitcoin-345944bd6cf8.tar.gz
55ceb9d4bd2f4707b6823d69403c659a8b1af96a24b0e4b2a6ad4a288a98214f guix-build-345944bd6cf8/output/x86_64-w64-mingw32/SHA256SUMS.part
978a9f8d8726d0fe1b81fa498f8601f7b33f88ac1fb9a4b71026edadd879d7b7 guix-build-345944bd6cf8/output/x86_64-w64-mingw32/bitcoin-345944bd6cf8-win64-codesigning.tar.gz
2668cceae2d6793ff38b25a42987b9c3b491c270377e3df
...

💬 fanquake commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2857513292)
> Add usage notes for contrib/devtools/security-check.py

I think you can actually remove these docs. The symbol / security scripts are not for general developer usage, and there's no expectation that the scripts should pass, outside of a Guix build. We could move them out of contrib to make this more clear.

> Fixes an existing bug, where bcrypt.dll was not one of the expected symbols in contrib/devtools/symbol-check.py, even though this symbol gets included by way of secp256k1:

This see
...

💬 i5hi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857513687)
anyway; i've made all my important points. I don't want to bloat this discussion with points already made but its worth repeating that:

```
If we are removing limits on OP_RETURN as the default setting for technical reasons thats fair; but not allowing a node runner to configure limits on their mempool to make it feasible to run a node (in terms of RAM) goes against our core value of making nodes easy to run for personal use.
```

I hope the core devs to act with integrity and do what is
...

💬 i5hi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857514867)
anyway; i've made all my important points. I don't want to bloat this discussion with points already made but its worth repeating that:

**If we are removing limits on OP_RETURN as the default setting for technical reasons thats fair; but not allowing a node runner to configure limits on their mempool to make it feasible to run a node (in terms of RAM) goes against our core value of making nodes easy to run for personal use.**

I hope the core devs to act with integrity and do what is right
...

💬 fanquake commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2077060913)
Was this added to appease a linter? There should be no need for code like this, as every file passed to these scripts in Guix, is a binary. Leaving the script to fail if `.parse` fails is fine.

💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2857659697)
re-ACK de054df6dc32cbd8b127c6761d9c65d95025e08f

💬 josibake commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2857741552)
Concept ACK

> A notable use-case for the kernel library is accessing and analyzing existing block data

A concrete example is index building in electrs / esplora / etc. For example, Electrs does this today by:

1. Waiting for Bitcoin Core to finish IBD
2. Reading all of the blocks out over JSON-RPC
3. Parsing them and writing them into the appropriate indexes

I started on a PoC for Electrs to use libbitcoinkernel for index building to demonstrate how this could be done much more effi
...

💬 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-2857758897)
I have just re-tested and do not find that removing `CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH` sees the depends toolchain working correctly on a default NixOS install, using the repro steps in OP.

> I'd avoid making assumptions about undocumented CMAKE_PREFIX_PATH-related behaviour. Let's preserve the idempotence of the toolchain file.

Am I correctly understanding therefore that you are suggesting for the following changes as a fix for this?:

1. Remove `CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH`
2.
...

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2857767885)
Rebased after #32086.

💬 hebasto 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-2857794327)
> Am I correctly understanding therefore that you are suggesting for the following changes as a fix for this?:
>
> 1. Remove `CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH`

Building our own depends implies that we fully rely on them, ideally skipping all other search paths used by CMake's package, library, and header search mechanisms. Therefore, I believe we should retain as many restrictions as possible.

> 2. Explicitly set `CMAKE_PREFIX_PATH` using an if-guard, as per: [437a4da](https://gith
...

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-2857833155)
> I'll compare the reindex-chainstate performance next, before & after, let's see if there's a regression (we do expect a 4% slowdown for max cache, like in IBD).

IBD seems to be basically the same as before, this change indeed only affects `reindex-chainstate`.

<details>
<summary>Details</summary>

```bash
COMMITS="baa848b8d38187ce6b24a57cfadf1ea180209711 c822dd1f355b6f403191072cdd3c9c3e234bbcaa"; \
STOP_HEIGHT=888888; DBCACHE=45000; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage";
...

💬 sipsorcery commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2857899480)
tACK b074ae1b4f52471a9d71e9431deebae5ec3d603e native Windows build.

```
c:\dev\github\bitcoin>mt.exe -nologo -inputresource:build\bin\Release\bitcoind.exe -validate_manifest
Parsing of manifest successful.
```

👍 l0rinc approved a pull request: "test: Add and use ElapseTime helper"
(https://github.com/bitcoin/bitcoin/pull/32430#pullrequestreview-2821125017)
Concept ACK - was thinking of something similar when reviewing https://github.com/bitcoin/bitcoin/pull/32414/commits/41479ed1d23ea752d0ce14c2cf5627f43bceb722#diff-0d33078447a7be03c4ca7c0c86e3f4b64f77afae719c7a353e86d688932bff3eR38

💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#issuecomment-2857909160)
Sorry for the force-pushes, but this uncovered pre-existing issues in the fuzz tests that were not using mocktime during init. So I went ahead and fixed those as well in a separate commit. Should be good now, hopefully :sweat_smile:

💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077252899)
Why do we need to use a third-party repository, that runs some Javascript, to configure a Windows Command Prompt?

💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077259998)
Why do we need this check? Regex over a copy-pasted blob of XML is pretty meh. We aren't really checking anything important here either?