💬 hebasto commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1848422903)
Concept ACK.

> This silences the following (clang 18):
>
> ```
> common/args.cpp:288:31: error: returning variable 'm_cached_blocks_path' by reference requires holding mutex 'cs_args' [-Werror,-Wthread-safety-reference-return]
> 288 | if (!path.empty()) return path;
> | ^
> ```

Does it happen on the master branch?

📝 martinus opened a pull request: "fuzz/util: make FuzzedDataProvider & FastRandomContext usage deterministic"
(https://github.com/bitcoin/bitcoin/pull/29043)
There exist many usages of `fuzzed_data_provider` where it is evaluated directly in the function call.
Unfortunately, the order of evaluation of function arguments is unspecified, and a simple example shows that it can differ e.g. between clang++ and g++: https://godbolt.org/z/jooMezWWY

When the evaluation order is not consistent, the same fuzzing/random input will produce different output, which is bad for coverage/reproducibility. This PR fixes all these cases I have found where unspecifie
...

💬 martinus commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1421427984)
I created #29043

💬 murchandamus commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#discussion_r1421429261)
Sure, I updated it to use the same formula in both lines, so that only the parameters appear in either order

💬 ismaelsadeeq commented on pull request "fuzz/util: make FuzzedDataProvider & FastRandomContext usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1848452118)
Concept ACK

📝 hebasto opened a pull request: "msvc: Define the same `QT_...` macros as in Autotools builds"
(https://github.com/bitcoin/bitcoin/pull/29044)
There are no reasons to have such a diversion.

Also it fixes https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1847971114.

💬 hebasto commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1848453153)
> It's something to do with `CMainSignals&`. Wherever it's used in libbitcoin_qt the msvc compiler generates a syntax error of:
>
> `Error C2238 unexpected token(s) preceding ';'`

Fixed in #29044.

💬 ismaelsadeeq commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#issuecomment-1848453371)
Thanks for reviewing @martinus @maflcko
Forced pushed from https://github.com/bitcoin/bitcoin/commit/fab46cc454f94b33dda6bc68a7234b439310f358 to https://github.com/bitcoin/bitcoin/commit/f8ef5dbc8471b711d233e3bec64597498a641252
to fix CI failure
[compare_changes](https://github.com/bitcoin/bitcoin/compare/fab46cc454f94b33dda6bc68a7234b439310f358..f8ef5dbc8471b711d233e3bec64597498a641252)

💬 ismaelsadeeq commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1421441743)
Thanks @martinus Will review

📝 hebasto opened a pull request: "msvc: Optimize "Release" builds"
(https://github.com/bitcoin/bitcoin/pull/29045)
It is awkward not using optimization.

This PR reduces the duration of functional tests by an hour.

💬 hebasto commented on pull request "msvc: Optimize "Release" builds":
(https://github.com/bitcoin/bitcoin/pull/29045#issuecomment-1848456277)
Friendly ping @sipsorcery :)

👍 hebasto approved a pull request: "util: Remove DirIsWritable, GetUniquePath"
(https://github.com/bitcoin/bitcoin/pull/28075#pullrequestreview-1773778627)
ACK fa3da629a1aebcb4500803d7417feed8e34285b0, I have reviewed the code and it looks OK.

💬 Riahiamirreza commented on pull request "rpc: show P2(W)SH redeemScript in getrawtransaction #27637":
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-1848591715)
@sipa Would you please review my PR? I really appreciate it.

💬 martinus commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1848609143)
I updated my PR by removing the changes that I did in initializer-lists. These were not necessary, evaluation order in these is well defined.

💬 sipsorcery commented on pull request "msvc: Define the same `QT_...` macros as in Autotools builds":
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848766633)
@hebasto what version of QT are you using for Windows builds?

It's been a while since I did a QT build and I'm having lots of challenges with `5.15.11` on `master`.

💬 hebasto commented on pull request "msvc: Define the same `QT_...` macros as in Autotools builds":
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848769593)
> @hebasto what version of QT are you using for Windows builds?

Qt 5.15.11

> It's been a while since I did a QT build and I'm having lots of challenges with `5.15.11` on `master`.

I've made another Qt 5.15.11 build just today with no issues.

💬 sipsorcery commented on pull request "msvc: Define the same `QT_...` macros as in Autotools builds":
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848781032)
tACK 1a5dae630df1eef9eac51557b2f1c5dba0924953.

💬 sipsorcery commented on pull request "msvc: Optimize "Release" builds":
(https://github.com/bitcoin/bitcoin/pull/29045#issuecomment-1848790814)
tACK 6e0f1d2abbb700d4fd4b956a7d1f9505b653653c.

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421613674)
Leftover comment?

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421615040)
Not sure if I'm missing something, but this only checks that the v3 inheritance rules are enforced among transactions in the package. What if someone sent a package of 2 v3 transactions, but the child transaction also spent a transaction that was already in the mempool? I think the `CheckV3Inheritance()` needs to be able to pull parents from both the mempool and from the package, perhaps?