💬 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

👍 ryanofsky approved a pull request: "Have createNewBlock() return a BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/30440#pullrequestreview-2191872106)
Code review ACK bde6c7f7b126beea1359990e012c76c5cafc34a6. Left one suggestion, but this change is much simpler now and look good.

💬 ryanofsky commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1686727569)
In commit "Have createNewBlock return BlockTemplate interface" (f434c8562281dfec1bab37e7232ca35fd759e25f)

While changing this it would make sense to add std::move() on line 148 below so the block data does not need to be copied there.

👍 hebasto approved a pull request: "contrib: simplify `test-security-check`"
(https://github.com/bitcoin/bitcoin/pull/30423#pullrequestreview-2191894000)
ACK 1bc9f64bee919bc46eb061ef8c66f936eb6a8918 (assuming my Guix hashes match; I'll provide them shortly).

My [concern](https://github.com/bitcoin/bitcoin/pull/30423#discussion_r1674030590) about `-pie` support for Windows is not a blocker.