✅ maflcko closed a pull request: "test: Trigger UB in wallet crosschain migration"
(https://github.com/bitcoin/bitcoin/pull/32988)
(https://github.com/bitcoin/bitcoin/pull/32988)
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2209598289)
Worth mentioning, the only reason we wouldn't be able to do this is if this function (and `TopUp`) are called generically on SPKMans, e.g., in a loop. I don't think this is the case, but wanted to mention it.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2209598289)
Worth mentioning, the only reason we wouldn't be able to do this is if this function (and `TopUp`) are called generically on SPKMans, e.g., in a loop. I don't think this is the case, but wanted to mention it.
💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2209623040)
Hi @achow101 kindly give some update on this.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2209623040)
Hi @achow101 kindly give some update on this.
📝 willcl-ark opened a pull request: "Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989)
This changeset migrates all current self-hosted CI jobs over to hosted [Cirrus Runners](https://cirrus-runners.app/).
These runners cost a flat rate of $150/month, and we qualify for an open source discount of 50%. Therefore they are $75/month/runner.
One "runner" should more accurately be thought of in terms of the number of vCPU you are purchasing: https://cirrus-runners.app/pricing/ or in terms of "concurrency", where 1 runners gets you 1.0 concurrency.
e.g. a Linux x86 Runner gets you
...
(https://github.com/bitcoin/bitcoin/pull/32989)
This changeset migrates all current self-hosted CI jobs over to hosted [Cirrus Runners](https://cirrus-runners.app/).
These runners cost a flat rate of $150/month, and we qualify for an open source discount of 50%. Therefore they are $75/month/runner.
One "runner" should more accurately be thought of in terms of the number of vCPU you are purchasing: https://cirrus-runners.app/pricing/ or in terms of "concurrency", where 1 runners gets you 1.0 concurrency.
e.g. a Linux x86 Runner gets you
...
🤔 BrandonOdiwuor reviewed a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3023823917)
Tested ACK 01688e17534d3c9011cce92cff9f7935691bb80c
Was able to verify that this fixes the JSON parsing error on `unloadwallet` and `getdescriptoractivity` RPCs
Tested on macOS 15.5
Branch: Master
<img width="870" height="203" alt="Screenshot 2025-07-16 at 11 41 41" src="https://github.com/user-attachments/assets/88508a93-3dfb-4676-9040-d8789677da1a" />
Commit: 01688e17534d3c9011cce92cff9f7935691bb80c
<img width="1511" height="887" alt="Screenshot 2025-07-15 at 14 36 44" src="http
...
(https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3023823917)
Tested ACK 01688e17534d3c9011cce92cff9f7935691bb80c
Was able to verify that this fixes the JSON parsing error on `unloadwallet` and `getdescriptoractivity` RPCs
Tested on macOS 15.5
Branch: Master
<img width="870" height="203" alt="Screenshot 2025-07-16 at 11 41 41" src="https://github.com/user-attachments/assets/88508a93-3dfb-4676-9040-d8789677da1a" />
Commit: 01688e17534d3c9011cce92cff9f7935691bb80c
<img width="1511" height="887" alt="Screenshot 2025-07-15 at 14 36 44" src="http
...
📝 rkrux opened a pull request: "wallet: remove outdated `pszSkip` arg of database `Rewrite` func"
(https://github.com/bitcoin/bitcoin/pull/32990)
This argument might have been used in the legacy wallets, but I don't see any implementation using this argument in the SQLite wallets. Removing it cleans up the code a bit.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation
...
(https://github.com/bitcoin/bitcoin/pull/32990)
This argument might have been used in the legacy wallets, but I don't see any implementation using this argument in the SQLite wallets. Removing it cleans up the code a bit.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation
...
💬 rkrux commented on pull request "wallet: remove outdated `pszSkip` arg of database `Rewrite` func":
(https://github.com/bitcoin/bitcoin/pull/32990#issuecomment-3077678719)
I noted this while reviewing #28333.
(https://github.com/bitcoin/bitcoin/pull/32990#issuecomment-3077678719)
I noted this while reviewing #28333.
💬 fanquake commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209816654)
Can this be specific? I think either document the warning, or drop this comment, or just drop the workaround, if it's only a warning.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209816654)
Can this be specific? I think either document the warning, or drop this comment, or just drop the workaround, if it's only a warning.
💬 fanquake commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209824251)
> and can help achieve
This is a bit vauge; have we seen this not work properly? It's also not completely clear to me that we need buildx to use the registry. Seems like it also works with buildkit, which is now just the default `docker build`, it any recent Docker version.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209824251)
> and can help achieve
This is a bit vauge; have we seen this not work properly? It's also not completely clear to me that we need buildx to use the registry. Seems like it also works with buildkit, which is now just the default `docker build`, it any recent Docker version.
💬 fanquake commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3077789486)
Concept ACK. This will also need to go back to `27.x`.
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3077789486)
Concept ACK. This will also need to go back to `27.x`.
💬 fanquake commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209832345)
Wonder if we should just move this into the scripts, rather than set it here, so that it'll also be done for local usage.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209832345)
Wonder if we should just move this into the scripts, rather than set it here, so that it'll also be done for local usage.
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-3077810296)
CI failure is due to the "test each commit" job not being able to handle a subtree merge commit. Ignoring for now; will look into fixing the CI job.
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-3077810296)
CI failure is due to the "test each commit" job not being able to handle a subtree merge commit. Ignoring for now; will look into fixing the CI job.
💬 josibake commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#issuecomment-3077814381)
CI failure is from the test each commit job not handling a subtree pull; ignoring for now and will look into a CI fix later.
(https://github.com/bitcoin/bitcoin/pull/28201#issuecomment-3077814381)
CI failure is from the test each commit job not handling a subtree pull; ignoring for now and will look into a CI fix later.
✅ rkrux closed a pull request: "test: test that descriptorprocesspsbt removes non witness utxos in PSBT"
(https://github.com/bitcoin/bitcoin/pull/32413)
(https://github.com/bitcoin/bitcoin/pull/32413)
💬 rkrux commented on pull request "test: test that descriptorprocesspsbt removes non witness utxos in PSBT":
(https://github.com/bitcoin/bitcoin/pull/32413#issuecomment-3077817862)
Closing for now, will reopen later if this piques someone's interest.
(https://github.com/bitcoin/bitcoin/pull/32413#issuecomment-3077817862)
Closing for now, will reopen later if this piques someone's interest.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2209852968)
Cherry-picked this commit into https://github.com/bitcoin/bitcoin/pull/28201/commits/b94934e089e70b478ea6aa535098586fedb5a12d, in #28201. I added it to the commit where we enable silent payment addresses as valid addresses, at the end of the sending PR.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2209852968)
Cherry-picked this commit into https://github.com/bitcoin/bitcoin/pull/28201/commits/b94934e089e70b478ea6aa535098586fedb5a12d, in #28201. I added it to the commit where we enable silent payment addresses as valid addresses, at the end of the sending PR.
⚠️ josibake opened an issue: "ci: improve "test each commit" job to handle more complex scenarios"
(https://github.com/bitcoin/bitcoin/issues/32991)
### Please describe the feature you'd like to see added.
In #28122, I start by updating the libsecp subtree with `git subtree pull --prefix src/secp256k1 <myfork> <mybranch> --squash`. When rebasing on this commit, I often need to use something like:
```
git rebase --rebase-merges --strategy subtree -X theirs
```
.. which Just Works. However, now the CI fails when running:
```
git rebase --exec 'git merge --no-commit ...'
```
.. as seen in https://github.com/bitcoin/bitcoin/actions/runs/163
...
(https://github.com/bitcoin/bitcoin/issues/32991)
### Please describe the feature you'd like to see added.
In #28122, I start by updating the libsecp subtree with `git subtree pull --prefix src/secp256k1 <myfork> <mybranch> --squash`. When rebasing on this commit, I often need to use something like:
```
git rebase --rebase-merges --strategy subtree -X theirs
```
.. which Just Works. However, now the CI fails when running:
```
git rebase --exec 'git merge --no-commit ...'
```
.. as seen in https://github.com/bitcoin/bitcoin/actions/runs/163
...
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209918817)
It is only a warning.
It's not present current as we are using podman, but docker does this build check: https://docs.docker.com/reference/build-checks/invalid-default-arg-in-from/
If we are happy with a warning on every `docker buildx build ...` invocation we can drop it, but this hack silences the warning and will always fail the build if `scratch` ends up being used (and should be pretty easy to diagnose the failure).
Preference between adding a link to the specific check for more co
...
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209918817)
It is only a warning.
It's not present current as we are using podman, but docker does this build check: https://docs.docker.com/reference/build-checks/invalid-default-arg-in-from/
If we are happy with a warning on every `docker buildx build ...` invocation we can drop it, but this hack silences the warning and will always fail the build if `scratch` ends up being used (and should be pretty easy to diagnose the failure).
Preference between adding a link to the specific check for more co
...
🤔 pinheadmz reviewed a pull request: "net: Fix Discover() not running when using -bind=0.0.0.0:port"
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-3024172439)
Thanks for fixing the nit in the test.
In general, nit-fixes should be squashed (not added as an extra commit) so the final commit history is clean and perfect ;-)
Also I think you rebased on master which is ok, but it makes review slightly harder because my local copy of your branch is now dozens of commits different from the current state of the PR. I used `git range-diff` to isolate the changes which is why its not like, a super hard rule, but again in general, try not to rebase on mast
...
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-3024172439)
Thanks for fixing the nit in the test.
In general, nit-fixes should be squashed (not added as an extra commit) so the final commit history is clean and perfect ;-)
Also I think you rebased on master which is ok, but it makes review slightly harder because my local copy of your branch is now dozens of commits different from the current state of the PR. I used `git range-diff` to isolate the changes which is why its not like, a super hard rule, but again in general, try not to rebase on mast
...
💬 pinheadmz commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2209917430)
If you really want to log the RPC output, you could capture it in a variable just before the `for local in...` and then iterate that variable inside the for loop, and print it in the error message. It's easy to reproduce this if you need to test it (I had trouble getting 1.1.1.1 and 2.2.2.2 set up on my machine for this test!)
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2209917430)
If you really want to log the RPC output, you could capture it in a variable just before the `for local in...` and then iterate that variable inside the for loop, and print it in the error message. It's easy to reproduce this if you need to test it (I had trouble getting 1.1.1.1 and 2.2.2.2 set up on my machine for this test!)