💬 achow101 commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2187344321)
75bcc1057bbf7d8e0803df09780f4237c10c4264 says "rand_exp_delay" in the commit message, but the actual function is "rand_exp_duration".

💬 romanz commented on pull request "rest: don't copy data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2187360008)
Added https://github.com/bitcoin/bitcoin/commit/9c9375cf3b581c31f047b87a2c05507dc7b31804 with a few more fixes, following [the suggestion above](https://github.com/bitcoin/bitcoin/pull/30321#discussion_r1649705033).

💬 fjahr commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2187466319)
Concept ACK

💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2187485180)
Rebased to fix merge conflict

FYI: This PR is scheduled as the topic for the PR Review Club on July 3rd: https://bitcoincore.reviews/29775 You can see a preview of my suggested questions here: https://github.com/bitcoin-core-review-club/website/pull/765 Consider yourself invited!

👍 tdb3 approved a pull request: "rest: don't copy data when sending binary response"
(https://github.com/bitcoin/bitcoin/pull/30321#pullrequestreview-2136908546)
re ACK 9c9375cf3b581c31f047b87a2c05507dc7b31804

Re-ran the tests in https://github.com/bitcoin/bitcoin/pull/30321#pullrequestreview-2133999354

Also performed similar PR branch / master (538363738e9e30813cf3e76ca4f71c1aaff349e7) comparison tests with `tx`, `headers`, `blockhashbyheight`, and `getutxos` endpoints. All matched.
```
curl -v http://127.0.0.1:38332/rest/tx/7382c61e570cbc882a3d32b272318d353386981a475f58341fb4268971a6f1ec.bin
curl -v http://127.0.0.1:38332/rest/headers/0000005
...

⚠️ techy2 opened an issue: "error cross compiling linux X64 => 32 bit i686"
(https://github.com/bitcoin/bitcoin/issues/30330)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

While building depends for
make HOST=i686-pc-linux-gnu -j8

issues this warning
configure: WARNING: using cross tools not prefixed with host triplet
.... then fails .....
configure: error: in `/master/gitrepo/bitcoin/depends/work/build/i686-pc-linux-gnu/libevent2.1.12-stable-f9904577bfb':
configure: error: C compiler cannot create executables


### Expected behaviour

clean bui
...

✅ techy2 closed an issue: "automake script error building 32 bit depends libevent-2.1.12"
(https://github.com/bitcoin/bitcoin/issues/30311)

💬 techy2 commented on issue "automake script error building 32 bit depends libevent-2.1.12":
(https://github.com/bitcoin/bitcoin/issues/30311#issuecomment-2187569050)
Ok, will open new issue cross compiling X64 => i686 32 bit

👍 ryanofsky approved a pull request: "init: Add option for rpccookie permissions (replace 26088)"
(https://github.com/bitcoin/bitcoin/pull/28167#pullrequestreview-2136945831)
Code review ACK e4798038c00e787023b9dedc907966a08592d79f. Seems very clean now with the test simplification and without the win32 stuff.

💬 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::
...