💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2929886599)
> While touching this code, it seems reasonable to also address the other @davidgumberg's [comment](https://github.com/bitcoin/bitcoin/pull/32634#issuecomment-2918081325):
Added
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2929886599)
> While touching this code, it seems reasonable to also address the other @davidgumberg's [comment](https://github.com/bitcoin/bitcoin/pull/32634#issuecomment-2918081325):
Added
💬 fanquake commented on pull request "depends: don't install & then delete sqlite pkgconf":
(https://github.com/bitcoin/bitcoin/pull/32656#issuecomment-2929895945)
Guix Build:
```bash
545db2ab3bf28143eda2307089bb7266ae64802ea4a15371bacccf4574ee2c67 guix-build-72a5aa9b791e/output/aarch64-linux-gnu/SHA256SUMS.part
660030fb8c70d09e11f35e2ccdde4187f6bd90009bc1da2826be1cf8a73e6f4b guix-build-72a5aa9b791e/output/aarch64-linux-gnu/bitcoin-72a5aa9b791e-aarch64-linux-gnu-debug.tar.gz
e4a702b1d4f589cd9a1d6a0534eee7ddab725a9aa753b5a276510ee9b956ed94 guix-build-72a5aa9b791e/output/aarch64-linux-gnu/bitcoin-72a5aa9b791e-aarch64-linux-gnu.tar.gz
932b1525b03ed1ad
...
(https://github.com/bitcoin/bitcoin/pull/32656#issuecomment-2929895945)
Guix Build:
```bash
545db2ab3bf28143eda2307089bb7266ae64802ea4a15371bacccf4574ee2c67 guix-build-72a5aa9b791e/output/aarch64-linux-gnu/SHA256SUMS.part
660030fb8c70d09e11f35e2ccdde4187f6bd90009bc1da2826be1cf8a73e6f4b guix-build-72a5aa9b791e/output/aarch64-linux-gnu/bitcoin-72a5aa9b791e-aarch64-linux-gnu-debug.tar.gz
e4a702b1d4f589cd9a1d6a0534eee7ddab725a9aa753b5a276510ee9b956ed94 guix-build-72a5aa9b791e/output/aarch64-linux-gnu/bitcoin-72a5aa9b791e-aarch64-linux-gnu.tar.gz
932b1525b03ed1ad
...
💬 hebasto commented on issue "cmake: Replace f`ile(GLOB ...)` command with an explicit list of `*.ts` files":
(https://github.com/bitcoin/bitcoin/issues/32653#issuecomment-2929957366)
> What's the actual blocker to just making this change?
There are no blockers.
However, [`bitcoin-maintainer-tools/update-translations.py`](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/update-translations.py) should be updated first.
(https://github.com/bitcoin/bitcoin/issues/32653#issuecomment-2929957366)
> What's the actual blocker to just making this change?
There are no blockers.
However, [`bitcoin-maintainer-tools/update-translations.py`](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/update-translations.py) should be updated first.
💬 mabu44 commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2120739704)
Moving these lines directly before the check on sigops (e.g. to line 228) can improve performances because we avoid performing the calculation when a "return false" is executed for another reason in the block between lines 206 and 227.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2120739704)
Moving these lines directly before the check on sigops (e.g. to line 228) can improve performances because we avoid performing the calculation when a "return false" is executed for another reason in the block between lines 206 and 227.
💬 mabu44 commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2120745163)
Moving these lines directly before the check on sigops (e.g. to line 228) can improve performances because we avoid performing the calculation when a "return false" is executed for another reason in the block between lines 206 and 227.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2120745163)
Moving these lines directly before the check on sigops (e.g. to line 228) can improve performances because we avoid performing the calculation when a "return false" is executed for another reason in the block between lines 206 and 227.
💬 hebasto commented on issue "windows: depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2929989264)
> Oh, no. I'm so sorry. I modify my ci.yml as this:
What CI are you using? Is the `VCPKG_INSTALLATION_ROOT` environment variable defined there?
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2929989264)
> Oh, no. I'm so sorry. I modify my ci.yml as this:
What CI are you using? Is the `VCPKG_INSTALLATION_ROOT` environment variable defined there?
👍 stickies-v approved a pull request: "depends: don't install & then delete sqlite pkgconf"
(https://github.com/bitcoin/bitcoin/pull/32656#pullrequestreview-2887874268)
ACK 72a5aa9b791e80a6204990c27c8165ce8aa81dd7
https://github.com/bitcoin/bitcoin/pull/31276/commits/319a4e82614283afb3dbc5d38ff3b9d17fb911b3 already removed the pkgconfig data (and as per my understanding, could have just removed the `install-pkgconfigDATA` target as an equivalent approach). Since the postprocess_cmds are executed right after the stage_cmds, my understanding is that nothing was done with the pkgconfig data, so simply not installing it makes sense.
Tested with depends build
...
(https://github.com/bitcoin/bitcoin/pull/32656#pullrequestreview-2887874268)
ACK 72a5aa9b791e80a6204990c27c8165ce8aa81dd7
https://github.com/bitcoin/bitcoin/pull/31276/commits/319a4e82614283afb3dbc5d38ff3b9d17fb911b3 already removed the pkgconfig data (and as per my understanding, could have just removed the `install-pkgconfigDATA` target as an equivalent approach). Since the postprocess_cmds are executed right after the stage_cmds, my understanding is that nothing was done with the pkgconfig data, so simply not installing it makes sense.
Tested with depends build
...
💬 laanwj commented on pull request "cmake: Replace deprecated `qt6_add_translation` with `qt6_add_lrelease`":
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2930085376)
From the documentation i understand that it's not `qt_add_translations` itself that was deprecated, but the specific syntax of using it. Starting with Qt 6.7, it uses a different format, but it won't be removed? It's slightly ambigious to me, tbh.
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2930085376)
From the documentation i understand that it's not `qt_add_translations` itself that was deprecated, but the specific syntax of using it. Starting with Qt 6.7, it uses a different format, but it won't be removed? It's slightly ambigious to me, tbh.
💬 hebasto commented on pull request "cmake: Replace deprecated `qt6_add_translation` with `qt6_add_lrelease`":
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2930095789)
> From the documentation i understand that it's not `qt_add_translations` itself that was deprecated, but the specific syntax of using it. Starting with Qt 6.7, it uses a different format, but it won't be removed? It's slightly ambigious to me, tbh.
`qt_add_translation` (singular) is deprecated. `qt_add_translations` (plural) is a new Qt 6 command that combines `lupdate` and `lrelease` invocations.
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2930095789)
> From the documentation i understand that it's not `qt_add_translations` itself that was deprecated, but the specific syntax of using it. Starting with Qt 6.7, it uses a different format, but it won't be removed? It's slightly ambigious to me, tbh.
`qt_add_translation` (singular) is deprecated. `qt_add_translations` (plural) is a new Qt 6 command that combines `lupdate` and `lrelease` invocations.
💬 laanwj commented on issue "cmake: Replace f`ile(GLOB ...)` command with an explicit list of `*.ts` files":
(https://github.com/bitcoin/bitcoin/issues/32653#issuecomment-2930104903)
Concept ACK. Using a glob is kind of fragile here, harder to ensure the build matches git when there's loose `.ts` files around.
(https://github.com/bitcoin/bitcoin/issues/32653#issuecomment-2930104903)
Concept ACK. Using a glob is kind of fragile here, harder to ensure the build matches git when there's loose `.ts` files around.
💬 laanwj commented on pull request "cmake: Replace deprecated `qt6_add_translation` with `qt6_add_lrelease`":
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2930112960)
> qt_add_translation (singular) is deprecated. qt_add_translations (plural) is a new Qt 6 command that combines the lupdate and lrelease invocations.
Right, and we only need `lrealease` here. Okay, concept ACK.
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2930112960)
> qt_add_translation (singular) is deprecated. qt_add_translations (plural) is a new Qt 6 command that combines the lupdate and lrelease invocations.
Right, and we only need `lrealease` here. Okay, concept ACK.
💬 hebasto commented on issue "cmake: Replace f`ile(GLOB ...)` command with an explicit list of `*.ts` files":
(https://github.com/bitcoin/bitcoin/issues/32653#issuecomment-2930114468)
> Using a glob is kind of fragile here, harder to ensure the build matches git when there's loose `.ts` files around.
True. Please see https://github.com/bitcoin/bitcoin/issues/32653.
(https://github.com/bitcoin/bitcoin/issues/32653#issuecomment-2930114468)
> Using a glob is kind of fragile here, harder to ensure the build matches git when there's loose `.ts` files around.
True. Please see https://github.com/bitcoin/bitcoin/issues/32653.
🤔 hebasto reviewed a pull request: "depends: don't install & then delete sqlite pkgconf"
(https://github.com/bitcoin/bitcoin/pull/32656#pullrequestreview-2887975315)
My Guix build:
```
aarch64
545db2ab3bf28143eda2307089bb7266ae64802ea4a15371bacccf4574ee2c67 guix-build-72a5aa9b791e/output/aarch64-linux-gnu/SHA256SUMS.part
660030fb8c70d09e11f35e2ccdde4187f6bd90009bc1da2826be1cf8a73e6f4b guix-build-72a5aa9b791e/output/aarch64-linux-gnu/bitcoin-72a5aa9b791e-aarch64-linux-gnu-debug.tar.gz
e4a702b1d4f589cd9a1d6a0534eee7ddab725a9aa753b5a276510ee9b956ed94 guix-build-72a5aa9b791e/output/aarch64-linux-gnu/bitcoin-72a5aa9b791e-aarch64-linux-gnu.tar.gz
932b1525
...
(https://github.com/bitcoin/bitcoin/pull/32656#pullrequestreview-2887975315)
My Guix build:
```
aarch64
545db2ab3bf28143eda2307089bb7266ae64802ea4a15371bacccf4574ee2c67 guix-build-72a5aa9b791e/output/aarch64-linux-gnu/SHA256SUMS.part
660030fb8c70d09e11f35e2ccdde4187f6bd90009bc1da2826be1cf8a73e6f4b guix-build-72a5aa9b791e/output/aarch64-linux-gnu/bitcoin-72a5aa9b791e-aarch64-linux-gnu-debug.tar.gz
e4a702b1d4f589cd9a1d6a0534eee7ddab725a9aa753b5a276510ee9b956ed94 guix-build-72a5aa9b791e/output/aarch64-linux-gnu/bitcoin-72a5aa9b791e-aarch64-linux-gnu.tar.gz
932b1525
...
👍 hebasto approved a pull request: "depends: don't install & then delete sqlite pkgconf"
(https://github.com/bitcoin/bitcoin/pull/32656#pullrequestreview-2887977405)
ACK 72a5aa9b791e80a6204990c27c8165ce8aa81dd7.
(https://github.com/bitcoin/bitcoin/pull/32656#pullrequestreview-2887977405)
ACK 72a5aa9b791e80a6204990c27c8165ce8aa81dd7.
🚀 hebasto merged a pull request: "depends: don't install & then delete sqlite pkgconf"
(https://github.com/bitcoin/bitcoin/pull/32656)
(https://github.com/bitcoin/bitcoin/pull/32656)
💬 hebasto commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2930192028)
FWIW, https://codeberg.org/guix/guix/pulls/353 has been landed recently.
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2930192028)
FWIW, https://codeberg.org/guix/guix/pulls/353 has been landed recently.
⚠️ Sjors opened an issue: "wallet: render BIP388 wallet policies"
(https://github.com/bitcoin/bitcoin/issues/32659)
[BIP 388](https://github.com/bitcoin/bips/blob/master/bip-0388.mediawiki) wallet policies were introduced by @bigspider to make it easier to verify descriptors on a signing device.
A first step towards supporting them could be to add a `policy` and `keys` field to the `getdescriptorinfo` and `listdescriptors` RPC results. Only for the subset of descriptors that BIP388 allows.
These fields can be manually passed into HWI using this (somewhat old) PR https://github.com/bitcoin-core/HWI/pull/647
...
(https://github.com/bitcoin/bitcoin/issues/32659)
[BIP 388](https://github.com/bitcoin/bips/blob/master/bip-0388.mediawiki) wallet policies were introduced by @bigspider to make it easier to verify descriptors on a signing device.
A first step towards supporting them could be to add a `policy` and `keys` field to the `getdescriptorinfo` and `listdescriptors` RPC results. Only for the subset of descriptors that BIP388 allows.
These fields can be manually passed into HWI using this (somewhat old) PR https://github.com/bitcoin-core/HWI/pull/647
...
💬 hebasto commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2930369097)
> @ryanofsky CI complains about `std::move` in `libmultiprocess`: https://cirrus-ci.com/task/6187773452877824, e.g.:
>
> ```
> /ci_container_base/src/ipc/libmultiprocess/include/mp/proxy-io.h:252:16: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
> [05:29:23.932] 252 | m_fn = std::move(fn);
> [05:29:23.932] | ^~~~~~~~~
> [
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2930369097)
> @ryanofsky CI complains about `std::move` in `libmultiprocess`: https://cirrus-ci.com/task/6187773452877824, e.g.:
>
> ```
> /ci_container_base/src/ipc/libmultiprocess/include/mp/proxy-io.h:252:16: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
> [05:29:23.932] 252 | m_fn = std::move(fn);
> [05:29:23.932] | ^~~~~~~~~
> [
...
📝 maflcko opened a pull request: "rpc: Use type-safe HelpException"
(https://github.com/bitcoin/bitcoin/pull/32660)
The current "catch-all" `catch (const std::exception& e)` in `CRPCTable::help` is problematic, because it could catch exceptions unrelated to passing the help string up.
Fix this by using a dedicated exception type.
(https://github.com/bitcoin/bitcoin/pull/32660)
The current "catch-all" `catch (const std::exception& e)` in `CRPCTable::help` is problematic, because it could catch exceptions unrelated to passing the help string up.
Fix this by using a dedicated exception type.
💬 maflcko commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2930384842)
Can be tested by adding a bug, like
```diff
diff --git a/src/rpc/signmessage.cpp b/src/rpc/signmessage.cpp
index 5597f8d237..d62893bf0c 100644
--- a/src/rpc/signmessage.cpp
+++ b/src/rpc/signmessage.cpp
@@ -19,7 +19,7 @@ static RPCHelpMan verifymessage()
return RPCHelpMan{"verifymessage",
"Verify a signed message.",
{
- {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address to use for the signature."},
+ {"addres\ns", R
...
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2930384842)
Can be tested by adding a bug, like
```diff
diff --git a/src/rpc/signmessage.cpp b/src/rpc/signmessage.cpp
index 5597f8d237..d62893bf0c 100644
--- a/src/rpc/signmessage.cpp
+++ b/src/rpc/signmessage.cpp
@@ -19,7 +19,7 @@ static RPCHelpMan verifymessage()
return RPCHelpMan{"verifymessage",
"Verify a signed message.",
{
- {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address to use for the signature."},
+ {"addres\ns", R
...