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

🤔 hebasto reviewed a pull request: "depends: build expat with CMake"
(https://github.com/bitcoin/bitcoin/pull/29878#pullrequestreview-2191573891)
Tested 6ccf95586047ad520af15a8451154ae519e272f9.

1. Autotools compiles with `-fexceptions`, but CMake doesn't. Considering that this option is disabled for C by default, should we mirror this behaviour?

2. Should we bump the [CMake minimum required version](https://github.com/libexpat/libexpat/blob/3bab6c09bbe8bf42d84b81563ddbcf4cca4be838/expat/CMakeLists.txt#L36):
```cmake
cmake_minimum_required(VERSION 3.1.3)
```
?