💬 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?

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077268171)
Currently, no, but after #32380 it becomes critical because it determines the process code page.

> (not too important yet now as we're just adding some metadata, but when we add critical things in there like "use UTF-8 codepage" as in https://github.com/bitcoin/bitcoin/pull/32380, it's really important for them to be in the release too)

This is a quick and dirty check that the manifest is added and is what we expect, feel free to add detailed XML parsing.

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077273311)
It also already found one bug in this PR: the manifest was not added to `test_bitcoin.exe` .

💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077274476)
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?

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077279482)
i don't see the problem with having it right now like this, but okay.

💬 laanwj commented on pull request "lint: Remove string exclusion from locale check":
(https://github.com/bitcoin/bitcoin/pull/32434#issuecomment-2858004369)
> The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().

i wonder if the same is true for the comment exclusion. Would it detect `/* bla */ wrong()`.

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077314447)
One thing i did think about is using the input file (`make/windows-app.manifest.in`) as template instead of hardcoding it here, but having the check depend on files scattered throughout the repository is probably not that great, either.

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2077317447)
Can be marked as resolved, thanks! I think we were talking past each other a bit but resolved offline.