💬 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.
💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995454609)
I was trying to say in https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986292978 that a default doesn't make sense at all here (in any way), because:
* Every call site that omits the default has the same fallback, so changing the default in the future will make review harder.
* Every call site needs to document the default in the RPC help anyway, so simply using the default from the docs is self-consistent and self-documenting and avoids the need for a default here
Finally, def
...
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995454609)
I was trying to say in https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986292978 that a default doesn't make sense at all here (in any way), because:
* Every call site that omits the default has the same fallback, so changing the default in the future will make review harder.
* Every call site needs to document the default in the RPC help anyway, so simply using the default from the docs is self-consistent and self-documenting and avoids the need for a default here
Finally, def
...
⚠️ polespinasa opened an issue: "Doc: Mempool Policy documentation Outdated since TRUC"
(https://github.com/bitcoin/bitcoin/issues/32067)
Checking the [policy documentation](https://github.com/bitcoin/bitcoin/tree/master/doc/policy) I think I noticed that some things are outdated since Version 3 transaction relay / TRUC.
Some sentences [like](https://github.com/bitcoin/bitcoin/blob/master/doc/policy/packages.md#package-fees-and-feerate):
> Note: Package feerate cannot be used to meet the minimum relay feerate (-minrelaytxfee) requirement. For example, if the mempool minimum feerate is 5sat/vB and the minimum relay feerate is se
...
(https://github.com/bitcoin/bitcoin/issues/32067)
Checking the [policy documentation](https://github.com/bitcoin/bitcoin/tree/master/doc/policy) I think I noticed that some things are outdated since Version 3 transaction relay / TRUC.
Some sentences [like](https://github.com/bitcoin/bitcoin/blob/master/doc/policy/packages.md#package-fees-and-feerate):
> Note: Package feerate cannot be used to meet the minimum relay feerate (-minrelaytxfee) requirement. For example, if the mempool minimum feerate is 5sat/vB and the minimum relay feerate is se
...
💬 hodlinator commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995495622)
[Seems](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/setting-a-default-shell-and-working-directory#example-set-the-default-shell-and-working-directory) like we might be able override default shell at the top of ci.yml file:
```
defaults:
run:
shell: bash
```
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995495622)
[Seems](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/setting-a-default-shell-and-working-directory#example-set-the-default-shell-and-working-directory) like we might be able override default shell at the top of ci.yml file:
```
defaults:
run:
shell: bash
```