👍 paplorinc approved a pull request: "lint: Use consistent out-of-tree build for python and test_runner"
(https://github.com/bitcoin/bitcoin/pull/30499#pullrequestreview-2191431156)
utACK fa8d73e86e1c11cdfe8154ab84edc1948283454b

💬 paplorinc commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686468669)
can `popd` fail if `pushd` succeeded?

💬 paplorinc commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686464727)
Now that we're setting this explicitly, does the prefixing of `CI_RETRY_EXE` still make sense in every case?
If it does, would it make sense to use it for e.g. `${CI_RETRY_EXE} curl` (like in https://github.com/bitcoin/bitcoin/blob/master/ci/test/01_base_install.sh#L88) or `${CI_RETRY_EXE} cargo build` commands as well?

💬 paplorinc commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686470495)
Unrelated, but shouldn't git clone contain `--depth=1` throughout this file?

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686476487)
Would fully agree if we were introducing a new test function, but as it is in the middle of existing `BOOST_CHECK`s, I think it's fine. Won't sweat it if I don't show up in the `git blame` after the `BOOST_CHECK_EQUAL`-refactor. And I'm happy to review such a follow-up PR.

(Original change here was suggested in https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675658574)

💬 Sjors commented on pull request "Fix lint-spelling warnings":
(https://github.com/bitcoin/bitcoin/pull/30500#issuecomment-2242862369)
ACK bccfca0382bbf00092db6e7828fc79b6ce399c5d

The new lint failure is unrelated, see #30496. Once fixed, you should rebase this to make sure.

💬 maflcko commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686485506)
> `ci/test`

`ci/lint` is completely separate from `ci/test` (well after the first commit), so there is no interaction.

💬 maflcko commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686487708)
I don't known. I had to change this due to a lint error:

```
In ci/lint/04_install.sh line 68:
popd
^--^ SC2164 (warning): Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
Did you mean:
popd || exit
For more information:
https://www.shellcheck.net/wiki/SC2164 -- Use 'popd ... || exit' or 'popd ....
^---- ⚠️ Failure generated from lint-shell.py

💬 maflcko commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686489990)
I may do this in a follow-up

💬 Sjors commented on pull request "fuzz: reduce keypool size in `scriptpubkeyman` target":
(https://github.com/bitcoin/bitcoin/pull/30494#issuecomment-2242870330)
Concept ACK

💬 paplorinc commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686491315)
k, thanks

💬 paplorinc commented on pull request "Fix lint-spelling warnings":
(https://github.com/bitcoin/bitcoin/pull/30500#issuecomment-2242875432)
Thanks for the review @Sjors.

> The new lint failure is unrelated, see https://github.com/bitcoin/bitcoin/issues/30496

Yes, it was the [PR](https://github.com/bitcoin/bitcoin/pull/30499) that triggered this change

💬 josibake commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#issuecomment-2242876699)
utACK https://github.com/bitcoin/bitcoin/commit/fa8d73e86e1c11cdfe8154ab84edc1948283454b

ran `DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter`, more as a sanity check since this isn't really testing the underlying issue this PR is fixing.

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686494462)
See https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686054773

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686494537)
Not sure about `const`ing `string_view` parameters, could only find one pre-existing case of that pattern (`GetBuriedDeployment()`). Can sympathize with forcing introduction of new names over mutation. I like how you named the first digit `hi` (maybe `high` is more precise).

Ultimately, my modification of the function is meant to be a straightforward transformation of the original code. Since it has received a few ACKs already, I am going to put off changing it for now. But might draw some in
...

💬 vasild commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2242880306)
Ok, so in summary `-DENABLE_FUZZ=ON` does 1. force `BUILD_FUZZ_BINARY=ON`, 2. disable all other targets, 3. enable `ABORT_ON_FAILED_ASSUME`. Just that. It does not enable fuzzing.

@hebasto mentioned, "This functionality does not require any build options", so to avoid this confusing option, what about:

Remove `-DENABLE_FUZZ` and introduce `-DABORT_ON_FAILED_ASSUME=ON|OFF`? Then the current workflow:

```
cmake -DSANITIZERS=fuzzer -DENABLE_FUZZ=ON -B /tmp/build
cmake --build /tmp/build
...

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686497693)
> TryParseHash, TryParseHex, SetHexFragile

None of them are touched or changed in this pull, and as explained, the tests already exist in various places:

```
src/test/util_tests.cpp: result = ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f");
src/test/util_tests.cpp: result = ParseHex("12 34 56 78");
src/test/util_tests.cpp: result = ParseHex(" 89 34 56 78");
src/test/util_tests.cpp:
...

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686498210)
I did, of course, I'm more interested here in your opinion.

👍 hebasto approved a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723#pullrequestreview-2191487571)
ACK 0388ad0d65b6c9ee802ca641eb01d69fcdd5605d.

> > CMake compiles 7 fewer source files compared to Autotools. It skips::
>
> That's expected. We aren't opting in to either of these features.

Right. Perhaps, it should be considered an upstream bug in `configure.ac`, as we aren't opting into these features in Autotools either.

💬 vasild commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#issuecomment-2242887487)
Concept ACK