💬 fanquake commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2866437307)
> But moving both to a guix-specific path would be fine with me.

See #32458.

💬 l0rinc commented on pull request "bench: replace benchmark block with more representative one (413567 → 784588)":
(https://github.com/bitcoin/bitcoin/pull/32457#issuecomment-2866447793)
Agree - do you have a better idea?

🤔 maflcko reviewed a pull request: "bench: replace benchmark block with more representative one (413567 → 784588)"
(https://github.com/bitcoin/bitcoin/pull/32457#pullrequestreview-2827710648)
Is there a benchmark that needs this? If yes, going for synthetic, but representative (and easily adjustable) data may be a better choice for that benchmark.

💬 maflcko commented on pull request "bench: replace benchmark block with more representative one (413567 → 784588)":
(https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2081323234)
Instead of a block, this could just be random bytes from a fast random context?

🤔 TheCharlatan reviewed a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2828221038)
lgtm, waiting for guix build.

🚀 fanquake merged a pull request: "depends: Avoid using helper variables in toolchain file"
(https://github.com/bitcoin/bitcoin/pull/31360)

💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081627012)
> since it's relied on by getblocktemplate in proposal mode

That was a mistake on my end, because `checkBlock` calls `connectBlock` which does the full check.

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2081628843)
Great catch! I found [this:](https://docs.python.org/3/library/socket.html#socket.timeout)

> `exception socket.timeout`
> A deprecated alias of [TimeoutError](https://docs.python.org/3/library/exceptions.html#TimeoutError).
> Changed in version 3.10: This class was made an alias of [TimeoutError](https://docs.python.org/3/library/exceptions.html#TimeoutError).

Since Python 3.10 is minimum version required in dependencies.md (and the test passes!) I think it's ok to leave as-is

💬 maflcko commented on pull request "refactor: Removals after bdb removal":
(https://github.com/bitcoin/bitcoin/pull/32438#discussion_r2081633399)
I guess it could make sense to use a previous release where bdb wasn't yet deprecated, but not sure if this is worth it, and it could be a follow-up. Removed this commit for now, thanks.

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#issuecomment-2866480355)
> Maybe the three commits could be squashed?

Done! thanks for reviewing

👍 theStack approved a pull request: "psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows"
(https://github.com/bitcoin/bitcoin/pull/32419#pullrequestreview-2828263249)
ACK d31158d3646f3c7e4832b9ca50f6ffe02800ff4c

The introduced comments match my understanding of the deserialization of PSBT types and will hopefully help reviewing future code introduced/touched in this area. It's not ideal that the same comments are copied at three places, but don't have a better idea how to avoid this (still better to have duplicated documentation than no documentation).

💬 TheCharlatan commented on pull request "guix: move `*-check.py` scripts under contrib/guix/":
(https://github.com/bitcoin/bitcoin/pull/32458#issuecomment-2866517322)
Concept ACK

💬 Sjors commented on pull request "refactor: Removals after bdb removal":
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2866544280)
ACK fa061bfcdb0caea240fd15bcc309e7847132a4ff if CI is also happy

I didn't attempt a vcpkg build (fa5f3e62c8801cca80997cfb046c13983e0876e7), but I assume the Windows CI does that?

I didn't check for _additional_ things that could be removed. Though there is bunch of watch-only related code in the GUI that can be dropped.

🤔 vasild reviewed a pull request: "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory"
(https://github.com/bitcoin/bitcoin/pull/32423#pullrequestreview-2828006168)
Almost ACK 3acfc071c3445e943069b2778bbc5c74f702ef36

Only unsure about `The configured rpcuser or rpcpassword cannot contain a ":"`, see comment below.

💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081681184)
This looks like a new restriction. Could it introduce a breakage for users that have `-rpcpassword=contains:colon`?

It is sub-optimal to join/craft the string "user:pass" and to split/parse it a few lines later. Better store the user and pass in separate variables, like this:

<details>
<summary>[patch] use separate variables for user and pass</summary>

```diff
diff --git i/src/httprpc.cpp w/src/httprpc.cpp
index ed5a0253c6..493f1b0320 100644
--- i/src/httprpc.cpp
+++ w/src/httprpc.
...

💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081509904)
Maybe also worth adding the warning in the descriptions of `-rpcuser` and `-rpcpassword` in `init.cpp`:

https://github.com/bitcoin/bitcoin/blob/5b8752198e979ea4987d8b6238f42f8ed9a38846/src/init.cpp#L668

and

https://github.com/bitcoin/bitcoin/blob/5b8752198e979ea4987d8b6238f42f8ed9a38846/src/init.cpp#L664

💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081503770)
It is like this in `master`, but the meaning of `share/rpcauth` may not be obvious. Especially considering that some distros may not install it in their packages. Is not installed by our `cmake --install BUILDDIR` either. Maybe change to "See share/rpcauth in the source directory for more information".

💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081550947)
nit:
```suggestion
std::array<unsigned char, CHMAC_SHA256::OUTPUT_SIZE> out;
```

💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081564262)
Just noting: the way this works (unchanged by this PR) is that it allows duplicate usernames with different passwords (e.g. `joe:123` and `joe:456`) and would allow login with either password (`123` or `456`).

(just a comment, feel free to resolve right away)

💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081596938)
I did not test to confirm this, just read the code, but before this PR if `-rpcuser=joe -rpcpassword=abc:def` was configured, it would have been possible to login with user `joe:abc` and password `def`.

(just a comment, feel free to resolve right away)