🤔 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
...
✅ maflcko closed an issue: "ci: Intermittent failure "Could not resolve host: github.com""
(https://github.com/bitcoin/bitcoin/issues/31889)
(https://github.com/bitcoin/bitcoin/issues/31889)
💬 maflcko commented on issue "ci: Intermittent failure "Could not resolve host: github.com"":
(https://github.com/bitcoin/bitcoin/issues/31889#issuecomment-2663219347)
Closing for now, because it seems out-of-scope for this repo. (Not sure where to file otherwise, so feel free to continue discussion here for now)
(https://github.com/bitcoin/bitcoin/issues/31889#issuecomment-2663219347)
Closing for now, because it seems out-of-scope for this repo. (Not sure where to file otherwise, so feel free to continue discussion here for now)
🤔 sipa reviewed a pull request: "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop"
(https://github.com/bitcoin/bitcoin/pull/31826#pullrequestreview-2621167189)
utACK 09b150bb8adae00854f02ece69fc6ef222fb07d9
(https://github.com/bitcoin/bitcoin/pull/31826#pullrequestreview-2621167189)
utACK 09b150bb8adae00854f02ece69fc6ef222fb07d9
💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#discussion_r1958297299)
thanks, will do in the follow-up
(https://github.com/bitcoin/bitcoin/pull/31836#discussion_r1958297299)
thanks, will do in the follow-up
💬 hodlinator commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1958297307)
I'd say #29500 is more related to #23119 than this PR.
> 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.
Yes, that's why I commented about the linter situation above.
> @hodlinator, could we separate fixing dead code from introducing another asserti
...
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1958297307)
I'd say #29500 is more related to #23119 than this PR.
> 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.
Yes, that's why I commented about the linter situation above.
> @hodlinator, could we separate fixing dead code from introducing another asserti
...
💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2663237229)
Thanks for the two nits. I'll address them in one of the related follow-ups that will touch this area anyway.
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2663237229)
Thanks for the two nits. I'll address them in one of the related follow-ups that will touch this area anyway.
💬 maflcko commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2663270815)
Thanks for explaining. I must have skipped over the last paragraph in the commit message, which mentions the mpgen executable. However, given the lack of any fuzz target that could possibly call into mp code, I wonder if it may be easier to just disable the MP configure option when compiling for fuzzing. Otherwise, it seems like changing the build code to support compiling code that will never be executed anyway.
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2663270815)
Thanks for explaining. I must have skipped over the last paragraph in the commit message, which mentions the mpgen executable. However, given the lack of any fuzz target that could possibly call into mp code, I wonder if it may be easier to just disable the MP configure option when compiling for fuzzing. Otherwise, it seems like changing the build code to support compiling code that will never be executed anyway.
✅ l0rinc closed a pull request: "WIP: speed up `BatchWrite` by sorting the batches in descending order"
(https://github.com/bitcoin/bitcoin/pull/31875)
(https://github.com/bitcoin/bitcoin/pull/31875)
💬 l0rinc commented on pull request "WIP: speed up `BatchWrite` by sorting the batches in descending order":
(https://github.com/bitcoin/bitcoin/pull/31875#issuecomment-2663275569)
Closing for now to avoid needless reviews, since further benchmarks cast doubt on the validity of this finding, will reopen after I clarify the source of this ambiguity.
(https://github.com/bitcoin/bitcoin/pull/31875#issuecomment-2663275569)
Closing for now to avoid needless reviews, since further benchmarks cast doubt on the validity of this finding, will reopen after I clarify the source of this ambiguity.