💬 maflcko commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1957883107)
nit: missing trailing comma.

Also, what is the point of combining them, when they are handled completely separate anyway?

💬 maflcko commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1957883443)
nit: please use `std::span` for new code

💬 maflcko commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1957893638)
why remove this call?

💬 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-2662571593)
This is an interesting writeup [Hurdles of macOS distribution](https://gist.github.com/rsms/929c9c2fec231f0cf843a1a746a416f5). It suggests we shouldn't use `--deep`.

💬 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-2662667293)
It might be worth trying to notarize the binaries (instead of only the GUI bundle). This [forum thread](https://forums.developer.apple.com/forums/thread/721117) suggests it can be done by temporarily zipping them and sending them Apple in that form.

(maybe in a followup)

💬 dergoegge commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2662789145)
rfm?

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1958078713)
There is no need to remove it, will put back. Probably missed it during the rework. Thanks.

💬 laanwj commented on pull request "correct wrong assumptions in the contrib linearize data script":
(https://github.com/bitcoin/bitcoin/pull/31888#issuecomment-2662871105)
Lint is failing on the formatting of the commit message:
```
[10:12:47.126] The subject line of commit hash 0cf4a6245d2bc8d57f91dd2b4f5bd5a0eed18ed4 is followed by a non-empty line. Subject lines should always be followed by a blank line.
[10:12:47.126] ^^^
[10:12:47.126]
[10:12:47.126] ^---- ⚠️ Failure generated from lint check 'commit_msg' (Check that commit messages have a new line before the body or no body at all.)!
```

It also looks like the changes somehow break the `feature_loa
...

💬 dergoegge commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2662894878)
Concept ACK

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

Why? It precisely mirrors the scenario “I don't have python and I don't care about the tests” and simply serves as a reminder to the user of the consequences:

```
$ cmake -B build -DWITH_PYTHON=OFF
...
Use ccache for compiling .............. ON


******

CMake Warning at CMakeLists.txt:680 (message):
Utils and rpcauth tests are disabled.

Use "-DWITH_PYTHON=ON" to enable them.


******

-- Configuring done (12.8s)
-- Generating don
...

💬 maflcko commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#discussion_r1958149211)
I see. I guess I wanted to say the opposite: Can't this be removed, because it seems a bit odd to print a warning that something is disabled, when the users explicitly asked it to be disabled? There is also no warning when a user sets `BUILD_TESTS=OFF`.

Another alternative would be to just require python3, without a way to disable it. Thus, the option `WITH_PYTHON` can be removed.

I am just wondering: Was there ever a single user (compiling from source) to ask for the python dependency to
...

✅ hebasto closed a pull request: "cmake: Introduce `WITH_PYTHON` build option"
(https://github.com/bitcoin/bitcoin/pull/31669)

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1958228951)
Done

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1958229143)
Done

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1958232618)
I just removed the `ALL` option. I don't see anything that needs all of them combined.

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2663128872)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1957893638, https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1957883443 and https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1957883107. Thanks, @maflcko.

🤔 BrandonOdiwuor reviewed a pull request: "cmake: Add `libbitcoinkernel` target"
(https://github.com/bitcoin/bitcoin/pull/31869#pullrequestreview-2621071470)
Tested ACK 3a914ab96bdeb70cdf848dd74deda5a80d0a7f78

💬 maflcko commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1958240211)
```suggestion
FuzzCoinSelectionAlgorithm<CoinSelectionAlgorithm::KNAPSACK>(buffer);
```

nit: it is already a span,no?

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1958248096)
yes, done.

🤔 marcofleon reviewed a pull request: "contrib: Add deterministic-fuzz-coverage"
(https://github.com/bitcoin/bitcoin/pull/31836#pullrequestreview-2621090220)
Tested ACK fa3e409c9a084112fc2644a2bba9aa196bdb229d