π€ janb84 reviewed a pull request: "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3263298901)
ACK 9c13be9c45cad19d9db78e318b1e8376d56d6ec5
Thanks for squashing your commits!
My Guix build checksums:
**Commit:** `9c13be9c45ca`
```shell
22ad5809ed8d8399b0c7960e8c57cbaae5220b2667650262a7ec98a25eaf671f guix-build-9c13be9c45ca/output/aarch64-linux-gnu/SHA256SUMS.part
60b2048157462a5bd7ae00fa60ec8f1d59d84857d55125b0eafa98fad4aa780e guix-build-9c13be9c45ca/output/aarch64-linux-gnu/bitcoin-9c13be9c45ca-aarch64-linux-gnu-debug.tar.gz
7a7280f6b1221a62e9e8591365431f2cc3a0fe111abe
...
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3263298901)
ACK 9c13be9c45cad19d9db78e318b1e8376d56d6ec5
Thanks for squashing your commits!
My Guix build checksums:
**Commit:** `9c13be9c45ca`
```shell
22ad5809ed8d8399b0c7960e8c57cbaae5220b2667650262a7ec98a25eaf671f guix-build-9c13be9c45ca/output/aarch64-linux-gnu/SHA256SUMS.part
60b2048157462a5bd7ae00fa60ec8f1d59d84857d55125b0eafa98fad4aa780e guix-build-9c13be9c45ca/output/aarch64-linux-gnu/bitcoin-9c13be9c45ca-aarch64-linux-gnu-debug.tar.gz
7a7280f6b1221a62e9e8591365431f2cc3a0fe111abe
...
π¬ fanquake commented on pull request "Backport Cirrus runners to 28.x":
(https://github.com/bitcoin/bitcoin/pull/33406#issuecomment-3329042228)
cc @m3dwards
(https://github.com/bitcoin/bitcoin/pull/33406#issuecomment-3329042228)
cc @m3dwards
π¬ mstampfer commented on pull request "contrib: Add zsh completion scripts":
(https://github.com/bitcoin/bitcoin/pull/33402#issuecomment-3329042312)
commits squashed to a single commit for this branch
(https://github.com/bitcoin/bitcoin/pull/33402#issuecomment-3329042312)
commits squashed to a single commit for this branch
π¬ pablomartin4btc commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2376072486)
Following your suggestion @ryanofsky... If a new type is added (`NUM_OR_STR` or even `ARR_OR_STR` from #32468), is `also_string` still needed?
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2376072486)
Following your suggestion @ryanofsky... If a new type is added (`NUM_OR_STR` or even `ARR_OR_STR` from #32468), is `also_string` still needed?
π€ rkrux reviewed a pull request: "doc: update multisig tutorial to use multipath descriptors"
(https://github.com/bitcoin/bitcoin/pull/33286#pullrequestreview-3263305638)
crACK 3d42607fb5966d8e3b79fdb878d59483432625d9
(https://github.com/bitcoin/bitcoin/pull/33286#pullrequestreview-3263305638)
crACK 3d42607fb5966d8e3b79fdb878d59483432625d9
π¬ rkrux commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2376065785)
In 5424b4f1ce74c82b7ae01034bbc1592088048128 "doc: Update multisig-tutorial.md to use multipath descriptors"
Nit: By convention, the first index of the two corresponds to the receive address and the other being the change address as quoted in the above tutorial too: `doc/multisig-tutorial.md`.
```diff
- # Multipath descriptor expands to change and receive
+ # Multipath descriptor expands to receive and change
```
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2376065785)
In 5424b4f1ce74c82b7ae01034bbc1592088048128 "doc: Update multisig-tutorial.md to use multipath descriptors"
Nit: By convention, the first index of the two corresponds to the receive address and the other being the change address as quoted in the above tutorial too: `doc/multisig-tutorial.md`.
```diff
- # Multipath descriptor expands to change and receive
+ # Multipath descriptor expands to receive and change
```
π¬ rkrux commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2376077617)
In 5424b4f1ce74c82b7ae01034bbc1592088048128 "doc: Update multisig-tutorial.md to use multipath descriptors"
> with <code><0;1></code> as the change index.
I find it confusing to read it as the change index when one of the indices is non-change (receive) when expanded later. Maybe the below one?
```diff
- with <code><0;1></code> as the change index.
+ with <code><0;1></code> as the derivation index.
```
----------------------------------------
Nit: To keep it consistent with how
...
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2376077617)
In 5424b4f1ce74c82b7ae01034bbc1592088048128 "doc: Update multisig-tutorial.md to use multipath descriptors"
> with <code><0;1></code> as the change index.
I find it confusing to read it as the change index when one of the indices is non-change (receive) when expanded later. Maybe the below one?
```diff
- with <code><0;1></code> as the change index.
+ with <code><0;1></code> as the derivation index.
```
----------------------------------------
Nit: To keep it consistent with how
...
π¬ zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2376117984)
It makes sense to add JSON_OR_STRING and related parsing logic after this is merged.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2376117984)
It makes sense to add JSON_OR_STRING and related parsing logic after this is merged.
π¬ RandyMcMillan commented on pull request "Enhanced Traffic Graph Widget with Multi-timeframe Support and Data Persistence":
(https://github.com/bitcoin-core/gui/pull/866#issuecomment-3329161743)
concept ACK
(https://github.com/bitcoin-core/gui/pull/866#issuecomment-3329161743)
concept ACK
π€ pablomartin4btc reviewed a pull request: "rpc: addpeeraddress: throw on invalid IP"
(https://github.com/bitcoin/bitcoin/pull/33430#pullrequestreview-3263399644)
tACK 316a0c513278d53cb25f42ea502d20691962aad6
`master:`
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc addpeeraddress "" 8883
{
"success": false
}
```
This branch:
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc addpeeraddress "" 8883
error code: -30
error message:
Invalid IP address
```
I agree with @vasild with the [suggested](https://github.com/bitcoin/bitcoin/pull/33430#pullrequestreview-3257726472) alternative, not strong opinion.
Would it be useful for
...
(https://github.com/bitcoin/bitcoin/pull/33430#pullrequestreview-3263399644)
tACK 316a0c513278d53cb25f42ea502d20691962aad6
`master:`
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc addpeeraddress "" 8883
{
"success": false
}
```
This branch:
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc addpeeraddress "" 8883
error code: -30
error message:
Invalid IP address
```
I agree with @vasild with the [suggested](https://github.com/bitcoin/bitcoin/pull/33430#pullrequestreview-3257726472) alternative, not strong opinion.
Would it be useful for
...
π ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3263312993)
Code review ACK 6730abb137eed82073814bdb8dbd577e79520b59. Just some comments and indentation changed since last review. Thanks for considering all the suggestions
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3263312993)
Code review ACK 6730abb137eed82073814bdb8dbd577e79520b59. Just some comments and indentation changed since last review. Thanks for considering all the suggestions
π¬ ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2376071372)
In commit "rpc: Handle -named argument parsing where '=' character is used" (03068e67cd9a62aa9105ca6a4c971b6261a783c5)
Every place this function is called, it called with a `size_t` value cast to an `int`. It would seem nice to make it take an a `size_t` value directly, since it is a new function. (Could also change the `CRPCConvertParam` struct to use `size_t` instead of `int` but would suggest not doing that to keep the changes minimal.)
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2376071372)
In commit "rpc: Handle -named argument parsing where '=' character is used" (03068e67cd9a62aa9105ca6a4c971b6261a783c5)
Every place this function is called, it called with a `size_t` value cast to an `int`. It would seem nice to make it take an a `size_t` value directly, since it is a new function. (Could also change the `CRPCConvertParam` struct to use `size_t` instead of `int` but would suggest not doing that to keep the changes minimal.)
π¬ ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2376104099)
In commit "rpc: Handle -named argument parsing where '=' character is used" (03068e67cd9a62aa9105ca6a4c971b6261a783c5)
Maybe add a sentence like "See \ref RPCConvertNamedValues for more information on how named and positional arguments are distinguished with -named." since this comment is only describing what entries need to be added to the table, not exactly how the entries are used.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2376104099)
In commit "rpc: Handle -named argument parsing where '=' character is used" (03068e67cd9a62aa9105ca6a4c971b6261a783c5)
Maybe add a sentence like "See \ref RPCConvertNamedValues for more information on how named and positional arguments are distinguished with -named." since this comment is only describing what entries need to be added to the table, not exactly how the entries are used.
π¬ ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2376123734)
In commit "rpc: Handle -named argument parsing where '=' character is used" (03068e67cd9a62aa9105ca6a4c971b6261a783c5)
Could change "==label==" to "my=wallet" to be consistent with example below. The "my=wallet" example is probably clearer anyway since there is only one `=` and it's in the middle of the string, and wallet names are probably more familiar than labels.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2376123734)
In commit "rpc: Handle -named argument parsing where '=' character is used" (03068e67cd9a62aa9105ca6a4c971b6261a783c5)
Could change "==label==" to "my=wallet" to be consistent with example below. The "my=wallet" example is probably clearer anyway since there is only one `=` and it's in the middle of the string, and wallet names are probably more familiar than labels.
π¬ RandyMcMillan commented on pull request "Updated MacOS icon to more closely fit Apple's design standards":
(https://github.com/bitcoin-core/gui/pull/852#issuecomment-3329236575)
I like the "flat" icon in https://github.com/bitcoin-core/gui/pull/879
maybe cherry-pick the art from #879 and post some variations?
(https://github.com/bitcoin-core/gui/pull/852#issuecomment-3329236575)
I like the "flat" icon in https://github.com/bitcoin-core/gui/pull/879
maybe cherry-pick the art from #879 and post some variations?
π¬ ryanofsky commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2376156696)
re: https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2376072486
> Following your suggestion @ryanofsky... If a new type is added (`NUM_OR_STR` or even `ARR_OR_STR` from #32468), is `also_string` still needed?
Yes my suggestion above is just a way of improving this python test, which makes sure the client-side type table in `src/rpc/client.cpp` is consistent with type information in the server RPC code. The other file `src/rpc/util.cpp` is a server-side file while provides informat
...
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2376156696)
re: https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2376072486
> Following your suggestion @ryanofsky... If a new type is added (`NUM_OR_STR` or even `ARR_OR_STR` from #32468), is `also_string` still needed?
Yes my suggestion above is just a way of improving this python test, which makes sure the client-side type table in `src/rpc/client.cpp` is consistent with type information in the server RPC code. The other file `src/rpc/util.cpp` is a server-side file while provides informat
...
π€ furszy reviewed a pull request: "key: use static context for libsecp256k1 calls where applicable"
(https://github.com/bitcoin/bitcoin/pull/33399#pullrequestreview-3263479028)
ACK 1ff9e929489e625a603e8755b8efe849feda1f16
(https://github.com/bitcoin/bitcoin/pull/33399#pullrequestreview-3263479028)
ACK 1ff9e929489e625a603e8755b8efe849feda1f16
π¬ marcofleon commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3329445838)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
I think this warning makes sense to remove for v30 if thereβs not yet a clear timeline for deprecation. I donβt think the wording matters too much for now, and can be decided on later along with a deprecation plan and better documentation wrt this setting.
Could be good to set various arbitrary values on some monitoring nodes and see what the impacts are (potentially more stale blocks?). Ultimately, if users, miners included, want to set this to
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3329445838)
ACK 451ba9ada41f687c0e4bb34d5925374a68a8f8a3
I think this warning makes sense to remove for v30 if thereβs not yet a clear timeline for deprecation. I donβt think the wording matters too much for now, and can be decided on later along with a deprecation plan and better documentation wrt this setting.
Could be good to set various arbitrary values on some monitoring nodes and see what the impacts are (potentially more stale blocks?). Ultimately, if users, miners included, want to set this to
...
π ismaelsadeeq opened a pull request: "bugfix: miner: fix `addPackageTxs` unsigned integer overflow"
(https://github.com/bitcoin/bitcoin/pull/33475)
This PR fixes an unsigned integer overflow in the `addPackageTxs` method of the `BlockAssembler`.
The overflow is a rare edge case that might occur on master when a miner reserves 2000 WU and wants to create an block to be empty.
i.e, by starting with `-blockmaxweight=2000`, `-blockreservedweight=2000`, or just `blockmaxweight=2000`, and then calling the mining interface `createNewBlock` with `blockReservedWeight` set to `2000`.
Instead of bailing out after going through transactions eq
...
(https://github.com/bitcoin/bitcoin/pull/33475)
This PR fixes an unsigned integer overflow in the `addPackageTxs` method of the `BlockAssembler`.
The overflow is a rare edge case that might occur on master when a miner reserves 2000 WU and wants to create an block to be empty.
i.e, by starting with `-blockmaxweight=2000`, `-blockreservedweight=2000`, or just `blockmaxweight=2000`, and then calling the mining interface `createNewBlock` with `blockReservedWeight` set to `2000`.
Instead of bailing out after going through transactions eq
...
π¬ jesterhodl commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3329617679)
> > > Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
> >
> >
> > Friendly ping to coordinators for addressing issues:
> >
> > * @sr-gi -- Catalan (ca)
> > * @ostruvek -- Czech (cs)
> > * @pryds -- Danish (da)
> > * @laanwj @sipa -- Dutch (nl)
> > * @Emzy -- German (de)
> > * @cryptomeow -- Greek (el)
> > * @jesterhodl -- Polish (pl)
> >
> > UPD. French (fr) and Spanish (es) coordinators have been notified vi
...
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3329617679)
> > > Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
> >
> >
> > Friendly ping to coordinators for addressing issues:
> >
> > * @sr-gi -- Catalan (ca)
> > * @ostruvek -- Czech (cs)
> > * @pryds -- Danish (da)
> > * @laanwj @sipa -- Dutch (nl)
> > * @Emzy -- German (de)
> > * @cryptomeow -- Greek (el)
> > * @jesterhodl -- Polish (pl)
> >
> > UPD. French (fr) and Spanish (es) coordinators have been notified vi
...