π¬ 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.
π€ BrandonOdiwuor reviewed a pull request: "cmake: Add `libbitcoinkernel` target"
(https://github.com/bitcoin/bitcoin/pull/31869#pullrequestreview-2621071470)
Tested ACK 3a914ab96bdeb70cdf848dd74deda5a80d0a7f78
(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?
(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.
(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
(https://github.com/bitcoin/bitcoin/pull/31836#pullrequestreview-2621090220)
Tested ACK fa3e409c9a084112fc2644a2bba9aa196bdb229d
π¬ hodlinator commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1958267479)
Thanks, added type annotations to `assert_true()` in latest push. `mypy --check-untyped-defs` catches type mismatches for it, but not for built-in `assert`. It also triggers a deluge of other errors though.
The point of #23119 is for assertion failures to output the values that made them fail. While it also entails replacing a subset of instances of built-in `assert` with our own, the purpose is different.
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1958267479)
Thanks, added type annotations to `assert_true()` in latest push. `mypy --check-untyped-defs` catches type mismatches for it, but not for built-in `assert`. It also triggers a deluge of other errors though.
The point of #23119 is for assertion failures to output the values that made them fail. While it also entails replacing a subset of instances of built-in `assert` with our own, the purpose is different.
π¬ l0rinc commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1958276728)
It's also similar to https://github.com/bitcoin/bitcoin/pull/29500 from a year ago.
Btw, [type hints](https://github.com/bitcoin/bitcoin/compare/84485368c4234596519aace2b851402e3b53e624..790c509a4796ec47be2275d86b35370ff25a599a#diff-e20e1f68486e5c096fdc11bca1cda063aacef524411f011d9c3497eb650c5eafR73) in Python are just hints, adding `assert_true(1)` will still pass the test.
@hodlinator, could we separate fixing dead code from introducing another assertion?
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1958276728)
It's also similar to https://github.com/bitcoin/bitcoin/pull/29500 from a year ago.
Btw, [type hints](https://github.com/bitcoin/bitcoin/compare/84485368c4234596519aace2b851402e3b53e624..790c509a4796ec47be2275d86b35370ff25a599a#diff-e20e1f68486e5c096fdc11bca1cda063aacef524411f011d9c3497eb650c5eafR73) in Python are just hints, adding `assert_true(1)` will still pass the test.
@hodlinator, could we separate fixing dead code from introducing another assertion?
π brunoerg approved a pull request: "contrib: Add deterministic-fuzz-coverage"
(https://github.com/bitcoin/bitcoin/pull/31836#pullrequestreview-2621138925)
tested ACK fa3e409c9a084112fc2644a2bba9aa196bdb229d
```sh
...
--- /Users/brunogarcia/projects/bitcoin-core-dev/build_fuzz/fuzz_det_cov.show.0.txt 2025-02-17 10:47:20
+++ /Users/brunogarcia/projects/bitcoin-core-dev/build_fuzz/fuzz_det_cov.show.1.txt 2025-02-17 10:47:20
@@ -19737,7 +19737,7 @@
24| 4.48k|# define bitcoin_builtin_bswap32(x) __builtin_bswap32(x)
25| |# endif
26| |# if __has_builtin(__builtin_bswap64)
- 27| 8.37M|# de
...
(https://github.com/bitcoin/bitcoin/pull/31836#pullrequestreview-2621138925)
tested ACK fa3e409c9a084112fc2644a2bba9aa196bdb229d
```sh
...
--- /Users/brunogarcia/projects/bitcoin-core-dev/build_fuzz/fuzz_det_cov.show.0.txt 2025-02-17 10:47:20
+++ /Users/brunogarcia/projects/bitcoin-core-dev/build_fuzz/fuzz_det_cov.show.1.txt 2025-02-17 10:47:20
@@ -19737,7 +19737,7 @@
24| 4.48k|# define bitcoin_builtin_bswap32(x) __builtin_bswap32(x)
25| |# endif
26| |# if __has_builtin(__builtin_bswap64)
- 27| 8.37M|# de
...
π¬ brunoerg commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#discussion_r1958279670)
nit: maybe could mention `fuzz_corpora` directly
```suggestion
RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_dir $PWD/qa-assets/fuzz_corpora fuzz_target_name
```
(https://github.com/bitcoin/bitcoin/pull/31836#discussion_r1958279670)
nit: maybe could mention `fuzz_corpora` directly
```suggestion
RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/build_dir $PWD/qa-assets/fuzz_corpora fuzz_target_name
```
π¬ brunoerg commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2663217465)
Also, worths adding a .gitignore for the tool (especially for the `target` folder)?
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2663217465)
Also, worths adding a .gitignore for the tool (especially for the `target` folder)?
β οΈ maflcko opened an issue: "ci: Intermittent failure "Could not resolve host: github.com""
(https://github.com/bitcoin/bitcoin/issues/31889)
Looks like this happens for some workers/users on the same machine. For example, it happens for `ci_worker_1738693519_026122670`, but none of the others. Also, it seems to *almost* persist for several hours. That is, it happens on almost all runs, once it happened at least once.
Example failures:
* https://cirrus-ci.com/task/6482591089426432?logs=ci#L603
* https://cirrus-ci.com/task/6102859474796544?logs=ci#L624
* https://cirrus-ci.com/task/6278234062454784?logs=ci#L614
However, the same work
...
(https://github.com/bitcoin/bitcoin/issues/31889)
Looks like this happens for some workers/users on the same machine. For example, it happens for `ci_worker_1738693519_026122670`, but none of the others. Also, it seems to *almost* persist for several hours. That is, it happens on almost all runs, once it happened at least once.
Example failures:
* https://cirrus-ci.com/task/6482591089426432?logs=ci#L603
* https://cirrus-ci.com/task/6102859474796544?logs=ci#L624
* https://cirrus-ci.com/task/6278234062454784?logs=ci#L614
However, the same work
...
π¬ ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2663218525)
> > [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.
@maflcko guessing you maybe did not notice the comments or commit message? All the points you raised should be covered there, but let me know if anything should be updated or clarified.
Steps to reproduce are to check out the branch, revert the commit, and try to
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2663218525)
> > [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.
@maflcko guessing you maybe did not notice the comments or commit message? All the points you raised should be covered there, but let me know if anything should be updated or clarified.
Steps to reproduce are to check out the branch, revert the commit, and try to
...