💬 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
```
💬 maflcko commented on pull request "test: Rename send_message to send_without_ping":
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2724589926)
rebased and taken out of draft. This should be ready for review, as all conflicts are draft pull requests.
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2724589926)
rebased and taken out of draft. This should be ready for review, as all conflicts are draft pull requests.
💬 laanwj commented on pull request "[draft] Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2724599518)
Concept ACK, nice work
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2724599518)
Concept ACK, nice work
💬 vasild commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995506801)
Dropped changes to `depends/hosts/linux.mk`.
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995506801)
Dropped changes to `depends/hosts/linux.mk`.
💬 vasild commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995510268)
I didn't realize that these flags are used also for the libraries. Dropped these changes.
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995510268)
I didn't realize that these flags are used also for the libraries. Dropped these changes.
💬 vasild commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995511934)
Given the above I left non-debug builds alone.
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995511934)
Given the above I left non-debug builds alone.
💬 vasild commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724614671)
`8cf75a4...a3a799c`: reduce the scope of this to only add hardening on libc++ when in debug mode, updated the OP.
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724614671)
`8cf75a4...a3a799c`: reduce the scope of this to only add hardening on libc++ when in debug mode, updated the OP.
👋 vasild's pull request is ready for review: "build: enable libc++ hardening"
(https://github.com/bitcoin/bitcoin/pull/31424)
(https://github.com/bitcoin/bitcoin/pull/31424)
💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2724627981)
Reviewed the first four commits up to 0d6e10674ea8578ddcf75318b25e86dee430bc09.
- I've been rebasing #30975 and #31802 on top of this for increased CI coverage and Guix builds. Initially this found several bugs / issues, but recently it's been smooth sailing.
- I ran `git-subtree-check.sh` (see PR description)
- the scripted diff to use `ENABLE_IPC` is trivial
- I tested c27d3a710b844e845075dea7e51f8f368ebf409f "cmake: Support building with libmultiprocess subtree" on macOS 15.3.2 by build
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2724627981)
Reviewed the first four commits up to 0d6e10674ea8578ddcf75318b25e86dee430bc09.
- I've been rebasing #30975 and #31802 on top of this for increased CI coverage and Guix builds. Initially this found several bugs / issues, but recently it's been smooth sailing.
- I ran `git-subtree-check.sh` (see PR description)
- the scripted diff to use `ENABLE_IPC` is trivial
- I tested c27d3a710b844e845075dea7e51f8f368ebf409f "cmake: Support building with libmultiprocess subtree" on macOS 15.3.2 by build
...