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

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686678529)
I'm not sure what you're saying, both are the same, one creates a copy via the params (unless it's inlined, I guess, that's why I find it confusing) and modifies the copied instance, the other returns a new copied instance.
I really love speed optimizations that also make the code more readable, but this does neither.

given:
```C++
static void SetHexBenchmark(benchmark::Bench& bench)
{
uint256 hash;
bench.run([&] {
hash.SetHex("1234567890abcdef1234567890abcdef1234567890
...

💬 mzumsande commented on pull request "validation: Improve, document and test logic for chains building on invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/30207#issuecomment-2243141098)
[2976f3a ](https://github.com/bitcoin/bitcoin/commit/2976f3a8dc130318c6eb222d5836e8354f5b1c1a)to [d7409a7](https://github.com/bitcoin/bitcoin/commit/d7409a71d0aa74182df190fbef016af97dbb81eb): rebased

💬 craigraw commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2243160526)
> So Mallory can put the 9sat/vB transaction in the broadcast pool

From the OP: "A broadcast pool is a cache local to the node which contains transactions which have been initially broadcast from that node". So Mallory's transaction would not enter Alice's broadcast pool, as Alice did not initiate its broadcast. The broadcast pool *only* contains transactions that Alice has broadcast either from her Core wallet or via the `sendrawtransaction` RPC. It does not contain transactions received ove
...