🤔 hodlinator reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2716185555)
Reviewed/tested 3b5d00f65ae37b733db74d9c9c905eb35e7363e5

💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2013601414)
`$(pwd)` is redundant for `mkdir`:
```suggestion
mkdir -p build/raw_profile_data
```

💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2013604383)
nit: Could be more precise, use leading Capital letter like other comments.
```suggestion
# Merge all the raw profile data into a single file
find build/raw_profile_data -name "*.profraw" | xargs llvm-profdata merge -o build/coverage.profdata
```

💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2013599427)
Thread https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1999113012:

`open` is MacOS-specific AFAIK. I don't have it on my Linux install. Also overly repetitive/verbose IMO.

💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2013595756)
Thread https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1999092770:

nit: Would prefer not repeating the text below, and think mentioning brew is overly verbose, adding "instead" is sufficient IMO:
```
# MacOS may instead require `-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++"`
```

💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2013563924)
Thread https://github.com/bitcoin/bitcoin/pull/31933/files#r1976483663:

@Prabhat1308 what do you think about shrinking the title?

💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2013585005)
Thread https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979323892:

With `-sparse`:
```
warning: 16 functions have mismatched data
```
Without `-sparse` (current version of PR):
```
warning: 65 functions have mismatched data
```

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2013618590)
> > and provided a working proof of concept that addresses #30206.
>
> Thanks, however I tested that with GCC removed, and it didn't work?

Which standard library are you trying to build with? If it's `libstdc++`, then it must still be available somehow after removing `gcc-toolchain`, right?

> (from the new commit)
>
> > Previously, we explicitly unset these variables when invoking clang for
> > cross-compiling; however, that approach proved suboptimal (see #30451).
>
> What is su
...

💬 maflcko commented on issue "RFC: Macro Regression Test Suite for Historical Reorgs":
(https://github.com/bitcoin/bitcoin/issues/32130#issuecomment-2753618413)
I'd say the testing bias should be towards full coverage of the consensus rules via synthetic tests (whether it is a unit, fuzz, or functional test). Obviously it can't hurt to confirm this with real data, but as soon as a shortcoming or lack of testing is seen, it should be added as a "synthetic" test.

Not sure if https://github.com/bitcoin-data/stale-blocks includes invalid blocks, but if this test suite is done, it could make sense to include and check invalid blocks as well.

💬 Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2013640368)
done.

💬 Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2013640536)
done.

💬 Prabhat1308 commented on pull request "doc: Add Clang/LLVM based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2753626486)
Addressed the reviews by @hodlinator

💬 hodlinator commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#issuecomment-2753647810)
Switched to stripped-down version of this PR without the former attempt to introduce `assert_true()`. It seemed fortuitous that I found these examples to illustrate it with, but will kick that can down the road for now.

💬 ajtowns commented on pull request "doc: Update comments for AreInputsStandard to match code":
(https://github.com/bitcoin/bitcoin/pull/32129#discussion_r2013661749)
Added some text.

💬 maflcko commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#issuecomment-2753665544)
lgtm ACK 35d17cd5eecabd36a2a14b35a47de22726ede0f8

👍 TheCharlatan approved a pull request: "lint: Remove needless borrow to fix Clippy warning"
(https://github.com/bitcoin/bitcoin/pull/32144#pullrequestreview-2716372123)
ACK e3ce2bd9829bbe14e5da26505ac9d68ae0d2af2d

👍 hodlinator approved a pull request: "doc: Add Clang/LLVM based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2716373281)
re-ACK b96f1a696aa792068b5a4fa16e2d4a342e4f55b8

Thanks for addressing my feedback!

Still think it's strange that removing `-sparse` increases the functions with [mismatched data](https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2013585005). But it's certainly not blocking for me.

💬 ajtowns commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2013687204)
> I don't think I the understand reason for ABORT_ON_FAILED_ASSUME affecting return value of EnableFuzzDeterminism(), and trying to force non-determinism when assume doesn't abort.

The initial thought was actually more that the running the fuzz tests without Assumes being checked isn't going to be anywhere near as thorough, particularly influenced by the logic documented in #32100. Added some comment text on this.

💬 maflcko commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2753704261)
re-review-only-ACK 1f072cfb108d8f71daf6281d97353ff755af9569

Only change is a comment

🤔 janb84 reviewed a pull request: "doc: Add Clang/LLVM based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2716463180)
Re ACK [b96f1a6](https://github.com/bitcoin/bitcoin/commit/b96f1a696aa792068b5a4fa16e2d4a342e4f55b8)

After reading the discussion and the Clang documentation, I think that the call to remove the DEBUG build type is a good call. Agree with other changes.
Good work !