💬 maflcko commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2572824301)
Not sure about the first commit. There is no issue or risk with the existing code that is fixed by the style fixups, so to me it seems better to not change it, unless there is a compelling reason. Generally when it comes to style modernizations, my recommendation would be to include them in changes that touch the line anyway, or leave them as-is, or do them wholesale for the whole project (only if there is a good reason for them). But going out and doing them one-by one for specific subsections
...
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2572824301)
Not sure about the first commit. There is no issue or risk with the existing code that is fixed by the style fixups, so to me it seems better to not change it, unless there is a compelling reason. Generally when it comes to style modernizations, my recommendation would be to include them in changes that touch the line anyway, or leave them as-is, or do them wholesale for the whole project (only if there is a good reason for them). But going out and doing them one-by one for specific subsections
...
💬 maflcko commented on pull request "#31403 follow-up":
(https://github.com/bitcoin/bitcoin/pull/31599#issuecomment-2572826673)
Also, a rebase would be good
(https://github.com/bitcoin/bitcoin/pull/31599#issuecomment-2572826673)
Also, a rebase would be good
💬 fanquake commented on pull request "ci: Bump centos stream 10":
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2572832061)
https://cirrus-ci.com/task/4631012182851584
```bash
[10:35:39.510] + bash -c 'dnf -y --allowerasing install gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache make git python3 python3-pip which patch lbzip2 xz procps-ng dash rsync coreutils bison e2fsprogs cmake '
[10:35:40.618] Extra Packages for Enterprise Linux 10 - x86_64 3.7 MB/s | 3.1 MB 00:00
[10:35:41.599] Last metadata expiration check: 0:00:01 ago on Mon Jan 6 05:35:40 2025.
[10
...
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2572832061)
https://cirrus-ci.com/task/4631012182851584
```bash
[10:35:39.510] + bash -c 'dnf -y --allowerasing install gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache make git python3 python3-pip which patch lbzip2 xz procps-ng dash rsync coreutils bison e2fsprogs cmake '
[10:35:40.618] Extra Packages for Enterprise Linux 10 - x86_64 3.7 MB/s | 3.1 MB 00:00
[10:35:41.599] Last metadata expiration check: 0:00:01 ago on Mon Jan 6 05:35:40 2025.
[10
...
🚀 fanquake merged a pull request: "doc: Update NetBSD Build Guide"
(https://github.com/bitcoin/bitcoin/pull/31586)
(https://github.com/bitcoin/bitcoin/pull/31586)
📝 brunoerg opened a pull request: "test: importdescriptors is not available for non-descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/31609)
This PR adds test coverage for the error thrown when trying to import descriptors into a non-descriptor wallet.
(https://github.com/bitcoin/bitcoin/pull/31609)
This PR adds test coverage for the error thrown when trying to import descriptors into a non-descriptor wallet.
👍 hebasto approved a pull request: "ci: Allow build dir on CI host"
(https://github.com/bitcoin/bitcoin/pull/31428#pullrequestreview-2531869230)
re-ACK 8888ee4403fa62972c49e18752695d15fd32c0b0.
Only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/31428#pullrequestreview-2517422562). Verified with:
```
git range-diff master fac0ab3286888c2eab674f6814ba8006e3a850a8 8888ee4403fa62972c49e18752695d15fd32c0b0
```
(https://github.com/bitcoin/bitcoin/pull/31428#pullrequestreview-2531869230)
re-ACK 8888ee4403fa62972c49e18752695d15fd32c0b0.
Only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/31428#pullrequestreview-2517422562). Verified with:
```
git range-diff master fac0ab3286888c2eab674f6814ba8006e3a850a8 8888ee4403fa62972c49e18752695d15fd32c0b0
```
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2572844315)
Rebased on top of the https://github.com/bitcoin/bitcoin/pull/31428.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2572844315)
Rebased on top of the https://github.com/bitcoin/bitcoin/pull/31428.
💬 maflcko commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2572872762)
Also, for the second commit, it may be helpful to add the commit since the change is possible, like I did in f23d6a572d62ff640a28718ff0771c3f0f87f365, where I presume it is cherry-picked from?
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2572872762)
Also, for the second commit, it may be helpful to add the commit since the change is possible, like I did in f23d6a572d62ff640a28718ff0771c3f0f87f365, where I presume it is cherry-picked from?
💬 maflcko commented on pull request "ci: Bump centos stream 10":
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2572880648)
Yeah, it is blocked on https://bugzilla.redhat.com/show_bug.cgi?id=2335616.
Also, I am thinking it is fine to drop the 32-bit build, given that it is mostly redundant with ci/test/00_setup_env_i686_multiprocess.sh anyway.
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2572880648)
Yeah, it is blocked on https://bugzilla.redhat.com/show_bug.cgi?id=2335616.
Also, I am thinking it is fine to drop the 32-bit build, given that it is mostly redundant with ci/test/00_setup_env_i686_multiprocess.sh anyway.
📝 maflcko converted_to_draft a pull request: "ci: Bump centos stream 10"
(https://github.com/bitcoin/bitcoin/pull/31593)
This is a follow-up to fa47baa03bcfcf44fb2ed05f009a32d32f860c45, which bumped the gcc version to avoid a warning bloat in the CI log. However, it is also required to bump python3, see https://github.com/bitcoin/bitcoin/issues/31476#issue-2735206340
> This will uncover an issue in the centos task that the correct python version is missing. I guess this should be fixed by installing and activating an acceptable python version.
Instead of bumping the packages individually in centos stream 9,
...
(https://github.com/bitcoin/bitcoin/pull/31593)
This is a follow-up to fa47baa03bcfcf44fb2ed05f009a32d32f860c45, which bumped the gcc version to avoid a warning bloat in the CI log. However, it is also required to bump python3, see https://github.com/bitcoin/bitcoin/issues/31476#issue-2735206340
> This will uncover an issue in the centos task that the correct python version is missing. I guess this should be fixed by installing and activating an acceptable python version.
Instead of bumping the packages individually in centos stream 9,
...
🚀 fanquake merged a pull request: "txmempool: fix typos in comments"
(https://github.com/bitcoin/bitcoin/pull/31584)
(https://github.com/bitcoin/bitcoin/pull/31584)
💬 hebasto commented on pull request "doc: Clarify min macOS and Xcode version":
(https://github.com/bitcoin/bitcoin/pull/31608#discussion_r1904026312)
nit: While touching this line, we can use "+" or "and newer" consistently without mixing both.
(https://github.com/bitcoin/bitcoin/pull/31608#discussion_r1904026312)
nit: While touching this line, we can use "+" or "and newer" consistently without mixing both.
👍 hebasto approved a pull request: "doc: Clarify min macOS and Xcode version"
(https://github.com/bitcoin/bitcoin/pull/31608#pullrequestreview-2531924374)
ACK fa728e4f184ebbbcab07047cd8790fb9b16d5681.
(https://github.com/bitcoin/bitcoin/pull/31608#pullrequestreview-2531924374)
ACK fa728e4f184ebbbcab07047cd8790fb9b16d5681.
🤔 fanquake reviewed a pull request: "doc: Clarify min macOS and Xcode version"
(https://github.com/bitcoin/bitcoin/pull/31608#pullrequestreview-2531932011)
> only the latest security release is supported.
Should this change `OSX_MIN_VERSION`? Otherwise we are just changing the docs to no-longer reflect the compatibility of the binary we ship (it will still run on any 13.x).
(https://github.com/bitcoin/bitcoin/pull/31608#pullrequestreview-2531932011)
> only the latest security release is supported.
Should this change `OSX_MIN_VERSION`? Otherwise we are just changing the docs to no-longer reflect the compatibility of the binary we ship (it will still run on any 13.x).
💬 fanquake commented on pull request "doc: Clarify min macOS and Xcode version":
(https://github.com/bitcoin/bitcoin/pull/31608#discussion_r1904030950)
typo: `providing the at least the`
(https://github.com/bitcoin/bitcoin/pull/31608#discussion_r1904030950)
typo: `providing the at least the`
💬 maflcko commented on pull request "doc: Clarify min macOS and Xcode version":
(https://github.com/bitcoin/bitcoin/pull/31608#issuecomment-2572916602)
> > only the latest security release is supported.
>
> Should this change `OSX_MIN_VERSION`? Otherwise we are just changing the docs to no-longer reflect the compatibility of the binary we ship (it will still run on any 13.x).
While I think that only the latest minor security release is supported, it may be a bit too much handholding and churn to bump `OSX_MIN_VERSION` on every new macOS minor release. In reality it will take time to get into a release and until users upgrade, so I wonder
...
(https://github.com/bitcoin/bitcoin/pull/31608#issuecomment-2572916602)
> > only the latest security release is supported.
>
> Should this change `OSX_MIN_VERSION`? Otherwise we are just changing the docs to no-longer reflect the compatibility of the binary we ship (it will still run on any 13.x).
While I think that only the latest minor security release is supported, it may be a bit too much handholding and churn to bump `OSX_MIN_VERSION` on every new macOS minor release. In reality it will take time to get into a release and until users upgrade, so I wonder
...
🚀 fanquake merged a pull request: "depends: Update capnproto to 1.1.0"
(https://github.com/bitcoin/bitcoin/pull/31552)
(https://github.com/bitcoin/bitcoin/pull/31552)
💬 hebasto commented on pull request "qa: Ensure consistent use of decimals instead of floats":
(https://github.com/bitcoin/bitcoin/pull/31595#issuecomment-2572924040)
> Why? While we are currently relying on the `short` python `repr()` algorithm for floats, this has been stable and reliable since it's been introduced in python 3.1 as far as I know - so I don't really see the benefit of changing dozens of functional tests / adding a new linter - unless there have been other bugs related to this in the past?
Consider two lines from [`test/functional/rpc_psbt.py`](https://github.com/bitcoin/bitcoin/blob/master/test/functional/rpc_psbt.py):https://github.com/b
...
(https://github.com/bitcoin/bitcoin/pull/31595#issuecomment-2572924040)
> Why? While we are currently relying on the `short` python `repr()` algorithm for floats, this has been stable and reliable since it's been introduced in python 3.1 as far as I know - so I don't really see the benefit of changing dozens of functional tests / adding a new linter - unless there have been other bugs related to this in the past?
Consider two lines from [`test/functional/rpc_psbt.py`](https://github.com/bitcoin/bitcoin/blob/master/test/functional/rpc_psbt.py):https://github.com/b
...
🚀 fanquake merged a pull request: "doc: Install `net/py-pyzmq` port on FreeBSD for `interface_zmq.py`"
(https://github.com/bitcoin/bitcoin/pull/31526)
(https://github.com/bitcoin/bitcoin/pull/31526)
📝 fanquake converted_to_draft a pull request: "rpc, cli: add getbalances#total, and use it for -getinfo"
(https://github.com/bitcoin/bitcoin/pull/31353)
Add a "total" field in RPC getbalances to be able to easily see the total amount held in the wallet, and use that field for the wallet balances in CLI -getinfo.
Currently -getinfo only returns getbalances#mine.trusted for wallet balances. It would make sense to instead return the total balance. For instance, to see:
- watchonly balances
- reused outputs, like coins returned to a wallet from an exchange or third party service that uses a fixed (reused) address, whether `avoid_reuse` is se
...
(https://github.com/bitcoin/bitcoin/pull/31353)
Add a "total" field in RPC getbalances to be able to easily see the total amount held in the wallet, and use that field for the wallet balances in CLI -getinfo.
Currently -getinfo only returns getbalances#mine.trusted for wallet balances. It would make sense to instead return the total balance. For instance, to see:
- watchonly balances
- reused outputs, like coins returned to a wallet from an exchange or third party service that uses a fixed (reused) address, whether `avoid_reuse` is se
...