Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 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
...
💬 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.
💬 vasild commented on pull request "build: enable libc++ hardening":
(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
...
💬 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
...
💬 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
💬 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
💬 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
...
💬 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
💬 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.
💬 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.
...
💬 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
...
💬 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.
💬 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
...
🤔 hebasto reviewed a pull request: "build: Remove manpages when making MacOS app"
(https://github.com/bitcoin/bitcoin/pull/32064#pullrequestreview-2685743124)
Post-merge ACK 80b5e7f2cb7fbfbd724e1f52b00c0e72b79a200b.
💬 maflcko commented on pull request "RFC: Add `operator""_uint256` compile-time user-defined literal":
(https://github.com/bitcoin/bitcoin/pull/31991#issuecomment-2724882650)
> I thought of removing the `consteval_ctor` hack as well, now that [consteval conversion is claimed to work in visual studio](https://developercommunity.visualstudio.com/t/consteval-conversion-function-fails/1579014#T-N10721450), but since https://godbolt.org/z/j837req5h still seems to fail for latest `x64 msvc v19`, I postponed doing that.

I think the upstream bug link may be wrong. https://developercommunity.visualstudio.com/t/Can-not-initialize-array-with-consteval/10828473 looks closer
💬 maflcko commented on pull request "RFC: Add `operator""_uint256` compile-time user-defined literal":
(https://github.com/bitcoin/bitcoin/pull/31991#discussion_r1995691590)
Not sure what the goal here is, or the improvement. Looks like extra code is added to do the same thing in two different ways?
🤔 maflcko reviewed a pull request: "RFC: Add `operator""_uint256` compile-time user-defined literal"
(https://github.com/bitcoin/bitcoin/pull/31991#pullrequestreview-2685765529)
tend toward NACK for now, as I don't see the goal or benefit

Would be happy to ack a doc fix of the upstream msvc bug report
💬 maflcko commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#issuecomment-2724895783)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 Sjors commented on issue "depends: capnp build ignores config_opts":
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2724899476)
Both macs have GNU Make 3.81

Trying your modification. If it works, it's yet another reason to migrate depends to cmake...?