🚀 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
...
💬 maflcko commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2120966114)
> type-safe
fixed in https://github.com/bitcoin/bitcoin/pull/32660
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2120966114)
> type-safe
fixed in https://github.com/bitcoin/bitcoin/pull/32660
💬 fanquake commented on pull request "cmake: Replace deprecated `qt6_add_translation` with `qt6_add_lrelease`":
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2930387771)
Guix Build:
```bash
6ec55ec67ac33aeb89eae88bd55525e36e05d3c768eb98cf9c718cc8221de76e guix-build-18cf727429e9/output/aarch64-linux-gnu/SHA256SUMS.part
f3e260544a199b5b06f3f5de4352fcff61f6a3e36fd12a827317c2ba168ab3a8 guix-build-18cf727429e9/output/aarch64-linux-gnu/bitcoin-18cf727429e9-aarch64-linux-gnu-debug.tar.gz
259b8bd0260adc1517c11a8579983210501f6625083b780f3451c5bf6d531c52 guix-build-18cf727429e9/output/aarch64-linux-gnu/bitcoin-18cf727429e9-aarch64-linux-gnu.tar.gz
6772df343eb3f628
...
(https://github.com/bitcoin/bitcoin/pull/32651#issuecomment-2930387771)
Guix Build:
```bash
6ec55ec67ac33aeb89eae88bd55525e36e05d3c768eb98cf9c718cc8221de76e guix-build-18cf727429e9/output/aarch64-linux-gnu/SHA256SUMS.part
f3e260544a199b5b06f3f5de4352fcff61f6a3e36fd12a827317c2ba168ab3a8 guix-build-18cf727429e9/output/aarch64-linux-gnu/bitcoin-18cf727429e9-aarch64-linux-gnu-debug.tar.gz
259b8bd0260adc1517c11a8579983210501f6625083b780f3451c5bf6d531c52 guix-build-18cf727429e9/output/aarch64-linux-gnu/bitcoin-18cf727429e9-aarch64-linux-gnu.tar.gz
6772df343eb3f628
...
💬 hebasto commented on pull request "depends: sqlite 3.50.0; switch to autosetup":
(https://github.com/bitcoin/bitcoin/pull/32655#discussion_r2120976295)
How does this affect the overall dependency graph?
(https://github.com/bitcoin/bitcoin/pull/32655#discussion_r2120976295)
How does this affect the overall dependency graph?
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120826619)
(Incorporated most of this, breaking out commits, expanding function prototypes into multiple lines, even removing the `HeadersGeneratorSetup` type in favor of free functions).
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120826619)
(Incorporated most of this, breaking out commits, expanding function prototypes into multiple lines, even removing the `HeadersGeneratorSetup` type in favor of free functions).
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120864823)
Yeah, seems src/ipc/ really went all in on `-`. Holding off on changing for now to see what others think.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120864823)
Yeah, seems src/ipc/ really went all in on `-`. Holding off on changing for now to see what others think.
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120817742)
Ah, the Sonarcloud recommendation was actually for the `uint64_t`-ctor, similar to my response here: https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115831187.
Tested out making it explicit, modifying 13 additional files. It did uncover some implicit casts that went from `int64_t Params::nPowTargetTimespan` to `arith_uint256`... But the rest felt a bit forced. I think it is intentional and makes sense for u64s to implicitly cast to u256.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120817742)
Ah, the Sonarcloud recommendation was actually for the `uint64_t`-ctor, similar to my response here: https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2115831187.
Tested out making it explicit, modifying 13 additional files. It did uncover some implicit casts that went from `int64_t Params::nPowTargetTimespan` to `arith_uint256`... But the rest felt a bit forced. I think it is intentional and makes sense for u64s to implicitly cast to u256.
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120844393)
Using `const vector&` communicates that we are sending in the entire chain, not some subset. I like the specificity. `const vector`s feel more neutered than spans IMO.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120844393)
Using `const vector&` communicates that we are sending in the entire chain, not some subset. I like the specificity. `const vector`s feel more neutered than spans IMO.
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120833106)
Is there anything to catch wrong order?
```C++
const auto [pow_validated_headers, success, request_more] = ...
```
vs
```C++
const auto [pow_validated_headers, request_more, success] = ...
```
(There's clang-tidy logic to catch `/*paramname=*/true` mismatches for function calls).
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2120833106)
Is there anything to catch wrong order?
```C++
const auto [pow_validated_headers, success, request_more] = ...
```
vs
```C++
const auto [pow_validated_headers, request_more, success] = ...
```
(There's clang-tidy logic to catch `/*paramname=*/true` mismatches for function calls).
⚠️ ismaelsadeeq opened an issue: "wallet, node: Redundant opt-in RBF wallet config and RPC parameters"
(https://github.com/bitcoin/bitcoin/issues/32661)
Following the removal of the `-mempoolfullrbf` node option in [#30592](https://github.com/bitcoin/bitcoin/pull/30592), the Bitcoin Core wallet still retains the `-walletrbf` option.
This option determines whether transactions are marked as opt-in RBF (i.e., using sequence numbers `MAX - 1` or `MAX - 2`).
With full-RBF now the default behavior, `-walletrbf` is effectively redundant and should be removed.
However, removing `-walletrbf` alone is not sufficient, as several wallet RPCs also suppor
...
(https://github.com/bitcoin/bitcoin/issues/32661)
Following the removal of the `-mempoolfullrbf` node option in [#30592](https://github.com/bitcoin/bitcoin/pull/30592), the Bitcoin Core wallet still retains the `-walletrbf` option.
This option determines whether transactions are marked as opt-in RBF (i.e., using sequence numbers `MAX - 1` or `MAX - 2`).
With full-RBF now the default behavior, `-walletrbf` is effectively redundant and should be removed.
However, removing `-walletrbf` alone is not sufficient, as several wallet RPCs also suppor
...
🤔 dergoegge reviewed a pull request: "p2p: Add mutation check inside compact block's FillBlock"
(https://github.com/bitcoin/bitcoin/pull/32646#pullrequestreview-2888127199)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32646#pullrequestreview-2888127199)
Concept ACK
💬 dergoegge commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2120969666)
nit
```suggestion
const CBlockIndex* prev_block{Assume(m_chainman.m_blockman.LookupBlockIndex(partialBlock.header.hashPrevBlock))};
```
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2120969666)
nit
```suggestion
const CBlockIndex* prev_block{Assume(m_chainman.m_blockman.LookupBlockIndex(partialBlock.header.hashPrevBlock))};
```
💬 mabu44 commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2121017491)
The third parameter should be 0, not false. The same apply for all occurrences of the AddCoins function.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2121017491)
The third parameter should be 0, not false. The same apply for all occurrences of the AddCoins function.
📝 hebasto opened a pull request: "doc: Remove build instruction for running `clang-tidy`"
(https://github.com/bitcoin/bitcoin/pull/32662)
One of the benefits of using a compilation database, which is available after the CMake build system generation step, is that it is not necessary to actually build the code in order to run `clang-tidy`.
(https://github.com/bitcoin/bitcoin/pull/32662)
One of the benefits of using a compilation database, which is available after the CMake build system generation step, is that it is not necessary to actually build the code in order to run `clang-tidy`.
💬 ismaelsadeeq commented on pull request "init: deprecate `-blockmaxweight` startup option":
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2930558750)
> (TBH, I think these options should move into the template request, rather than being node startup options)
Yes, the `BlockCreateOptions` have `block_reserved_weight` variable. From recent @Sjors comment I think there's no need for the `blockmaxweight` option for TP that's why it was not added.
It makes sense to add these options to the template request RPC parameter as well, since test networks and other protocols still rely on the RPC feels natural for this to be runtime option.
My
...
(https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2930558750)
> (TBH, I think these options should move into the template request, rather than being node startup options)
Yes, the `BlockCreateOptions` have `block_reserved_weight` variable. From recent @Sjors comment I think there's no need for the `blockmaxweight` option for TP that's why it was not added.
It makes sense to add these options to the template request RPC parameter as well, since test networks and other protocols still rely on the RPC feels natural for this to be runtime option.
My
...