💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390344607)
## 💡 Improvement Needed
**Line:** 2
The version number in the manual page should be updated to reflect the current release. The current entry shows 'v28.3.0rc1', which indicates a release candidate. For a stable release, this should be updated to the final version number.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390344607)
## 💡 Improvement Needed
**Line:** 2
The version number in the manual page should be updated to reflect the current release. The current entry shows 'v28.3.0rc1', which indicates a release candidate. For a stable release, this should be updated to the final version number.
💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390344497)
## 💡 Improvement Needed
**Line:** 12
The version number in the DESCRIPTION section should be updated to `v28.3.0rc1` to accurately reflect the current release of `bitcoin-tx`.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390344497)
## 💡 Improvement Needed
**Line:** 12
The version number in the DESCRIPTION section should be updated to `v28.3.0rc1` to accurately reflect the current release of `bitcoin-tx`.
💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390343799)
## 💡 Improvement Needed
**Line:** 5
The version number `_CLIENT_VERSION_RC` is hardcoded. For better maintainability and to avoid manual updates across different configuration files, consider using a more dynamic versioning strategy or a dedicated version management tool.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390343799)
## 💡 Improvement Needed
**Line:** 5
The version number `_CLIENT_VERSION_RC` is hardcoded. For better maintainability and to avoid manual updates across different configuration files, consider using a more dynamic versioning strategy or a dedicated version management tool.
💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390345189)
## 💡 Improvement Needed
**Line:** 6
The version number `v28.3.0rc1` in the DESCRIPTION section should be consistent with the version specified in the `.TH` directive (line 2) and the NAME section (line 4). This ensures accurate and unified version information throughout the manual page.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390345189)
## 💡 Improvement Needed
**Line:** 6
The version number `v28.3.0rc1` in the DESCRIPTION section should be consistent with the version specified in the `.TH` directive (line 2) and the NAME section (line 4). This ensures accurate and unified version information throughout the manual page.
💬 starkshade commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390345411)
## 💡 Improvement Needed
**Line:** 733
The default value for `blockmintxfee` has been changed from `0.00001` to `0.00000001`. This is a substantial decrease and should be clearly communicated as a potentially impactful change for users, possibly with a note about its implications for transaction fees and block inclusion.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2390345411)
## 💡 Improvement Needed
**Line:** 733
The default value for `blockmintxfee` has been changed from `0.00001` to `0.00000001`. This is a substantial decrease and should be clearly communicated as a potentially impactful change for users, possibly with a note about its implications for transaction fees and block inclusion.
💬 luke-jr commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390335595)
This feels better as a named-only param.
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390335595)
This feels better as a named-only param.
💬 luke-jr commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390359165)
This doesn't belong in rpc/mempool since it never touches the mempool...?
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390359165)
This doesn't belong in rpc/mempool since it never touches the mempool...?
💬 luke-jr commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390338390)
Probably should include the (more likely to be used) "maxfeerate" option too?
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390338390)
Probably should include the (more likely to be used) "maxfeerate" option too?
💬 luke-jr commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390361071)
Seems like this ought to check if the peer accepted it, and if not, return the rejection reason.
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390361071)
Seems like this ought to check if the peer accepted it, and if not, return the rejection reason.
💬 maflcko commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390376092)
> Upgrading Xcode 15.4 on macOS 14.4 requires updating macOS itself, at least to 14.5.
Correct. This should be no problem, because running on insecure minor releases is no different from running on EOL versions. See also the auto-upgrade: https://support.apple.com/guide/mac-help/software-update-settings-on-mac-mchla7037245/15.0/mac/15.0
> Therefore, the effective minimum supported macOS version becomes 14.5, not 14.0.
Correct. However, I don't think it is beneficial to micro-manage the
...
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2390376092)
> Upgrading Xcode 15.4 on macOS 14.4 requires updating macOS itself, at least to 14.5.
Correct. This should be no problem, because running on insecure minor releases is no different from running on EOL versions. See also the auto-upgrade: https://support.apple.com/guide/mac-help/software-update-settings-on-mac-mchla7037245/15.0/mac/15.0
> Therefore, the effective minimum supported macOS version becomes 14.5, not 14.0.
Correct. However, I don't think it is beneficial to micro-manage the
...
💬 pejfat08-rgb commented on issue "Inconsistent CJDNS address handling in Local addresses and AddLocal logs":
(https://github.com/bitcoin/bitcoin/issues/33471#issuecomment-3350965244)
- [ ]
(https://github.com/bitcoin/bitcoin/issues/33471#issuecomment-3350965244)
- [ ]
📝 willcl-ark opened a pull request: "ci: fix buildx gha cache authentication on forks"
(https://github.com/bitcoin/bitcoin/pull/33508)
When using `docker buildx build` in conjunction with the `gha` backend cache type (as we do in our CI) it's important to specify the URL and TOKEN needed to authenticate.
On Cirrus runners this is working with only `ACTIONS_CACHE_URL` and `ACTIONS_RUNTIME_TOKEN`, but this is not enough for the GitHub backend.
Fix this by exporting all `ACTIONS_*` variables.
This fixes docker build layer cache restore/save on forks or where GH-hosted runners are being used, and addresses https://github.c
...
(https://github.com/bitcoin/bitcoin/pull/33508)
When using `docker buildx build` in conjunction with the `gha` backend cache type (as we do in our CI) it's important to specify the URL and TOKEN needed to authenticate.
On Cirrus runners this is working with only `ACTIONS_CACHE_URL` and `ACTIONS_RUNTIME_TOKEN`, but this is not enough for the GitHub backend.
Fix this by exporting all `ACTIONS_*` variables.
This fixes docker build layer cache restore/save on forks or where GH-hosted runners are being used, and addresses https://github.c
...
💬 willcl-ark commented on pull request "ci: fix buildx gha cache authentication on forks":
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3351249496)
Fork run ongoing at: https://github.com/willcl-ark/bitcoin/actions/runs/18126864863
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3351249496)
Fork run ongoing at: https://github.com/willcl-ark/bitcoin/actions/runs/18126864863
💬 willcl-ark commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3351258011)
> > Yes, I've seen GHA caching work, will try and find logs.
>
> For reference, this still fails for me with `#4 ERROR: failed to parse error response 400: <h2>Our services aren't available right now</h2><p>We're working to restore all services as soon as possible. Please check back soon.</p>0eZvSaAAAAAClNmDP6CfYS7fcNJbDhpBqRE0yRURHRTA1MTYARWRnZQ==: invalid character '<' looking for beginning of value`
This did indeed require a fix, opened #33508 to address.
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3351258011)
> > Yes, I've seen GHA caching work, will try and find logs.
>
> For reference, this still fails for me with `#4 ERROR: failed to parse error response 400: <h2>Our services aren't available right now</h2><p>We're working to restore all services as soon as possible. Please check back soon.</p>0eZvSaAAAAAClNmDP6CfYS7fcNJbDhpBqRE0yRURHRTA1MTYARWRnZQ==: invalid character '<' looking for beginning of value`
This did indeed require a fix, opened #33508 to address.
💬 willcl-ark commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3351288235)
> > Can the `depends` CI cache be invalidated: (1) for this PR; (2) globally?
>
> I would have thought the cache should have been invalidated automatically, looking into it.
So what has happened here is that the depends caches have missed on an exact cache hit, but restored the latest backup hit via:
https://github.com/bitcoin/bitcoin/blob/25212dfdb4cd7291392b6a94130f658c5bfa0a48/.github/actions/restore-caches/action.yml#L29-L31
It was my understanding that `make -C depends` was able
...
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3351288235)
> > Can the `depends` CI cache be invalidated: (1) for this PR; (2) globally?
>
> I would have thought the cache should have been invalidated automatically, looking into it.
So what has happened here is that the depends caches have missed on an exact cache hit, but restored the latest backup hit via:
https://github.com/bitcoin/bitcoin/blob/25212dfdb4cd7291392b6a94130f658c5bfa0a48/.github/actions/restore-caches/action.yml#L29-L31
It was my understanding that `make -C depends` was able
...
💬 hebasto commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3351338626)
The feedback from @luke-jr and @willcl-ark have been addressed.
The PR description has been updated.
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3351338626)
The feedback from @luke-jr and @willcl-ark have been addressed.
The PR description has been updated.
👍 stickies-v approved a pull request: "build: Drop support for EOL macOS 13"
(https://github.com/bitcoin/bitcoin/pull/33489#pullrequestreview-3284002587)
ACK fa1506bdc6c072b022493c2f75ae80e762c9994b
(https://github.com/bitcoin/bitcoin/pull/33489#pullrequestreview-3284002587)
ACK fa1506bdc6c072b022493c2f75ae80e762c9994b
💬 hebasto commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#discussion_r2390916307)
Reworked.
(https://github.com/bitcoin/bitcoin/pull/33494#discussion_r2390916307)
Reworked.
👍 stickies-v approved a pull request: "wallet: reduce unconditional logging during load"
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3284021822)
ACK fc861332b351c9390400054ff74193ce26eb0713
nit: Would still prefer doing the logging from `WalletInit::Construct`, `VerifyWallets` feels out of place to me, but no biggie.
<details>
<summary>git diff on fc861332b3</summary>
```diff
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 79e2c9688d..4013e12967 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -20,6 +20,7 @@
#include <util/translation.h>
#include <wallet/coincontrol.h>
#include <wallet/walle
...
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3284021822)
ACK fc861332b351c9390400054ff74193ce26eb0713
nit: Would still prefer doing the logging from `WalletInit::Construct`, `VerifyWallets` feels out of place to me, but no biggie.
<details>
<summary>git diff on fc861332b3</summary>
```diff
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 79e2c9688d..4013e12967 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -20,6 +20,7 @@
#include <util/translation.h>
#include <wallet/coincontrol.h>
#include <wallet/walle
...
💬 stickies-v commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2390924243)
nit: docstring doesn't correspond to what's actually being printed. To avoid it going stale quickly, how about a simple:
> // Logs information about the database(s) used by the wallet.
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2390924243)
nit: docstring doesn't correspond to what's actually being printed. To avoid it going stale quickly, how about a simple:
> // Logs information about the database(s) used by the wallet.