💬 TheCharlatan commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#issuecomment-2662391575)
> I would be a fan of going further and making the BlockManagerOptions ctor take a BlockDirLock (or, as per my comment a DirectoryLock) to allow more granular error feedback, but this is already a step in the right direction regardless.
I attempted implementing this, but giving the options the lock starts to introduce non-trivial additional semantics to the options: Since the lock needs to be moved from the options to the block manager, the options cannot be re-used. I'm not sure how to imple
...
(https://github.com/bitcoin/bitcoin/pull/31860#issuecomment-2662391575)
> I would be a fan of going further and making the BlockManagerOptions ctor take a BlockDirLock (or, as per my comment a DirectoryLock) to allow more granular error feedback, but this is already a step in the right direction regardless.
I attempted implementing this, but giving the options the lock starts to introduce non-trivial additional semantics to the options: Since the lock needs to be moved from the options to the block manager, the options cannot be re-used. I'm not sure how to imple
...
💬 maflcko commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31863#issuecomment-2662416943)
> Eagerly awaiting C++26's explicit `[[indeterminate]]` [initialization](https://en.cppreference.com/w/cpp/language/attributes/indeterminate).
Maybe I am missing something, but I don't think `[[indeterminate]]` should be used here either. My understanding is that `[[indeterminate]]` in C++26 is simply a way to restore pre-C++-26 UB. However, if there really is UB in one of those functions, we'd want the C++26 erroneous behavior so that the implementation is encouraged to issue a diagnostic (a
...
(https://github.com/bitcoin/bitcoin/pull/31863#issuecomment-2662416943)
> Eagerly awaiting C++26's explicit `[[indeterminate]]` [initialization](https://en.cppreference.com/w/cpp/language/attributes/indeterminate).
Maybe I am missing something, but I don't think `[[indeterminate]]` should be used here either. My understanding is that `[[indeterminate]]` in C++26 is simply a way to restore pre-C++-26 UB. However, if there really is UB in one of those functions, we'd want the C++26 erroneous behavior so that the implementation is encouraged to issue a diagnostic (a
...
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2662441920)
`e1671ff42c...741f17e51d`: rebase and remove the first commit which was merged via https://github.com/bitcoin/bitcoin/pull/31854, thanks!
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2662441920)
`e1671ff42c...741f17e51d`: rebase and remove the first commit which was merged via https://github.com/bitcoin/bitcoin/pull/31854, thanks!
💬 maflcko commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1957847712)
how could a fuzz engine work around the checksum? I guess it may be small enough to allow it to rarely solve the checksum. However, given the removed unit test, I am not sure if the coverage is identical.
It would be good to submit the inputs from the unit test as fuzz inputs, as well as any fuzz inputs that got stale by renaming/splitting the fuzz target. See also:
```
[06:22:44.212] Fuzzing harnesses lacking a corpus: base32_encode_decode base58_encode_decode base58check_encode_decode
...
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1957847712)
how could a fuzz engine work around the checksum? I guess it may be small enough to allow it to rarely solve the checksum. However, given the removed unit test, I am not sure if the coverage is identical.
It would be good to submit the inputs from the unit test as fuzz inputs, as well as any fuzz inputs that got stale by renaming/splitting the fuzz target. See also:
```
[06:22:44.212] Fuzzing harnesses lacking a corpus: base32_encode_decode base58_encode_decode base58check_encode_decode
...
💬 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?
(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
(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?
(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`.
(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)
(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?
(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.
(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
...
(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
(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
...
(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
...
(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)
(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
(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
(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.
(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.
(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.