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

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2243013595)
I think this would benefit from a unit test. Would suggest including following test in `transaction_tests.cpp` in the first commit:

```c++
BOOST_AUTO_TEST_CASE(test_txid)
{
// TxidFromString currently ignores string_view length and reads the whole
// string, not the specified substring.
BOOST_CHECK_EQUAL(TxidFromString(std::string_view("ABCD1234").substr(0, 4)).ToString(),
"00000000000000000000000000000000000000000000000000000000abcd1234");
}
```

And then updati
...

💬 maflcko commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#issuecomment-2243014586)
> 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`.

Yes, I know, but someone will need to create a pull request to pull in the mlc version that fixes this, if such a version exists.

I think the consistency fixes here make sense either way. Fixing the bug is just a convenient side effect.

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1686644773)
no need, updated.

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1686645329)
fixed

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1686647290)
yes it does not.

💬 instagibbs commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#issuecomment-2243097482)
ACK https://github.com/bitcoin/bitcoin/pull/30275/commits/48faceb91ec06ab4959038237d5e69448677855d

💬 maflcko commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2243102274)
Well, your same scenario would be possible with a broadcast pool, because a broadcast pool would need to be size-limited. So Mallory can put the 9sat/vB transaction in the broadcast pool or mempool, when the 10sat/vB transaction has been dropped from the mempool and broadcast pool. Alice's wallet will still discard her transaction using the hierarchy `confirmed > mempool > local` (or `confirmed > mempool > broadcast_pool > local`).

I understand the use cases and advantages of a broadcast pool
...

💬 maflcko commented on issue "Spurious markdown linter failure":
(https://github.com/bitcoin/bitcoin/issues/30496#issuecomment-2243122559)
The cache is restored, so temporarily this will work until the cache is evicted again, which I do not known when it will happen.