💬 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
...
💬 maflcko commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724631464)
> Further considerations:
>
> * Consider enabling `libstdc++`s `_GLIBCXX_DEBUG _GLIBCXX_DEBUG_PEDANTIC` in debug builds. The problem is that ["Note that this flag changes the sizes and behavior of standard class templates such as std::vector, and therefore you can only link code compiled with debug mode and code compiled without debug mode if no instantiation of a container is passed between the two translation units."](https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html#d
...
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724631464)
> Further considerations:
>
> * Consider enabling `libstdc++`s `_GLIBCXX_DEBUG _GLIBCXX_DEBUG_PEDANTIC` in debug builds. The problem is that ["Note that this flag changes the sizes and behavior of standard class templates such as std::vector, and therefore you can only link code compiled with debug mode and code compiled without debug mode if no instantiation of a container is passed between the two translation units."](https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html#d
...
💬 saikiran57 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1995536108)
hi @furszy can you please check the fix.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1995536108)
hi @furszy can you please check the fix.
💬 vasild commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724652160)
Excellent! Removed from OP.
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724652160)
Excellent! Removed from OP.
⚠️ Sjors opened an issue: "depends: capnp build ignores config_opts"
(https://github.com/bitcoin/bitcoin/issues/32068)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
It seems that when building capnp via depends the `config_opts` in `capnp.mk` are ignored, i.e. `-DBUILD_TESTING=OFF`, `-DWITH_OPENSSL=OFF` and `-DWITH_ZLIB=OFF`.
This can be seen in the depends build log by e.g. `Built target capnp-heavy-tests`, a target that should be skipped entirely. It becomes a problem during the `cmake -B build --toolchain ...` step, because it will complain about
...
(https://github.com/bitcoin/bitcoin/issues/32068)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
It seems that when building capnp via depends the `config_opts` in `capnp.mk` are ignored, i.e. `-DBUILD_TESTING=OFF`, `-DWITH_OPENSSL=OFF` and `-DWITH_ZLIB=OFF`.
This can be seen in the depends build log by e.g. `Built target capnp-heavy-tests`, a target that should be skipped entirely. It becomes a problem during the `cmake -B build --toolchain ...` step, because it will complain about
...
💬 l0rinc commented on pull request "[IBD] flush UXTOs in bigger batches":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2724691617)
Reran the benchmarks on a Hetzner Linux machine with the new AssumeUTXO 880k set, parsing the logged flush times and plotting the results.
For different `dbcache` (440, 5000, 30000) and `dbbatchsize` (4-256MiB range, 16MiB (current) and 64MiB (proposed) are highlighted and trendline is added for clarity) sizes, each one twice for stability:
Sorting the same measurements might give us a better understanding of the trends:
<img width="1000" alt="image" src="https://github.com/user-attachments
...
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2724691617)
Reran the benchmarks on a Hetzner Linux machine with the new AssumeUTXO 880k set, parsing the logged flush times and plotting the results.
For different `dbcache` (440, 5000, 30000) and `dbbatchsize` (4-256MiB range, 16MiB (current) and 64MiB (proposed) are highlighted and trendline is added for clarity) sizes, each one twice for stability:
Sorting the same measurements might give us a better understanding of the trends:
<img width="1000" alt="image" src="https://github.com/user-attachments
...
💬 pinheadmz commented on pull request "build: Remove manpages when making MacOS app":
(https://github.com/bitcoin/bitcoin/pull/32064#issuecomment-2724739546)
post-merge tested ACK
successful guix build, code sign, and signature attach from current master, ran Qt and bitcoin on arm64/macos.
Didn't test x86: https://github.com/pinheadmz/bitcoin-detached-sigs/tree/master-698f86964c
(https://github.com/bitcoin/bitcoin/pull/32064#issuecomment-2724739546)
post-merge tested ACK
successful guix build, code sign, and signature attach from current master, ran Qt and bitcoin on arm64/macos.
Didn't test x86: https://github.com/pinheadmz/bitcoin-detached-sigs/tree/master-698f86964c