💬 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.
💬 maflcko commented on issue "intermittent issue in wallet_reorgsrestore.py in test_reorg_handling_during_unclean_shutdown: FailedToStartError: [node 0] bitcoind exited with status 1 during initialization. Error: Cannot obtain a lock on directory":
(https://github.com/bitcoin/bitcoin/issues/32066#issuecomment-2724425092)
https://cirrus-ci.com/task/4579981042384896?logs=ci#L2285
(https://github.com/bitcoin/bitcoin/issues/32066#issuecomment-2724425092)
https://cirrus-ci.com/task/4579981042384896?logs=ci#L2285
👋 maflcko's pull request is ready for review: "test: Rename send_message to send_without_ping"
(https://github.com/bitcoin/bitcoin/pull/31859)
(https://github.com/bitcoin/bitcoin/pull/31859)
💬 maflcko commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2724442464)
Looking at the diff, this seems to be dropping the linux restriction, so "adding support for macOS" doesn't seem entirely accurate. It could be better to say "drop linux restriction" or "add support for non-linux". For example, OpenBSD may work as well now.
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2724442464)
Looking at the diff, this seems to be dropping the linux restriction, so "adding support for macOS" doesn't seem entirely accurate. It could be better to say "drop linux restriction" or "add support for non-linux". For example, OpenBSD may work as well now.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995416821)
Thank you for highlighting this. I started looking into it and found a language level `assert` at the start of this function. Doesn't that make the `Assert` inside the for-loop redundant like it was before this change as well?
https://github.com/bitcoin/bitcoin/blob/698f86964c68041d938aaf54fdd39466266c371c/src/wallet/wallet.cpp#L1537-L1552
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995416821)
Thank you for highlighting this. I started looking into it and found a language level `assert` at the start of this function. Doesn't that make the `Assert` inside the for-loop redundant like it was before this change as well?
https://github.com/bitcoin/bitcoin/blob/698f86964c68041d938aaf54fdd39466266c371c/src/wallet/wallet.cpp#L1537-L1552
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995431543)
Oh I see this comment referred to the new implementation of this older function caused by the introduction of function overloading here. Yeah, I am not in favour of this as well, seem unnecessary for just a `version` param, the function signature defaults can suffice.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995431543)
Oh I see this comment referred to the new implementation of this older function caused by the introduction of function overloading here. Yeah, I am not in favour of this as well, seem unnecessary for just a `version` param, the function signature defaults can suffice.