⚠️ arejula27 opened an issue: "Request for Wiki Edit Permissions – Testing Guide: Bitcoin Core 29.0 RC"
(https://github.com/bitcoin/bitcoin/issues/31984)
Hello, maintainers,
We are currently working on the Testing Guide: Bitcoin Core 29.0 Release Candidate as part of the [BOSS program](https://learning.chaincode.com/#seminars). We are:
@musaHaruna
@arejula27
@janb84
@Prabhat1308
We would like to request permission to edit the Wiki so we can contribute and publish our guide.
Looking forward to your response.
Thank you!
(https://github.com/bitcoin/bitcoin/issues/31984)
Hello, maintainers,
We are currently working on the Testing Guide: Bitcoin Core 29.0 Release Candidate as part of the [BOSS program](https://learning.chaincode.com/#seminars). We are:
@musaHaruna
@arejula27
@janb84
@Prabhat1308
We would like to request permission to edit the Wiki so we can contribute and publish our guide.
Looking forward to your response.
Thank you!
💬 laanwj commented on pull request "torcontrol: Limit reconnect timeout to max seconds and log delay in whole seconds":
(https://github.com/bitcoin/bitcoin/pull/31979#issuecomment-2697457225)
Concept ACK.
It's debatable whether we need an exponential backoff here in the first place. After all Tor is generally a local service, there is no risk of creating a DoS. i added it back then, to reduce the amount of log spam in case Tor isn't running. However we have a better logging system now that takes both category and level into account, so that motivation is no longer relevant.
(https://github.com/bitcoin/bitcoin/pull/31979#issuecomment-2697457225)
Concept ACK.
It's debatable whether we need an exponential backoff here in the first place. After all Tor is generally a local service, there is no risk of creating a DoS. i added it back then, to reduce the amount of log spam in case Tor isn't running. However we have a better logging system now that takes both category and level into account, so that motivation is no longer relevant.
💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1979362441)
Typo, maybe something like:
```suggestion
Using `lld` is required due to issues with Apple's `ld` and `LLVM`.
```
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1979362441)
Typo, maybe something like:
```suggestion
Using `lld` is required due to issues with Apple's `ld` and `LLVM`.
```
💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1979383156)
Nice, finally we can get rid of that (tested, seems to be working without the warning, good find)!
Should we anything to https://github.com/llvm/llvm-project/issues/102630?
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1979383156)
Nice, finally we can get rid of that (tested, seems to be working without the warning, good find)!
Should we anything to https://github.com/llvm/llvm-project/issues/102630?
💬 m3dwards commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1979412430)
gah, taken!
Thanks for the spot.
(https://github.com/bitcoin/bitcoin/pull/31954#discussion_r1979412430)
gah, taken!
Thanks for the spot.
💬 hebasto commented on pull request "scripted-diff: rename libmultiprocess repository":
(https://github.com/bitcoin/bitcoin/pull/31982#issuecomment-2697553252)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31982#issuecomment-2697553252)
Concept ACK.
💬 l0rinc commented on pull request "doc: update fuzz instructions when on macOS":
(https://github.com/bitcoin/bitcoin/pull/31954#issuecomment-2697572586)
ACK 75486c8ed87a480b9f0c4dc7a10f3cd4eee87b12
Compared to [previous review](https://github.com/bitcoin/bitcoin/compare/725c0fd95b15157bd0ec141cc24d27818f536656..75486c8ed87a480b9f0c4dc7a10f3cd4eee87b12#diff-7ff2e1d1ed59fa96766a663236d71bfdd00b4af3321fde39f7dbf022bd968a0a), `APPEND_LDFLAGS` was removed and the description simplified to avoid repetition.
(https://github.com/bitcoin/bitcoin/pull/31954#issuecomment-2697572586)
ACK 75486c8ed87a480b9f0c4dc7a10f3cd4eee87b12
Compared to [previous review](https://github.com/bitcoin/bitcoin/compare/725c0fd95b15157bd0ec141cc24d27818f536656..75486c8ed87a480b9f0c4dc7a10f3cd4eee87b12#diff-7ff2e1d1ed59fa96766a663236d71bfdd00b4af3321fde39f7dbf022bd968a0a), `APPEND_LDFLAGS` was removed and the description simplified to avoid repetition.
💬 laanwj commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2697629617)
Concept and code review ACK 4cd95a2921805f447a8bcecc6b448a365171eb93
Seems low-risk. As this is compile-time logic, i don't think this should actually affect the compiled code? (haven't verified, though)
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2697629617)
Concept and code review ACK 4cd95a2921805f447a8bcecc6b448a365171eb93
Seems low-risk. As this is compile-time logic, i don't think this should actually affect the compiled code? (haven't verified, though)
🤔 vasild reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2656773286)
Approach ACK 1245e05325b41101eddc76ba9214d910489e35b6
In the light of https://github.com/bitcoin/bitcoin/issues/31047 and https://github.com/bitcoin/bitcoin/pull/31394 I have been thinking that is would be useful to at least document the steps needed to gather coverage with clang. Thanks for the initiative!
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2656773286)
Approach ACK 1245e05325b41101eddc76ba9214d910489e35b6
In the light of https://github.com/bitcoin/bitcoin/issues/31047 and https://github.com/bitcoin/bitcoin/pull/31394 I have been thinking that is would be useful to at least document the steps needed to gather coverage with clang. Thanks for the initiative!
💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979133216)
This works, but maybe it is better to use the `APPEND_*` alternatives?
https://github.com/bitcoin/bitcoin/blob/15717f0ef3960969ee550a4a41741987b86684dc/CMakeLists.txt#L33-L37
i.e.
```suggestion
-DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping" \
-DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping"
```
If you decide to do that, then you will also need `-DAPPEND_LDFLAGS="-fprofile-instr-generate -fcoverage-mapping"` because `CMAKE_CXX_FLAGS` causes the f
...
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979133216)
This works, but maybe it is better to use the `APPEND_*` alternatives?
https://github.com/bitcoin/bitcoin/blob/15717f0ef3960969ee550a4a41741987b86684dc/CMakeLists.txt#L33-L37
i.e.
```suggestion
-DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping" \
-DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping"
```
If you decide to do that, then you will also need `-DAPPEND_LDFLAGS="-fprofile-instr-generate -fcoverage-mapping"` because `CMAKE_CXX_FLAGS` causes the f
...
💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979323892)
Why `-sparse`?
```
--sparse[=true|false]
Do not emit function records with 0 execution count. Can only be
used in conjunction with -instr. Defaults to false, since it can
inhibit compiler optimization during PGO.
```
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979323892)
Why `-sparse`?
```
--sparse[=true|false]
Do not emit function records with 0 execution count. Can only be
used in conjunction with -instr. Defaults to false, since it can
inhibit compiler optimization during PGO.
```
💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979077210)
This defaults to `CMAKE_BUILD_TYPE=RelWithDebInfo` which uses `-O2`. Using a debugger on an optimized binary usually results in bogus line numbers due to optimizations that can't be mapped back to source code lines. I do not know if it is the same with coverage.
To be on the safe side I have been using `-DCMAKE_BUILD_TYPE=Debug` (which uses `-O0`). Also, I don't see a reason to use `-O2`, other than to have the tests finish faster.
The example in https://clang.llvm.org/docs/UsersManual.htm
...
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979077210)
This defaults to `CMAKE_BUILD_TYPE=RelWithDebInfo` which uses `-O2`. Using a debugger on an optimized binary usually results in bogus line numbers due to optimizations that can't be mapped back to source code lines. I do not know if it is the same with coverage.
To be on the safe side I have been using `-DCMAKE_BUILD_TYPE=Debug` (which uses `-O0`). Also, I don't see a reason to use `-O2`, other than to have the tests finish faster.
The example in https://clang.llvm.org/docs/UsersManual.htm
...
💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979318687)
Using `%m` will help reduce the chance of `.profraw` files being overwritten in case the same PID is reused by the OS:
```suggestion
LLVM_PROFILE_FILE="default_%m_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs.
```
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#running-the-instrumented-program mentions:
> For a program such as the [Lit](https://llvm.org/docs/CommandGuide/lit.html) testing tool which invokes other programs, it may be necessary to set
...
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979318687)
Using `%m` will help reduce the chance of `.profraw` files being overwritten in case the same PID is reused by the OS:
```suggestion
LLVM_PROFILE_FILE="default_%m_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs.
```
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#running-the-instrumented-program mentions:
> For a program such as the [Lit](https://llvm.org/docs/CommandGuide/lit.html) testing tool which invokes other programs, it may be necessary to set
...
💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979457702)
If there are profile files from more than one executable, like from `test_bitcoin` and from `bitcoind` then the syntax of this changes a little bit: drop `build/src/test/test_bitcoin` and add `-object=build/src/test/test_bitcoin -object=build/src/bitcoind`:
```
llvm-cov show
--object=build/src/test/test_bitcoin \
--object=build/src/bitcoind \
--instr-profile=build/coverage.profdata \
--format=html \
--show-instantiation-summary \
--show-line-counts-or-regions \
...
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979457702)
If there are profile files from more than one executable, like from `test_bitcoin` and from `bitcoind` then the syntax of this changes a little bit: drop `build/src/test/test_bitcoin` and add `-object=build/src/test/test_bitcoin -object=build/src/bitcoind`:
```
llvm-cov show
--object=build/src/test/test_bitcoin \
--object=build/src/bitcoind \
--instr-profile=build/coverage.profdata \
--format=html \
--show-instantiation-summary \
--show-line-counts-or-regions \
...
💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979389115)
I would suggest collecting coverage from functional tests as well because that would change a bit the arguments of `llvm-cov` (see later) and because the path to the profile file has to be specified as an absolute path which is not obvious:
```md
Generating the raw profile data based on `ctest` execution:
```shell
LLVM_PROFILE_FILE=$(pwd)/build/raw_profile_data/%m_%p.profraw build/test/functional/test_runner.py
`` `
```
it has to be a full path because otherwise the `.profraw` file wi
...
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979389115)
I would suggest collecting coverage from functional tests as well because that would change a bit the arguments of `llvm-cov` (see later) and because the path to the profile file has to be specified as an absolute path which is not obvious:
```md
Generating the raw profile data based on `ctest` execution:
```shell
LLVM_PROFILE_FILE=$(pwd)/build/raw_profile_data/%m_%p.profraw build/test/functional/test_runner.py
`` `
```
it has to be a full path because otherwise the `.profraw` file wi
...
💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979463614)
Locally, I have been using this as well:
```
-ignore-filename-regex="src/crc32c/|src/leveldb/|src/minisketch/|src/secp256k1/|src/test/"
```
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1979463614)
Locally, I have been using this as well:
```
-ignore-filename-regex="src/crc32c/|src/leveldb/|src/minisketch/|src/secp256k1/|src/test/"
```
💬 marcofleon commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1979488543)
It seems like the conditional never ends up being true, which you can see in the test's [coverage](https://marcofleon.github.io/coverage/miniminer/coverage/root/bitcoin/src/test/fuzz/mini_miner.cpp.html).
In general, I do think you're right about wanting the fuzz test to cover a range of possible values, but I'm not quite familar enough with the mining code to know what the best approach here would be.
(https://github.com/bitcoin/bitcoin/pull/31803#discussion_r1979488543)
It seems like the conditional never ends up being true, which you can see in the test's [coverage](https://marcofleon.github.io/coverage/miniminer/coverage/root/bitcoin/src/test/fuzz/mini_miner.cpp.html).
In general, I do think you're right about wanting the fuzz test to cover a range of possible values, but I'm not quite familar enough with the mining code to know what the best approach here would be.
💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2697689385)
friendly ping: @sipa @darosior
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2697689385)
friendly ping: @sipa @darosior
🤔 rkrux reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2657415572)
Few comments.
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2657415572)
Few comments.
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1979442112)
What scenario is not tested if this flow is not done and the transaction generated above is mined directly?
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1979442112)
What scenario is not tested if this flow is not done and the transaction generated above is mined directly?
💬 rkrux commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1979452088)
The `descriptorprocesspsbt` RPC uses a different flow than the `walletprocesspsbt` RPC because it calls `ProcessPSBT()` instead of `FillPSBT()`. I think it'd be nice if this tests also ensures the sighash is added via the `descriptorprocesspsbt` RPC as well.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1979452088)
The `descriptorprocesspsbt` RPC uses a different flow than the `walletprocesspsbt` RPC because it calls `ProcessPSBT()` instead of `FillPSBT()`. I think it'd be nice if this tests also ensures the sighash is added via the `descriptorprocesspsbt` RPC as well.