💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515212570)
> Would still be nice to if there was a way to take all of it out of the hot path

Can you give me hints on how to do that?
Since we have a primitive as a key now, we already skip xor with 0 value now, see https://github.com/bitcoin/bitcoin/pull/31144/files#diff-4020c723bb55e114bdc7ff769086a765dcc7ccfb61da2047a315db16c0c7a8b4R295

> but wondering it it might be better to just use a sampling of the most common write sizes

@fanquake mentioned that he thinks this benchmark could be useful -
...

💬 maaku commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2515230237)
What I’m saying is: what difference does it make who submits it? Given the security model, it would be weird to gatekeep (hehe) it to the original developer. It’s just asking Apple to run some checks on the binary, that’s all.

It makes no difference at all if the submitter is the same as the signer. Lots of people take advantage of this workflow, so it isn’t a hidden gotcha.

Notarization isn’t code signing. The submission identity check is merely a DoS prevention mechanism for Apple’s gate
...

💬 achow101 commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2515240900)
> What I’m saying is: what difference does it make who submits it? Given the security model, it would be weird to gatekeep it (hehe) to the original developer. It’s just asking Apple to run some checks on the binary, that’s all.

There's no different; it doesn't matter.

That statement is not a complaint about notarization nor am I saying it is a reason to not notarize. It is simply an observation that I found surprising. If that were common knowledge, I would have assumed someone would have
...

💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515273927)
> Just clicking the cirrus links here it's hard to tell what the nature of these failures is, what may be causing them, and whether they happen reliably or randomly.

The nature of the failures is:

* They are caused by Wine.
* They happen randomly.
* They are known for years (A random mention of them from 2017 is https://github.com/bitcoin/bitcoin/blame/8e02b480591008c457cc841050f6f755ff54fcba/test/util/test_runner.py#L162). If you use a search engine you may find more or earlier mentions
...

💬 theuni commented on pull request "cmake: Add `CheckLinkerSupportsPIE` module":
(https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2515287092)
> This seems very fragile and unintuitive, and the fact that this could silently break at any point is not documented in any way. I don't think other bad design decisions should lead to us having to write even more boilerplate code to fix things that should "just work" (minus the upstream bugs).

Agreed. I forgot that we set `CMAKE_TRY_COMPILE_TARGET_TYPE` globally. And even worse, it's buried in a module. If that upsets CMake internal tests, I think we should undo that.

Could we invert thi
...

👍 maflcko approved a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2476490310)
lgtm

💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1868207380)
I am halfway thinking that this can be put under the `DANGER_CI_ON_HOST_CCACHE_FOLDER`. Either keeping the name or renaming it.

This avoids a complexity explosion for stuff that is only used by GHA IIRC.

🤔 maflcko reviewed a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2476495514)
It would be good to mention in the pull request description one sentence on why this is a good idea, instead of just a single word "fixes #ref" and a link to a GHA run.

💬 maflcko commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515307127)
> It seems like if you take the top 50 sizes it covers 99.6% of the writes

I had the same thought. Obviously there could be an unlikely problem if the remaining 0.04% of writes accounted for the majority of the time, but that seems unlikely. Other than that, taking only the top N seems preferable.

💬 theuni commented on pull request "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2515323391)
🤦🤦

Could you split this into 2 commits? The fixes you describe sound simple enough, but the removal of the helpers makes this hard to review.

💬 theuni commented on pull request "[POC] cmake: Introduce LLVM's Source-based Code Coverage reports":
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2515362105)
I haven't played with coverage in CMake much, but this seems like a more reasonable approach than the build type.

> Would we keep supporting generating coverage with gcc?

I don't think we should keep both approaches. If we do want to continue to support gcc, IMO it should be migrated to this setting rather than keeping it as a build type.

But.. there's a lot of duplicated/hard-coded logic here. Can we not deduce how to filter/what to run based on target properties?

💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2515462391)
@rkrux I updated to master and rebased. There were no conflicts and this test continues to pass without needing any modifications. This should be ready to merge

Note: I will update to multi-path descriptors now that it's merged in a tiny followup PR. but I don't want to mess with all the prior reviews on this PR so not introducing any new changes

💬 achow101 commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#issuecomment-2515463182)
ACK 492e1f09943fcb6145c21d470299305a19e17d8b

✅ achow101 closed an issue: "log/dump more information if a CheckQueue failure occurs"
(https://github.com/bitcoin/bitcoin/issues/30960)

🚀 achow101 merged a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112)

👍 theuni approved a pull request: "cmake: Fix `IF_CHECK_PASSED` option handling"
(https://github.com/bitcoin/bitcoin/pull/31231#pullrequestreview-2476680219)
utACK 97a18c85458b898fe5e3abda9528a2de442766ad

Confirmed that this indeed a list.

To other reviewers: the `string(STRIP "${foo} ${bar}" ${baz})` syntax is confusing. It's just concatenating foo and bar and storing it in baz with no whitespace at the beginning or end.

@hebasto Is there a way to get CMake to warn us about these? It seems like it should know that we were attempting a string operation on a list.

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1868331201)
(Could possibly use `collections.defaultdict(int)` instead of `Counter` iff I really need to retouch).

💬 hebasto commented on pull request "cmake: Fix `IF_CHECK_PASSED` option handling":
(https://github.com/bitcoin/bitcoin/pull/31231#issuecomment-2515497249)
> @hebasto Is there a way to get CMake to warn us about these? It seems like it should know that we were attempting a string operation on a list.

I'm not aware of any such embedded functionality. CMake's internally represents both "strings" and "lists" as strings. A list can safely be treated as a string when it contains fewer than two elements.

🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2476650402)
Thanks for the reviews!

Updated da108a6e5be220654a65b6613ee7eb2c4ddc8677 -> 02567bf2bef5997ea5f0765780d196f36d3053e8 ([`pr/wrap.5`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.5) -> [`pr/wrap.6`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.5..pr/wrap.6)) to fix windows build warning and making a change to avoid a potentially confusing behavior https://github.com/bitcoin/bitcoin/pull/31375#discussion_r18618148
...

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1868303275)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1861814807

> [b06c7ad](https://github.com/bitcoin/bitcoin/commit/b06c7ad0ae91102fe8cdcac6f86d627aace2219b): this is potentially confusing. If I build without gui, I expect `build/src/bitcoin gui` to fail. Right now it would launch any `bitcoin-qt` in my `$PATH`.

Agree this behavior is potentially confusing, and prevented it for now, but I'm not totally I sure see it as a downside. I like the simplicity of `bitcoin daemon` being
...