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

💬 fjahr commented on pull request "rpc: Return precise loadtxoutset error messages":
(https://github.com/bitcoin/bitcoin/pull/30497#issuecomment-2243166522)
Concept ACK

I think the title is a bit confusing here. It sounds like the error message content changes but they are just moved from logs to RPC. I think the title of the the issue that is fixed is a better description.

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686699872)
> the data they point to is already immutable

The string part, but not the leading and trailing offsets.


The `const` signals the intention of the `string_view`.
By declaring it as `const std::string_view str` I'm announcing that its value will be constant throughout the method, right?
But if it's just `std::string_view str`, I can call `str.remove_prefix` or `str.remove_prefix`, which modifies the internals of the view, so after the call its internal state is different.

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686701139)
If you think it's redundant, please mark this as resolved.

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686709463)
> imaginary speed gain.


You are correct that this is not a speed gain. I'd be surprised if the compiler-produced ASM even differed at all. They should be exactly equivalent.

This pull request has more than a 100 comments for less lines of code changed, and several reviews that say it is an improvement. There will always be room for more style nits, but I think at some point it is best to leave them for follow-ups or ignore them.

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686711284)
it's not very important in this particular method, but `inline` + `const` could help with the actual inlining - it's also clearer to the reader to understand what happens when mutating the parameter, since after inlining it should ideally avoid the copying (which is easy to do for a const).
At least this was my thinking for recommending it, let me know if I'm mistaken.

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686714441)
if we're afraid to modify code, it's usually a code smell, which points to not enough test coverage.
Instead of being afraid, we should improve our code coverage.
We can't take a tank to battle if we're afraid we'll scratch it.

💬 maflcko commented on pull request "rpc: Return errors in loadtxoutset that currently go to logs":
(https://github.com/bitcoin/bitcoin/pull/30497#issuecomment-2243198401)
> I think the title is a bit confusing here. It sounds like the error message content changes but they are just moved from logs to RPC. I think the title of the the issue that is fixed is a better description.

Good point, fixed!

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686726401)
> it would help if you could say more specifically what you find confusing about this code

I'll try to rephrase.
Firstly, as you've mentioned as well, the name of the method hints at mutation, but it actually returns a copy of the view.
Second, it's inline + pass by value mutation, which implies it's not a simple search&replace type inline, since the parameters aren't consts, but it also can't just mutate the original values passed as parameters, so the copy of the parameter is implicit. Ma
...

✅ fanquake closed an issue: "Spurious markdown linter failure"
(https://github.com/bitcoin/bitcoin/issues/30496)

🚀 fanquake merged a pull request: "lint: Use consistent out-of-tree build for python and test_runner"
(https://github.com/bitcoin/bitcoin/pull/30499)

💬 edilmedeiros commented on pull request "depends: Fetch miniupnpc sources from an alternative website":
(https://github.com/bitcoin/bitcoin/pull/30151#issuecomment-2243216680)
Isn't possible/desirable to try to get it from the Github repo instead?

https://github.com/miniupnp/miniupnp

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686729618)
> This pull request has more than a 100 comments for less lines of code changed

I did all the changes I requested during review (even provided diffs to speed it up), they're not complicated.

💬 hebasto commented on pull request "depends: Fetch miniupnpc sources from an alternative website":
(https://github.com/bitcoin/bitcoin/pull/30151#issuecomment-2243222945)
> Isn't possible/desirable to try to get it from the Github repo instead?

See https://github.com/bitcoin/bitcoin/pull/30151#issuecomment-2124512367:
> If that does keep the sha256 hash the same then maybe. Mind that github-generated tar.gzs aren't guaranteed to have a stable hash.

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2243227225)
utACK 9afa2c9e50370b2918377f3f3eac0950a4296ec0