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

💬 dergoegge commented on pull request "refactor: Remove need to pass chainparams to BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/27570#discussion_r1185186631)
I feel like in the long run, only having **one** way of getting the params would be the consistent thing to do. Storing a public reference to the params on various components seems weird given that the params are already a global (might as well use the global directly then?).

Also, why should these getters be part of the public interface for the block storage? Doesn't seem right to me.

🤔 theuni requested changes to a pull request: "Bugfix: Skip tests for tools not being built"
(https://github.com/bitcoin/bitcoin/pull/23027#pullrequestreview-1413433373)
@luke-jr If you're still interested in this, I agree with the previous reviewers:
- PR needs a description, if only for posterity
- As @fanquake mentioned, `ENABLE_BITCOIN_UTIL` could be used here.

I'm not at all familiar with the test framework, but this looks a lot like a re-implementation of something that's already cleanly abstracted. What exactly isn't getting correctly skipped?

📝 MarcoFalke opened a pull request: " ci: Remove CI_EXEC bloat in test/06_script_b.sh "
(https://github.com/bitcoin/bitcoin/pull/27573)
`CI_EXEC` has many issues:

* It is roughly equivalent to `bash -c "$*"`, meaning that the full command will be treated as a single string, ignoring tokens.
* It must be put in front of (almost) every command, making it easy to forget, hard to debug the resulting failure, and the code verbose.

Fix all issues in one script by removing it.

🚀 fanquake merged a pull request: "test: add coverage to rpc_scantxoutset.py"
(https://github.com/bitcoin/bitcoin/pull/27422)

📝 MarcoFalke converted_to_draft a pull request: "ci: Run iwyu on all src files"
(https://github.com/bitcoin/bitcoin/pull/27571)
This makes it easier to look at the CI output of a file without having to manually add it first to the list.

💬 mzumsande commented on issue "Slow memory leak in v22.0?":
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-1535074436)
> Hi facing similar issue. I'm running an rpc node.

Also 22.0? Which value of `rpcthreads` are you using?

🤔 Sosyetekadir reviewed a pull request: "refactor: Remove need to pass chainparams to BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/27570#pullrequestreview-1413526067)
Sosyete

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1185280732)
or to be lazier, just set the initial utxos' prevout.n index to some impossibly high numbers to ever be generated

💬 Sosyetekadir commented on pull request "refactor: Remove need to pass chainparams to BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/27570#issuecomment-1535096000)
Ben ve sen