💬 ryanofsky commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1651743320)
In commit "init: add option for rpccookie permissions" (7ad5349a2ec04399c99c8cbf71c8d3b4627132f8)

Could move this variable inside the if statement since it's not used afterwards.

🚀 ryanofsky merged a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200)

👍 ryanofsky approved a pull request: "Support self-hosted Cirrus workers on forks"
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2136964998)
Code review ACK b80a573bb42e10f1ec550c79133317c1c0b021c6, but I would think about dropping the second commit ("ci: skip Github CI on branch pushes for forks" (b9fdd0dc75b5b4944dffc700b0391b38465f754a)) and moving it to another PR, since it is a little confusing and maybe could use more discussion.

Otherwise looks very good, I just left some suggestions to try to improve some comments.

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1651755883)
In commit "ci: forks can opt-out of CI branch push (Cirrus only)" (9e30be769dfc06bf122db1ea2042a68c597509a0)

Would suggest expanding comment: "Allow forks to specify NO_BRANCH=true and skip CI runs when a branch is pushed, but still run CI when a PR is created."

Otherwise it is not clear what the NO_BRANCH variable has to do with forks or why the CIRRUS_PR condition is there.

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1651756679)
In commit "ci: forks can opt-out of CI branch push (Cirrus only)" (9e30be769dfc06bf122db1ea2042a68c597509a0)

Would it be possible as a followup to delete this skip condition and just specify NO_BRANCH=true in the bitcoin GUI cirrus environment?

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1651761313)
In commit "ci: forks can opt-out of CI branch push (Cirrus only)" (9e30be769dfc06bf122db1ea2042a68c597509a0)

How actually do you set a custom in variable in cirrus for a fork? Maybe this could suggest a way to do it or link to the cirrus docs.

Also should this say NO_BRANCH=true instead of NO_BRANCH=1 since code below appears to compare against "true" specifically?

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1651782082)
In commit "ci: skip arm if no worker is configured" (dc98f4c1ab181c79352f9a14351efe87a58f203d)

Questions / suggestions:

- It looks like code below is specifically checking $NO_ARM == "true", so should this say NO_ARM=true instead of NO_ARM=1?

- It might be good to join this paragraph with the previous paragraph so the arm information is in one place.

- It might be good to say that forks either need to provide an arm worker or set NO_ARM=true, otherwise ARM tasks will appear pending f
...

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1651793332)
In commit "ci: test-each-commit merge base optional" (b80a573bb42e10f1ec550c79133317c1c0b021c6)

Might be useful to expand commit message to explain why this change is needed. Maybe say:

- The ci "test-each-commit" job fetches the PR branch being tested with a depth of (# of commits in PR + 2), and then tries to run tests on commits after the most recent merge commit.

When a PR is opened against a bitcoin core branch, a merge commit is always guaranteed to exist within the fetch depth
...

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1651775572)
In commit "ci: skip Github CI on branch pushes for forks" (b9fdd0dc75b5b4944dffc700b0391b38465f754a)

re: https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644151691

> Well, what problem is this trying to solve, given that it introduces new problems?

It seems like this commit is trying to be more efficient by not running CI twice for branch pushes to pull requests. I think it might also stop the "[ryanofsky/bitcoin] Run failed" emails I currently see when I push branches to my r
...

👍 tdb3 approved a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2137071147)
re ACK e4798038c00e787023b9dedc907966a08592d79f

Ran `rpc_users` locally (passed).
Re-ran the manual check in https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2105557076 (same results).
Left one minor nit.

💬 tdb3 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1651821391)
nit: If touching this file again, may want to consider defining a default argument for cookie_perms. Since std::optional is used, I'm assuming the intent is not to force the caller to provide fs::perms.

```diff
diff --git a/src/rpc/request.h b/src/rpc/request.h
index b40df16631..c7e723d962 100644
--- a/src/rpc/request.h
+++ b/src/rpc/request.h
@@ -19,7 +19,7 @@ std::string JSONRPCReply(const UniValue& result, const UniValue& error, const Un
UniValue JSONRPCError(int code, const std::
...

💬 ryanofsky commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1651844249)
In commit "refactor: PSBTError::MISSING_INPUTS" (77ef75d3ce4bff76a0db730b57896dcf7e8f3988):

I think it would be a little better if this message said just "Invalid inputs" instead of "Invalid inputs specified" since the input indices are internal to the PSBT data structure, not really specified by the user.

Someone more familiar with PSBT code than me might be able to suggest a better error code and message than this to use, but at least with this change the error should be more accurate.

...

👍 ryanofsky approved a pull request: "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN"
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2137101372)
Code review ACK 77ef75d3ce4bff76a0db730b57896dcf7e8f3988 if test failure is fixed (see comment below)

👍 tdb3 approved a pull request: "optimization: Moved repeated `-printpriority` fetching out of AddToBlock"
(https://github.com/bitcoin/bitcoin/pull/30324#pullrequestreview-2137173778)
ACK 6e519b83188e77c401bb9024196c499bee67da68

Thanks for optimizing.
Saw around a 6% speedup on my machine.

⚠️ Mariuszok12 opened an issue: "BTC address change bc1qee7hk4a3k3km7j8hwclm0pkl76dhhgxay5nevu"
(https://github.com/bitcoin/bitcoin/issues/30331)
### Please describe the feature you'd like to see added.

BTC withdrawal
bc1qee7hk4a3k3km7j8hwclm0pkl76dhhgxay5nevu

### Is your feature related to a problem, if so please describe it.

_No response_

### Describe the solution you'd like

_No response_

### Describe any alternatives you've considered

_No response_

### Please leave any additional context

_No response_

✅ fanquake closed an issue: "BTC address change bc1qee7hk4a3k3km7j8hwclm0pkl76dhhgxay5nevu"
(https://github.com/bitcoin/bitcoin/issues/30331)

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1652065170)
Thank you for the suggestions and learning opportunity! Made all the suggested changes since they seem like improvements to me. Rebased as well.

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1652111765)
I made a note to do this along with a PR to add something like `waitTipChanged()`
https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2160764070

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1652116935)
Yes, I could even change that here if someone sets `NO_BRANCH=true` before merging this.

🤔 maflcko reviewed a pull request: "optimization: Moved repeated `-printpriority` fetching out of AddToBlock"
(https://github.com/bitcoin/bitcoin/pull/30324#pullrequestreview-2137816916)
Not sure if this is used at all. Maybe it can be removed completely? Otherwise, see the feedback.