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

💬 hebasto commented on pull request "depends: Fetch miniupnpc sources from an alternative website":
(https://github.com/bitcoin/bitcoin/pull/30151#issuecomment-2243123262)
That's unfortunate.

As for today, https://miniupnp.free.fr/ is unavailable for me (

👍 ryanofsky approved a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2191641211)
Responded to a bunch of comments, but after reading them I do think this PR looks ok in its current form, and is an improvement.

Would be happy to merge as is, or re-ack if there are more changes.

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686605041)
re: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686541028

Tend to agree with marco, should avoid adding test cases if they are redundant with other ones. Better to put effort into organizing test cases and making them target specific functionality so it is more obvious if there are gaps in coverage and there is less code to maintain.

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686616766)
re: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686328446

I don't really think it makes sense to make declare string views const, since the data they point to is already immutable, and the argument is passed by value so no changes to the view inside the function could affect the caller. I think the const just adds verbosity. Not 100% sure about this and haven't thought about it much, but would want to know a specific reason for using const here, or see some broader recommendati
...

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686594294)
re: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686476487

Mildly prefer paplorinc's suggestion, but no strong opinion

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686625864)
> You can of course redo it, I didn't review your change for us to ponder on what could have been...

I think the while loop is a slightly better but range-for is also ok.

I still do think as long as you are going to update the loop to respect the length of the input (instead of blowing past it) it would be better update the loop to respect the length of the output as well, and not pointlessly read more data than can actually be used, as suggested https://github.com/bitcoin/bitcoin/pull/304
...