💬 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
...
🤔 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.
(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
(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?
(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
(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
(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...?
(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...?
📝 furszy opened a pull request: "test: fix intermittent failure in wallet_reorgsrestore.py"
(https://github.com/bitcoin/bitcoin/pull/32069)
In response to #32066 intermittent failure.
Wait until the node's process has fully stopped before starting a new instance of it.
(https://github.com/bitcoin/bitcoin/pull/32069)
In response to #32066 intermittent failure.
Wait until the node's process has fully stopped before starting a new instance of it.
💬 furszy 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-2724902606)
#32069 attempts to fix it.
(https://github.com/bitcoin/bitcoin/issues/32066#issuecomment-2724902606)
#32069 attempts to fix it.
✅ l0rinc closed a pull request: "RFC: Add `operator""_uint256` compile-time user-defined literal"
(https://github.com/bitcoin/bitcoin/pull/31991)
(https://github.com/bitcoin/bitcoin/pull/31991)
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1995695052)
Thanks for taking the time to elaborate.
"Initialization phase" was too terse for me - of course we're in the initialization phase... duh. :)
Changed to "Suppress these as they are expected during initialization."
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1995695052)
Thanks for taking the time to elaborate.
"Initialization phase" was too terse for me - of course we're in the initialization phase... duh. :)
Changed to "Suppress these as they are expected during initialization."
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1995687071)
Correct, especially now after maflcko's change in fae3bf6b870eb0f9cddd1adac82ba72890806ae3 to not call `stop()` on test failure, I agree it makes sense to assert instead, good suggestion! Fixed.
> were there actually any cases where CannotSendRequest exceptions were caught and suppressed? If so what were those cases? Commit description should say what goals of this change are and if CannotSendRequest is an actual or theoretical change in behavior.
It was a common knock-on issue from attemp
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1995687071)
Correct, especially now after maflcko's change in fae3bf6b870eb0f9cddd1adac82ba72890806ae3 to not call `stop()` on test failure, I agree it makes sense to assert instead, good suggestion! Fixed.
> were there actually any cases where CannotSendRequest exceptions were caught and suppressed? If so what were those cases? Commit description should say what goals of this change are and if CannotSendRequest is an actual or theoretical change in behavior.
It was a common knock-on issue from attemp
...