💬 janb84 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2724120384)
> > See [Readme](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md) on how to test this for deterministic unit test
>
> I guess this can be used to test both (unit and fuzz tests)? If yes, it could make sense to recommend a unit test and fuzz test, each. For example `util_string_tests` and `addition_overflow` (haven't tested either)? Before, the coverage should be non-deterministic, and after this change, it should be deterministic.
Have incorporated your suggestion
...
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2724120384)
> > See [Readme](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md) on how to test this for deterministic unit test
>
> I guess this can be used to test both (unit and fuzz tests)? If yes, it could make sense to recommend a unit test and fuzz test, each. For example `util_string_tests` and `addition_overflow` (haven't tested either)? Before, the coverage should be non-deterministic, and after this change, it should be deterministic.
Have incorporated your suggestion
...
💬 laanwj commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995241573)
At some point we might want to have something like `-salvage` for corrupted sqlite wallets, but makes sense to remove it for now.
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995241573)
At some point we might want to have something like `-salvage` for corrupted sqlite wallets, but makes sense to remove it for now.
👍 brunoerg approved a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2684995699)
reACK 32a522f2d825957f0c85d7b4ea9185a053b018e3
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2684995699)
reACK 32a522f2d825957f0c85d7b4ea9185a053b018e3
💬 laanwj commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995257444)
missing `return false;`?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995257444)
missing `return false;`?
💬 hodlinator commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1995271241)
Thanks, yes it often takes a little while to sync up so I think 0.1 would be slightly better than the default 0.05. Will do if I re-touch.
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1995271241)
Thanks, yes it often takes a little while to sync up so I think 0.1 would be slightly better than the default 0.05. Will do if I re-touch.
💬 laanwj commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995271595)
We could, but including binary files in the repo should be a last resort. The approach of creating them from the functional tests is preferable.
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995271595)
We could, but including binary files in the repo should be a last resort. The approach of creating them from the functional tests is preferable.
💬 hodlinator commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995292651)
Not sure I've ever come across this corner of C/C++ before. So you define `__llvm_profile_reset_counters(void) __attribute__((weak))` for it to hopefully link to something external, but then define local implements for them in case we don't link to code coverage libs (build with code coverage enabled)? So for when we include this test-related file but only include unit tests, and build without coverage support?
From the PR description:
> This change extends the functionality of the functio
...
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995292651)
Not sure I've ever come across this corner of C/C++ before. So you define `__llvm_profile_reset_counters(void) __attribute__((weak))` for it to hopefully link to something external, but then define local implements for them in case we don't link to code coverage libs (build with code coverage enabled)? So for when we include this test-related file but only include unit tests, and build without coverage support?
From the PR description:
> This change extends the functionality of the functio
...
💬 laanwj commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995301752)
nit: don't we normally use same-line `} else {`?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995301752)
nit: don't we normally use same-line `} else {`?
💬 laanwj commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995304691)
Why do we compare strings here instead of use the `enum DatabaseFormat`?
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1995304691)
Why do we compare strings here instead of use the `enum DatabaseFormat`?
💬 hodlinator commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995324930)
I think since we don't set `shell` for this specific step, we use [PowerShell by default on Windows](https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell). Slightly unclear [documentation](https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference) on what the expected exit code behavior is.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995324930)
I think since we don't set `shell` for this specific step, we use [PowerShell by default on Windows](https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell). Slightly unclear [documentation](https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference) on what the expected exit code behavior is.
💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1995330440)
@ldionne, do you know if `-O2` could have any adverse effects on coverage created with clang's `-fprofile-instr-generate -fcoverage-mapping`?
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1995330440)
@ldionne, do you know if `-O2` could have any adverse effects on coverage created with clang's `-fprofile-instr-generate -fcoverage-mapping`?
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995353888)
Thanks for the doc links. I guess this means there is no reliable way to get fail-fast behavior as expected by default on GHA for Windows?
Also, it looks like it would be possible to set a default shell. Also, it looks like it would be possible to use bash from Git on Windows.
Thus, I wonder if the default shell could be set to bash globally for our GHA to get reliable fail-fast.
An alternative would be to implement the CI logic in a standalone script with a well-defined language and be
...
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995353888)
Thanks for the doc links. I guess this means there is no reliable way to get fail-fast behavior as expected by default on GHA for Windows?
Also, it looks like it would be possible to set a default shell. Also, it looks like it would be possible to use bash from Git on Windows.
Thus, I wonder if the default shell could be set to bash globally for our GHA to get reliable fail-fast.
An alternative would be to implement the CI logic in a standalone script with a well-defined language and be
...
💬 hodlinator commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995369157)
There might be some limited value in using cmd/PowerShell on Windows in order to execute in a closer-to-end-user environment and tolerate the "&& \`". But I'm not totally against switching to bash/scripts either.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995369157)
There might be some limited value in using cmd/PowerShell on Windows in order to execute in a closer-to-end-user environment and tolerate the "&& \`". But I'm not totally against switching to bash/scripts either.
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995371508)
These docs are indeed outdated, last update was 4 years back: https://github.com/bitcoin-dot-org/developer.bitcoin.org. Only the code in this repo should be treated as source of truth.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995371508)
These docs are indeed outdated, last update was 4 years back: https://github.com/bitcoin-dot-org/developer.bitcoin.org. Only the code in this repo should be treated as source of truth.
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995371604)
These docs are indeed outdated, last update was 4 years back: https://github.com/bitcoin-dot-org/developer.bitcoin.org. Only the code in this repo should be treated as source of truth.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995371604)
These docs are indeed outdated, last update was 4 years back: https://github.com/bitcoin-dot-org/developer.bitcoin.org. Only the code in this repo should be treated as source of truth.
👍 vasild approved a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2685168897)
Almost ACK 7f57ad00d301b9564ece511d2e99cb0678b21174, just fix the path to the binaries which were recently relocated from `src` to `bin`.
I would be happy to review an extension to this that also does the fuzz part. That would involve compiling into another directory for fuzzing, running the fuzz tests, gathering their `.profraw` files as well and adding `--object=buildfuzz/bin/fuzz` to `llvm-cov`.
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2685168897)
Almost ACK 7f57ad00d301b9564ece511d2e99cb0678b21174, just fix the path to the binaries which were recently relocated from `src` to `bin`.
I would be happy to review an extension to this that also does the fuzz part. That would involve compiling into another directory for fuzzing, running the fuzz tests, gathering their `.profraw` files as well and adding `--object=buildfuzz/bin/fuzz` to `llvm-cov`.
💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1995352412)
nit: `test_runner.py` also supports `-j N` for parallel jobs.
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1995352412)
nit: `test_runner.py` also supports `-j N` for parallel jobs.
💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1995360840)
After https://github.com/bitcoin/bitcoin/pull/31161 the binaries are now in `bin/`:
```suggestion
--object=build/bin/test_bitcoin \
--object=build/bin/bitcoind \
```
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1995360840)
After https://github.com/bitcoin/bitcoin/pull/31161 the binaries are now in `bin/`:
```suggestion
--object=build/bin/test_bitcoin \
--object=build/bin/bitcoind \
```
💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995384046)
> Is this comment outdated? I don't understand where are the default args here that are referred to in the above comment.
In C++, function overloading can be used as a tool to set default values for function parameters. This is done here, by setting the default value in the function implementation, as opposed to in the function signature.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995384046)
> Is this comment outdated? I don't understand where are the default args here that are referred to in the above comment.
In C++, function overloading can be used as a tool to set default values for function parameters. This is done here, by setting the default value in the function implementation, as opposed to in the function signature.
💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995396151)
For me, it is not so much about tolerating the ugly "&& `", but more about it being easy to forget in the future, or it already being missing in other run commands. Allowing a foot-gun that allows the CI to silently pass on failures seems like it should be avoided, but no strong opinion, if reviewers are fine with this.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995396151)
For me, it is not so much about tolerating the ugly "&& `", but more about it being easy to forget in the future, or it already being missing in other run commands. Allowing a foot-gun that allows the CI to silently pass on failures seems like it should be avoided, but no strong opinion, if reviewers are fine with this.