💬 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)
```
?

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686550665)
I'm not sure why the type is relevant here, what I'm saying is that we had a [const](https://learn.microsoft.com/en-us/cpp/cpp/const-cpp) guarantee before which we can keep.

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686551381)
My other suggestions make this irrelevant, we can zero during iteration as well

💬 vasild commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2242965378)
@maflcko, there are no "fuzz specific build flags". `ABORT_ON_FAILED_ASSUME` is not fuzz specific, it is also enabled for `-DCMAKE_BUILD_TYPE=Debug`. Anyway, I guess you mean "fuzz needed", right? Would you want to

1. Have an option to enable `ABORT_ON_FAILED_ASSUME` plus any other future options that would be needed for fuzzing? or
2. The above 1. plus force `BUILD_FUZZ_BINARY=ON` and disable all other targets?

🤔 instagibbs reviewed a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2191593794)
better test than what I was asking for, thanks

💬 instagibbs commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1686563996)
```Suggestion
# conservative mode will consider longer time horizons while economical mode does not
```

💬 instagibbs commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1686562576)
why two loops to make 12 blocks?

💬 instagibbs commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1686565138)
doesn't use `self`

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2242983915)
reACK https://github.com/bitcoin/bitcoin/pull/30126/commits/cad318fa843f411e52c6761a4882bfaf0ad21812

via `git range-diff master 6160ccf9b4327649e9bb0293fba630a10b3befc3 cad318fa843f411e52c6761a4882bfaf0ad21812`

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686572142)
I like your version slightly more, will do if I need to re-touch the commit and others agree.

Sorry for being difficult. Wish your review had come in sooner.

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686574857)
> Wish your review had come in sooner

nothing is lost yet, we're all on the same side, people will glady reack :)

💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2243001524)
> there are no "fuzz specific build flags"

I am not a native speaker, but I wanted to say "build flags that make sense in the context of fuzzing". They are not "needed", rather "recommended".

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686585389)
The reason for using the standard C++ `remove_prefix()` function inside of the pre-existing `RemovePrefixView()` over `substr()` was covered here:
https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1679138796
> `base_blob` now calls `RemovePrefixView()` which it did not previously do. I want to make it as efficient as possible compared to the prior `base_blob` implementation, hence this change.

To be even more clear than I was that time - please compare the implementations of [subst
...