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

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2242896872)
NACK 788fe9cc9ab979c5e14f544ae05dc46436892b81, because of https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686336262 mostly.

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686514577)
> It's not a difficult or unreasonable request.

I think full test coverage exists, but I am more than happy to review a test-only pull request adding more tests. (I'd be thrilled to review one that adds missing coverage)

But no need to hold up a bugfix based on a test-only minor nit.

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686516716)
I know some hashes have long been represented in little-endian while other large integers are not. Do not know of a reason why. Changing it sounds dangerous. I will consider at least documenting which endian it is if I re-touch.

💬 maflcko commented on issue "Spurious markdown linter failure":
(https://github.com/bitcoin/bitcoin/issues/30496#issuecomment-2242908414)
Fixed in https://github.com/bitcoin/bitcoin/pull/30499, which has two ACKs right now

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686533487)
* `std::string_view::remove_prefix()` itself invites mutation.
* The names `TrimStringView()` and `RemovePrefixView()` imply mutation.
* As I said in https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686494537, I could only find one other case of `const` `string_view` parameters in the entire codebase. I'm sorry if you are used to another way. It may well be a better way, but I'd rather not change more pre-existing things like `TrimStringView()` and `RemovePrefixView()` taking non-con
...

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

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686538334)
Good find. Will do if I need to retouch that commit for other reasons.