💬 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
💬 Sjors commented on issue "depends: capnp build ignores config_opts":
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2724754746)
The macOS native builds don't use depends, so #30975 would not pick this is up.
cc @ryanofsky
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2724754746)
The macOS native builds don't use depends, so #30975 would not pick this is up.
cc @ryanofsky
💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1995616163)
May I know what is the issue with current fix: It will handle negative and positive, "never" cases.
` auto requestTimestamp = GetImportTimestamp(request, now);
// if any one entity has valid timestamp we can skp this check
if (!all_rescan_value) {
if (requestTimestamp < 0) {
const UniValue& timestamp = request["timestamp"];
// set all_rescan_value true if requested timestamp is negative otherwise i
...
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1995616163)
May I know what is the issue with current fix: It will handle negative and positive, "never" cases.
` auto requestTimestamp = GetImportTimestamp(request, now);
// if any one entity has valid timestamp we can skp this check
if (!all_rescan_value) {
if (requestTimestamp < 0) {
const UniValue& timestamp = request["timestamp"];
// set all_rescan_value true if requested timestamp is negative otherwise i
...
💬 vasild commented on pull request "[draft] Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2724787234)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2724787234)
Concept ACK
💬 Sjors commented on issue "depends: capnp build ignores config_opts":
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2724799001)
I can also reproduce this on Intel macOS 13.7.4 with Xcode 15.2.
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2724799001)
I can also reproduce this on Intel macOS 13.7.4 with Xcode 15.2.
💬 ryanofsky commented on issue "depends: capnp build ignores config_opts":
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2724806057)
Not sure if I can reproduce this locally. At least if I run `make -C depends NO_QT=1 NO_USDT=1 NO_ZMQ=1 NO_BDB=1 MULTIPROCESS=1 capnp V=1` I do see cmake being called with `-DBUILD_TESTING=OFF -DWITH_OPENSSL=OFF -DWITH_ZLIB=OFF`.
Diffing the native and non-native packages though, I wonder if there's a chance this could caused by syntax affecting older version of make, because the non-native package seems to have an extra := following the define. Could try:
```diff
--- a/depends/packages/capnp.
...
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2724806057)
Not sure if I can reproduce this locally. At least if I run `make -C depends NO_QT=1 NO_USDT=1 NO_ZMQ=1 NO_BDB=1 MULTIPROCESS=1 capnp V=1` I do see cmake being called with `-DBUILD_TESTING=OFF -DWITH_OPENSSL=OFF -DWITH_ZLIB=OFF`.
Diffing the native and non-native packages though, I wonder if there's a chance this could caused by syntax affecting older version of make, because the non-native package seems to have an extra := following the define. Could try:
```diff
--- a/depends/packages/capnp.
...
💬 VolodymyrBg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#discussion_r1995660544)
Suggested change
return keyhash_to_p2pkh_script(payload)
elif version == 196: # testnet script hash
return scripthash_to_p2sh_script(payload)
# TODO: also support other address formats
elif version == 0: # mainnet pubkey hash
return keyhash_to_p2pkh_script(payload)
elif version == 5: # mainnet script hash
return scripthash_to_p2sh_script(payload)
if version == 111 or version == 0: # testnet or mainnet pubkey hash
return ke
...
(https://github.com/bitcoin/bitcoin/pull/32060#discussion_r1995660544)
Suggested change
return keyhash_to_p2pkh_script(payload)
elif version == 196: # testnet script hash
return scripthash_to_p2sh_script(payload)
# TODO: also support other address formats
elif version == 0: # mainnet pubkey hash
return keyhash_to_p2pkh_script(payload)
elif version == 5: # mainnet script hash
return scripthash_to_p2sh_script(payload)
if version == 111 or version == 0: # testnet or mainnet pubkey hash
return ke
...
💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995669473)
yikes, good catch. Not a bug, but it wouldn’t hurt to remove it (or place it in the correct position) in a small follow-up.
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995669473)
yikes, good catch. Not a bug, but it wouldn’t hurt to remove it (or place it in the correct position) in a small follow-up.
💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2724844748)
Thanks for the review @ryanofsky
> would be good to have a more general solution to providing buffered reads and writes
That's what I had originally (with a fixed-size buffer, so was even more general than your suggestion), making the call-site-diff trivial: https://github.com/bitcoin/bitcoin/pull/31539.
It was suggested by @theuni that it may be simpler to serialize to buffer directly. It does seem simpler to reason about this one, especially since they basically had the same speed. Wha
...
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2724844748)
Thanks for the review @ryanofsky
> would be good to have a more general solution to providing buffered reads and writes
That's what I had originally (with a fixed-size buffer, so was even more general than your suggestion), making the call-site-diff trivial: https://github.com/bitcoin/bitcoin/pull/31539.
It was suggested by @theuni that it may be simpler to serialize to buffer directly. It does seem simpler to reason about this one, especially since they basically had the same speed. Wha
...