💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185038780)
use `SIGHASH_ALL` for clarity

🤔 instagibbs reviewed a pull request: "rpc: add `descriptorprocesspsbt` rpc"
(https://github.com/bitcoin/bitcoin/pull/25796#pullrequestreview-1413132311)
concept ACK, would appreciate test tighten-up for maintainability

💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185068008)
key_info.privkey?

💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185042239)
future work: someone make this a subroutine since this shows up in many places

💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185072414)
use named tuple args where you can

💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185073629)
Can you leave comments on each step to make it clear what's happening? It's been a few minutes of staring and I'm not quite sure what's exactly being tested here, making it difficult to update in the future.

My basic test flow would probably follow the lines of:

1) pick a node that doesn't have a wallet
2) make a single wpkh utxo
3) make single input psbt with that utxo
4) make sure it signs it when expected, and finalizes when expected (e.g., give it a descriptor unrelated to the utxo,
...

💬 instagibbs commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1185068135)
key_info.p2wpkh_addr?

🤔 theuni requested changes to a pull request: "Enable -Wstring-concatenation and -Wstring-conversion on clang builds"
(https://github.com/bitcoin/bitcoin/pull/26288#pullrequestreview-1413259393)
Concept ACK. This has been opened for a while, I'm curious to know if any new violations have snuck in since?

💬 theuni commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#discussion_r1185107526)
Likewise here.

💬 theuni commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#discussion_r1185107056)
Could you please split this up like this instead?
```c++
auto foo = ReadRawBlockFromDisk(...);
assert(foo);
```
That way the we don't introduce a new side-effect of `NDEBUG=1`

💬 theuni commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#discussion_r1185113523)
Imo it'd be cleaner and more readable to just leave the logic here as before, turning the wonky string assertions into comments.

💬 dergoegge commented on pull request "refactor: Remove need to pass chainparams to BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/27570#discussion_r1185134526)
Could these be private?

🤔 dergoegge reviewed a pull request: "refactor: Remove need to pass chainparams to BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/27570#pullrequestreview-1413309561)
> Yes, this is what this pull is doing.

Sorry, I was confused because Consensus::Params are stil being passed to `ReadBlockFromDisk`.

💬 hebasto commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#issuecomment-1534925772)
> Concept ACK. This has been open for a while, I'm curious to know if any new violations have snuck in since?

Maybe rebase it?

🤔 theuni requested changes to a pull request: "Enable HW-accelerated implementations of SHA256 for MSVC builds"
(https://github.com/bitcoin/bitcoin/pull/24773#pullrequestreview-1413320461)
Concept ACK. Coming to this after kill/shill/merge, sorry for the late re-review.

Would you mind splitting up the inline changes from the rest? Looks good to me, but as it introduces a general abstraction in the crypto code I think it'd be nice not to restrict review to win32 devs for that part.

After that the changes here look pretty uncontroversial to me (modulo actually verifying the cpuid bitwise stuff).

💬 MarcoFalke commented on pull request "refactor: Remove need to pass chainparams to BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/27570#discussion_r1185151993)
Yes, but why?

* They are also public in chainman (consistency)
* They return a read-only const reference, so there should be no risk
* Making them public now avoids code churn in the future when the need to be made public

💬 achow101 commented on pull request "test: various `converttopsbt` check cleanups in rpc_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/27325#issuecomment-1534939077)
ACK afc2dd54848fa70ed408ae259420ff8648f21efc

💬 hebasto commented on pull request "Enable HW-accelerated implementations of SHA256 for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/24773#issuecomment-1534948208)
> Would you mind splitting up the inline changes from the rest?

Just to clarify your point, you mean to put all s/`inline __attribute__((always_inline))`/`ALWAYS_INLINE`/ into the "Introduce platform-agnostic `ALWAYS_INLINE` macro" commit?

🚀 achow101 merged a pull request: "test: various `converttopsbt` check cleanups in rpc_psbt.py"
(https://github.com/bitcoin/bitcoin/pull/27325)

💬 theuni commented on pull request "Enable HW-accelerated implementations of SHA256 for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/24773#issuecomment-1534954977)
Sorry, yes, I meant to factor out those changes into a separate PR.