💬 purpleKarrot commented on pull request "cmake: Add optional source files to libraries directly":
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2661608489)
Concept NACK.

I am not a big fan of simple utility functions that group a small set of builtin cmake commands. I think they increase the cognitive load and impede knowledge transfer from one cmake project to another. (I remember a time when builtin functions like `add_library` had to be wrapped and I authored many such wrappers, because there was no way of propagating interface requirements from one target to another. Those times are thankfully over since CMake version 3.0).

💬 purpleKarrot commented on pull request "cmake: Add `libbitcoinkernel` target":
(https://github.com/bitcoin/bitcoin/pull/31869#discussion_r1957411682)
Synonyms can be created like this

```cmake
add_library(libbitcoinkernel ALIAS bitcoinkernel)
```

By the way, you can increase robustness by adding a namespace to the alias target:

```cmake
add_library(Bitcoin::Kernel ALIAS bitcoinkernel)
```

This is more robust against typos. Linking against `Bitcorn::Kernel` will result in a "no such target defined" error at configure time, while linking against `bitcornkernel` will simply result in `-lbitcornkernel` being passed to the linker.

💬 fjahr commented on pull request "test: chain reorg for coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/31885#issuecomment-2661631392)
Hm, we have a reorg test for coinstatsindex in the functional tests: https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_coinstatsindex.py#L258 Can you give additional context why you think this improves coverage or other motivation to add a unit test for this?

💬 hebasto commented on pull request "cmake: Add `libbitcoinkernel` target":
(https://github.com/bitcoin/bitcoin/pull/31869#discussion_r1957421498)
> Synonyms can be created like this
>
> ```cmake
> add_library(libbitcoinkernel ALIAS bitcoinkernel)
> ```

Indeed, that was my starting point. However, an `ALIAS` library cannot be built:
```
$ cmake -B build -G "Unix Makefiles" -DBUILD_KERNEL_LIB=ON
$ cmake --build build -t libbitcoinkernel
gmake: *** No rule to make target 'libbitcoinkernel'. Stop.
```
or
```
$ cmake -B build -G "Ninja" -DBUILD_KERNEL_LIB=ON
$ cmake --build build -t libbitcoinkernel
ninja: error: unknown tar
...

📝 midnightmagic opened a pull request: "correct wrong assumptions in the contrib linearize data script"
(https://github.com/bitcoin/bitcoin/pull/31888)
The linearize-data contrib script does not quite work correctly in the case of a chain reorg, and it fully-truncates files with an incorrect open mode.

This small patch clips off incorrect trailing data and corrects file open mode type.

(small patch being passed around which I did not write but thought would be helpful to the community)

💬 walterl commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1957466889)
Isn't this supposed to return `kernel_UNKNOWN_NEW_RULES_ACTIVATED`?

```suggestion
return kernel_Warning::kernel_UNKNOWN_NEW_RULES_ACTIVATED;
```

💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2662213412)
Tar archives, to test `bitcoind` (too large for Github):

- [bitcoin-096525e92cc2-arm64-apple-darwin.tar.gz](https://download.sprovoost.nl/download.php?id=13&token=8de04067101d0748ccba9cb0e5c568cb)
- [bitcoin-096525e92cc2-x86_64-apple-darwin.tar.gz](https://download.sprovoost.nl/download.php?id=14&token=2dd77c4ec797fb3c39c207e50cbd65d3)

💬 maflcko commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2662227740)
> [96d3b2a](https://github.com/bitcoin/bitcoin/commit/96d3b2a0bb0c267569162b055ca306f709ec0b4e) "cmake: Fix fuzzer "multiple definition of `main'" errors"

It may be good to explain this commit, or add steps to reproduce. When using libFuzzer one would only want to build for fuzzing, so not setting the corresponding build flag to disable all other executables seems odd. See also:

```
ci/test/00_setup_env_mac_native_fuzz.sh:export BITCOIN_CONFIG="-DBUILD_FOR_FUZZING=ON"
ci/test/00_setup_en
...

💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2662230723)
On Apple Silicon downloading through Safari and then extracting in Finder fails:

<img width="289" alt="arm bitcoind" src="https://github.com/user-attachments/assets/8c18ab58-ecce-4b4b-aa11-f3d48cba22b9" />

So does downloading in Safari and then extracting using the command line with `tar`.

What does work is downloading from a terminal and then extracting:

```
curl -o bitcoin.tar.gz "https://download.sprovoost.nl/download.php?id=13&token=8de04067101d0748ccba9cb0e5c568cb&download"
``
...

💬 maflcko commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2662238943)
Does it happen with a smaller (default) dbcache?
Does it happen on a different filesystem than exfat?
Does it happen on a different filesystem hardware, maybe something that is faster? (Can you benchmark the hardware?)

✅ maflcko closed a pull request: "RPC: add new `listmempooltransactions`"
(https://github.com/bitcoin/bitcoin/pull/29016)

💬 maflcko commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-2662243280)
Closing for now, due to lack of progress for more than a year. Please leave a comment, if you want this reopened. Anyone may also open a new pull request, referencing this one, and including a short summary of any relevant discussion comments from here.

👍 maflcko approved a pull request: "cmake: Introduce `WITH_PYTHON` build option"
(https://github.com/bitcoin/bitcoin/pull/31669#pullrequestreview-2620206804)
lgtm. Seems fine to add an option to say "I don't have python and I don't care about the tests"

💬 maflcko commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#discussion_r1957725461)
shouldn't this be an error now?

💬 maflcko commented on pull request "test: check `scanning` field from `getwalletinfo`":
(https://github.com/bitcoin/bitcoin/pull/31768#discussion_r1957729211)
https://github.com/bitcoin/bitcoin/issues/31881

💬 maflcko commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1957746797)
> expressed seeing value in reserving `assert` for internal sanity checks in the tests

Is there an example of an internal sanity check? How would that be different from the functional framework unit tests? Also, why would there be any risk in treating an internal test error as a "normal" test error?

I understand the distinction of internal errors in user-facing programs, but for dev-only test-only scripts, the meaning and the tradeoffs become less clear.

💬 maflcko commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1957749674)
Another review note, it seems like `assert_true("abc")` will pass. This seems confusing and I am also not sure if it is clearer than `assert "abc"`. (Same for integral values like `3`, ...)

And on a general note, the attempt here seems similar or related to the discussion (and changes) stemming from https://github.com/bitcoin/bitcoin/issues/23119. However that issue is stale for more than 3 years, with no meaningful progress, so I wonder how important it is and whether it can be closed.

💬 maflcko commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1957757389)
I think further changes were left for a follow-up, see https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589412550:

> Maybe a separate table for runtime dependencies makes more sense. It is pretty clear that runtime dependencies don't have a "Version used". An alternative would be to just remove the "Runtime" column, as it can be derived from the "Version used" column.

💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2662344876)
> Does it happen with a smaller (default) dbcache?

Yes, it does happen with default settings, including default value 450 of dbcache.

💬 TheCharlatan commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2662351232)
Re https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2660002068

> > Are extra steps such as manual codesigning expected when running the binaries archived in `output/x86_64-apple-darwin-codesigned/bitcoin-096525e92cc2-x86_64-apple-darwin.tar.gz`?
>
> No, these should all be codesigned now.

and re https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2662230723

> On Apple Silicon downloading through Safari and then extracting in Finder fails:
> ...
> Similarly on Intel
...