📝 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?

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421614259)
This is an aside -- in the cluster mempool branch, I think I'd like to change this to just look at direct parents rather than ancestors (I believe that in the normal course of transaction validation, we should no longer need to calculate ancestor sets when we're not worried about ancestor limits or updating cached ancestor state anymore). Can you think of any reason it would be problematic to replace ancestors with just an entry's direct parents here?