💬 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.
💬 stickies-v commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2390909544)
nit: the function name is informative enough imo, no need for docstring here
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2390909544)
nit: the function name is informative enough imo, no need for docstring here
📝 maflcko opened a pull request: "ci: Check macos-cross"
(https://github.com/bitcoin/bitcoin/pull/33509)
bla
(https://github.com/bitcoin/bitcoin/pull/33509)
bla
💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2391260220)
Hmm, reminds me that this hunk (and the one above) for `windows-cross` and `windows-native-test` was erroneous to replace, because the two tasks depend on each other and must checkout the exact same commit. Otherwise, there could be a CI failure when a pull is merged in between the run of the first task and the second.
Possible solutions could be:
* Revert the two hunks and restore the default behavior here.
* Export the commit ID from the first task and plug it into the checkout of the
...
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2391260220)
Hmm, reminds me that this hunk (and the one above) for `windows-cross` and `windows-native-test` was erroneous to replace, because the two tasks depend on each other and must checkout the exact same commit. Otherwise, there could be a CI failure when a pull is merged in between the run of the first task and the second.
Possible solutions could be:
* Revert the two hunks and restore the default behavior here.
* Export the commit ID from the first task and plug it into the checkout of the
...
👋 willcl-ark's pull request is ready for review: "ci: fix buildx gha cache authentication on forks"
(https://github.com/bitcoin/bitcoin/pull/33508)
(https://github.com/bitcoin/bitcoin/pull/33508)