💬 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

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2242887750)
> Perhaps, it should be considered an upstream bug in configure.ac

The files are compiled unconditionally, and the content is control by defines. If you look at the object files in depends. None have any symbols, so I'm not sure how it's a bug.