💬 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!)
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3077943430)
> Concept ACK. This will also need to go back to `27.x`.
Testing a backport to 29.x here: https://github.com/testing-cirrus-runners/bitcoin2/actions/runs/16316701949
I think the best course of action could be to look for a little more conceptual review here, and after that squash the "ci: port x" commits in this changeset down to a single one, to make backporting to the multiple supported branches easier.
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3077943430)
> Concept ACK. This will also need to go back to `27.x`.
Testing a backport to 29.x here: https://github.com/testing-cirrus-runners/bitcoin2/actions/runs/16316701949
I think the best course of action could be to look for a little more conceptual review here, and after that squash the "ci: port x" commits in this changeset down to a single one, to make backporting to the multiple supported branches easier.
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209927521)
That would make sense to me too. Happy to make that change here, or in a followup.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209927521)
That would make sense to me too. Happy to make that change here, or in a followup.
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209934860)
Yes, we have tested that even if we configure the docker builder using the [`setup-buildx-action`](https://github.com/bitcoin/bitcoin/pull/32989/files#diff-fb343e432f60927181ab89952fb0df6f7a0067b0f65727f833efad19b88e5c46R15) where `use` is set to `true` [by default](https://github.com/docker/setup-buildx-action#inputs) (which should enable that driver in future `docker build` invocations IIUC), that the incorrect builder is used unless `docker buildx build` command specifically is used.
We ha
...
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209934860)
Yes, we have tested that even if we configure the docker builder using the [`setup-buildx-action`](https://github.com/bitcoin/bitcoin/pull/32989/files#diff-fb343e432f60927181ab89952fb0df6f7a0067b0f65727f833efad19b88e5c46R15) where `use` is set to `true` [by default](https://github.com/docker/setup-buildx-action#inputs) (which should enable that driver in future `docker build` invocations IIUC), that the incorrect builder is used unless `docker buildx build` command specifically is used.
We ha
...
💬 fanquake commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209976950)
Ok. Could we change this to something like: `Using buildx is required to properly load the correct driver, for use with registry caching. Neither build, nor BUILDKIT=1 currently do this properly`. Just want to avoid phrases like "can help" or "is useful", and instead be specific about what we are doing & why.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2209976950)
Ok. Could we change this to something like: `Using buildx is required to properly load the correct driver, for use with registry caching. Neither build, nor BUILDKIT=1 currently do this properly`. Just want to avoid phrases like "can help" or "is useful", and instead be specific about what we are doing & why.
💬 maflcko commented on issue "ci: improve "test each commit" job to handle more complex scenarios":
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3078016710)
That's interesting. For other subtree merges, it does seem to work.
* E.g. https://github.com/bitcoin/bitcoin/actions/runs/16022448902/job/45202410762?pr=32856#step:7:1, where it correctly skips over all commits, because after the merge commit, there is only one commit.
* E.g. https://github.com/bitcoin/bitcoin/actions/runs/16008106824/job/45159350306?pr=32345#step:7:19, where it correctly starts at `commit f8fd3959d51f6f80b428828c4fd965e06a8fa19e` (after the merge) and stops at `commit 008f4ae
...
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3078016710)
That's interesting. For other subtree merges, it does seem to work.
* E.g. https://github.com/bitcoin/bitcoin/actions/runs/16022448902/job/45202410762?pr=32856#step:7:1, where it correctly skips over all commits, because after the merge commit, there is only one commit.
* E.g. https://github.com/bitcoin/bitcoin/actions/runs/16008106824/job/45159350306?pr=32345#step:7:19, where it correctly starts at `commit f8fd3959d51f6f80b428828c4fd965e06a8fa19e` (after the merge) and stops at `commit 008f4ae
...
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2210014658)
why not switch to buildx unconditionally?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2210014658)
why not switch to buildx unconditionally?
👍 stickies-v approved a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3024249604)
ACK 60f9dc8af66bbd4eda81049a10262527f83356a1 . Left a couple improvement suggestions that I think make sense to address here, but also aren't blocking. Happy to quickly re-review if you incorporate.
I think adding (very brief) release notes to indicate the slight behaviour change for `unloadwallet` and `getdescriptoractivity` could be useful for downstream projects.
(https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3024249604)
ACK 60f9dc8af66bbd4eda81049a10262527f83356a1 . Left a couple improvement suggestions that I think make sense to address here, but also aren't blocking. Happy to quickly re-review if you incorporate.
I think adding (very brief) release notes to indicate the slight behaviour change for `unloadwallet` and `getdescriptoractivity` could be useful for downstream projects.
💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2209990945)
nit: Unfortunately we have to keep the name "scanobjects" for backwards compatibility, but I think the description should be made more relevant to what this RPC is doing, in line with the `blockhashes` documentation, e.g.
```suggestion
RPCArg{"scanobjects", RPCArg::Type::ARR, RPCArg::Optional::NO, "The list of descriptors (scan objects) to examine for activity. Every scan object is either a string descriptor or an object:", {
```
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2209990945)
nit: Unfortunately we have to keep the name "scanobjects" for backwards compatibility, but I think the description should be made more relevant to what this RPC is doing, in line with the `blockhashes` documentation, e.g.
```suggestion
RPCArg{"scanobjects", RPCArg::Type::ARR, RPCArg::Optional::NO, "The list of descriptors (scan objects) to examine for activity. Every scan object is either a string descriptor or an object:", {
```
💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2209965857)
side note: the docstring for this RPC seems out of date, could be sensible to fix in this PR? e.g.
<details>
<summary>git diff on 01688e1753</summary>
```diff
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
index 67b7aafc00..26033d2119 100644
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -438,8 +438,8 @@ static RPCHelpMan createwallet()
static RPCHelpMan unloadwallet()
{
return RPCHelpMan{"unloadwallet",
- "Unloads the
...
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2209965857)
side note: the docstring for this RPC seems out of date, could be sensible to fix in this PR? e.g.
<details>
<summary>git diff on 01688e1753</summary>
```diff
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
index 67b7aafc00..26033d2119 100644
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -438,8 +438,8 @@ static RPCHelpMan createwallet()
static RPCHelpMan unloadwallet()
{
return RPCHelpMan{"unloadwallet",
- "Unloads the
...
💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2210134549)
I wrote some unit tests for this function, feel free to add to this PR if you think it's helpful: https://github.com/stickies-v/bitcoin/commit/60f9dc8af66bbd4eda81049a10262527f83356a1
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2210134549)
I wrote some unit tests for this function, feel free to add to this PR if you think it's helpful: https://github.com/stickies-v/bitcoin/commit/60f9dc8af66bbd4eda81049a10262527f83356a1
💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2210018576)
nit: I think the first paragraph of the 01688e17534d3c9011cce92cff9f7935691bb80c commit message can be improved to:
> Mark `blockhashes` and `scanobjects` arguments as required, so the user receives
> a clear help message when either is missing.
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2210018576)
nit: I think the first paragraph of the 01688e17534d3c9011cce92cff9f7935691bb80c commit message can be improved to:
> Mark `blockhashes` and `scanobjects` arguments as required, so the user receives
> a clear help message when either is missing.
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2210214872)
Does podman accept `docker buildx build` as a command? I will check in a min with it.
but this was why not originally…
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2210214872)
Does podman accept `docker buildx build` as a command? I will check in a min with it.
but this was why not originally…