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

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

> I find it quite confusing that we're sneaking in a mutation of the copy

paplorinc, maybe it would help if you could say more specifically what you find confusing about this code? At least to me the new implementation of this function seems more straightforward than the previous implementation, though I don't think there is a big difference.

I don't love the name of the `RemovePrefixView` function, because that name so
...

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

Again I agree with hodlinator and don't get the desire to add const to a parameter that is passed by value and can't be modified anyway. I'd be curious if there is more reasoning behind the suggestion, but it doesn't seem like an improvement off hand.

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

Calling SetNull, or skipping zeroing as suggested seem like improvements, but no strong opinion. Also think the other suggestions to use begin / end and simplify the loop are good, so added :+1:

I can also understand other point of view of wanting to change code as little as possible while fixing a bug, and wanting to avoid asking people to rereview code that is changing and potentially introduce new bugs. But whatev
...