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

📝 maflcko opened a pull request: " lint: Add missing docker.io prefix to ci/lint_imagefile "
(https://github.com/bitcoin/bitcoin/pull/30501)
Currently, the `ci/lint_imagefile` may pick the wrong (non-native) architecture due to the missing prefix.

For example, assuming the user has previously pulled an s390x image:

```
$ podman run --rm 'docker.io/s390x/debian:bookworm' dpkg --print-architecture
exec /usr/bin/dpkg: exec format error
```

Now, `debian:bookworm` will refer to the same image:

```
$ podman run --rm 'debian:bookworm' dpkg --print-architecture
exec /usr/bin/dpkg: exec format error
```

However, `docker.i
...

💬 maflcko commented on pull request "lint: Use consistent out-of-tree build for python and test_runner":
(https://github.com/bitcoin/bitcoin/pull/30499#discussion_r1686749450)
Done in https://github.com/bitcoin/bitcoin/pull/30501

👍 paplorinc approved a pull request: "lint: Add missing docker.io prefix to ci/lint_imagefile"
(https://github.com/bitcoin/bitcoin/pull/30501#pullrequestreview-2191914039)
utACK fa7bee13bf745d8d244fa8d3579a21016a0cb66d

Thanks!

👍 hebasto approved a pull request: "lint: Add missing docker.io prefix to ci/lint_imagefile"
(https://github.com/bitcoin/bitcoin/pull/30501#pullrequestreview-2191930364)
ACK fa7bee13bf745d8d244fa8d3579a21016a0cb66d.