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

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686541028)
> But no need to hold up a bugfix based on a test-only minor nit.

agree, this isn't the reason for my NACK.

💬 pythcoiner commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2242944331)
ACK a512245

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686546154)
> std::string_view::remove_prefix() itself invites mutation.

no, we din't mutate before, we returned a new view. We're also doing that now, but the copy is done implicitly in the parameters. The original `substr` way made more sense - like we're doing with `TrimStringView` as well.

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

Thanks for addressing @maflcko, this makes things much more sanitary.

That said, our `mlc` lint is _supposed_ to exclude all files/dirs which are not tracked by git, by calling `git ls-files --ignored --others --exclude-standard`.

It appears that the `--ignored` flag causes some directories/files to not appear in this listing, leading to the failure:

```bash
will@ubuntu in ~/src/bitcoin on  2407-lint-fixes [$?] : 🐍 (bitcoin)
$ git ls
...

👍 ryanofsky approved a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2191568788)
Code review ACK 788fe9cc9ab979c5e14f544ae05dc46436892b81. No changes since my last review other than a commit message change.

This might be ready for merge since it has 3 acks and is definitely an improvement over the status quo. But I haven't read through the comments yet or looked into the concerns behind the one nack. It also does seem like this PR might still be a little rough around the edges, so maybe it wouldn't hurt to iterate a little more.

re: https://github.com/bitcoin/bitcoin/p
...