💬 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.

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686502664)
We can get a reack, let's not focus on that.
I'm NACK 788fe9cc9ab979c5e14f544ae05dc46436892b81 -ing because of https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686336262 anyway.

👍 josibake approved a pull request: "Fix lint-spelling warnings"
(https://github.com/bitcoin/bitcoin/pull/30500#pullrequestreview-2191495785)
ACK https://github.com/bitcoin/bitcoin/pull/30500/commits/bccfca0382bbf00092db6e7828fc79b6ce399c5d

Ran the linter locally to verify all of the warnings are now fixed.

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686504600)
It is random access, but it's also contiguous, allowing for the safe range-`for`. If I had to do it again I might have lifted the `break;` up to the same line as the `if`. If I need to retouch and others support that change I might do that to make it slightly more terse like yours.

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686507836)
Since we're modifying the function I'm suggesting more coverage for the cases, based on what other hex parsers were testing for. It's not a difficult or unreasonable request.

💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2242892490)
> Remove `-DENABLE_FUZZ` and introduce `-DABORT_ON_FAILED_ASSUME=ON|OFF`? Then the current workflow:

I'd still want a fuzz-specific build option to enable fuzz specific build flags. Otherwise, if more fuzz-specific build flags are added in the future, it will be a global breaking change again.

> would become

The problem would be that someone forgetting to specify `--target fuzz` would run into link errors in the other targets. (I think it is fine, but I wanted to mention it).

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2242894082)
CI failure seems unrelated.

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686510133)
You can of course redo it, I didn't review your change for us to ponder on what could have been...