👍 ryanofsky approved a pull request: "Support self-hosted Cirrus workers on forks"
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2139263214)
Code review ACK 0882babb102108a4351714cc3749fceba1ecbb35.

---

I think the current PR title / description are a little hard to understand. Maybe consider editing it to something like the following:

- Fix issues with CI on forks

I find myself making pull requests against my fork (mostly on top of [#28983](https://github.com/bitcoin/bitcoin/pull/28983), or asking others to do so. Currently only the Github actions are run on forks, because we use self-hosted runners for the Cirrus task
...

💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653307211)
Didn't verify, but could all of this be replaced by a call to `fs::path GetDefaultDataDir()`?

👍 itornaza approved a pull request: "Mining interface followups"
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2139445312)
tested ACK 22fe97a5cc612f438ec6280757487934501bf527

Manually validated all instances of `testBlockValidity` in the entire code base are having their parameter list updated with `state` as their last output parameter using good old `git grep -n testBlockValidity`. Now, the interface can be used with the `libmultiprocess` code generator!

As for the standard testing, I got the pr from upstream, built and run all unit and functional tests including the entire extended test harness and all test
...

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1653310379)
I find this equally confusing, but was a bit reluctant to add a manual for Cirrus. Anyway, added.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2189651581)
@ryanofsky thanks for the PR description rewrite!

💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653311450)
(It may also help with the Win CI errors)

💬 ryanofsky commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2189661783)
@theuni I made a similar change in #29256. That approach might be less invasive because it makes the instance argument optional. So kernel code can be changed to log to a configurable instance while other code uses a global instance.

👍 ryanofsky approved a pull request: "Support self-hosted Cirrus workers on forks"
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2139490748)
Code review ACK 576828e732bacb61b608cd89f669a2936d3e8d00.

I also tweaked formatting in the PR description to fix a few problems in the markdown copy / paste.

The suggested text "Fix issues with CI on forks" in the description was actually meant to be a replacement title for the PR, since the second commit in the PR fixes an issue with a github actions job, not a cirrus job. So could remove this and maybe update the title.

🤔 mzumsande reviewed a pull request: "test/BIP324: disconnection scenarios during v2 handshake"
(https://github.com/bitcoin/bitcoin/pull/29431#pullrequestreview-2139518716)
ACK c9dacd958d7c1e98b08a7083c299d981e4c11193

I'd prefer combining some of the first commits with the respective test commits that actually make use of the change, but that's just my opinion ad maybe a matter of taste.

💬 mzumsande commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1653358992)
looks good to me now!

👍 rkrux approved a pull request: "refactor, wallet: get serialized size of `CRecipient`s directly"
(https://github.com/bitcoin/bitcoin/pull/30050#pullrequestreview-2139514584)
tACK [a9c7300](https://github.com/bitcoin/bitcoin/pull/30050/commits/a9c7300135f0188daa5bce5491e2daf2dd8da8ae)

`make, test/functional are successful`.

From what I understand about Silent Payments, the calculation of the recipient address is dependent on the input hash for which it makes sense to push down the `txoutputs` addition _after_ coin selection.

💬 rkrux commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1653355957)
Although being a bit pedantic, but as this commit is supposed to be move-only, the removal of `CTxOut txout...` in line 1107 and the replacement of `push_back` with `emplace_back` in line 1115 could have been part of the previous commit to avoid any confusion for future readers.

💬 romanz commented on pull request "rest: don't copy data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#discussion_r1653368609)
Thanks - fixed in 24e7925abb.

💬 TheCharlatan commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2189768540)
Guix builds (aarch64):
```
d702d02df48bc540da55c47ca7110d122a27ba179ab728fb8bdb6e27589f754c guix-build-f59e9057e2aa/output/aarch64-linux-gnu/SHA256SUMS.part
d806e1994bc873a975714ce75bca87fd3fcec4055e24d1ed0afe99ebc3503288 guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu-debug.tar.gz
a9844e6a4d3e86df69b98281c942ac01136b4b115fc6a2504740368c5d441fbd guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu.tar.gz
4e3ea3b82
...

📝 brunoerg opened a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339)
We currently do not test a successful call to `getaddednodeinfo` filtering by `node`, we only test it with an unknown address and checks whether it fails. This PR adds coverage to it.

💬 TheCharlatan commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2189810744)
Concept ACK

Re https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2090083022

> Doing that meaningfully means changing all the logging in all the bitcoin-kernel related modules (eg validation, mempool, blockstorage, scheduler, ...) to explicitly specify a logger rather than using the global functions, which seems like a lot of pointless busy work? I can't imagine why we'd want to support having multiple bitcoin-kernel instances in a single executable logging to different loggers; if
...

👍 willcl-ark approved a pull request: "contrib: Fixup verify-binaries OS platform parsing"
(https://github.com/bitcoin/bitcoin/pull/30147#pullrequestreview-2139666308)
re-ACK 3ab25201909bece9066ac6191670bcee09791d54

Based on range diff since previous review: `git range-diff a2fc9ddee9cbaeffb3c460973bab3c2dd734db55...3ab25201909bece9066ac6191670bcee09791d54`

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653457550)
It doesn't quite work, because it would return:
```
fs::path GetDefaultDataDir()
{
// Windows:
// old: C:\Users\Username\AppData\Roaming\Bitcoin
// new: C:\Users\Username\AppData\Local\Bitcoin
// macOS: ~/Library/Application Support/Bitcoin
// Unix-like: ~/.bitcoin
```
but we are looking for:
```
// Windows: C:\Users\Satoshi\
// macOS: /Users/satoshi/
// Unix-like: /home/satoshi/
```

💬 willcl-ark commented on issue "Setting `bip32derivs` to `false` with `walletprocesspsbt` includes `bip32_derivs` for outputs.":
(https://github.com/bitcoin/bitcoin/issues/30294#issuecomment-2189839521)
Hi @Fonta1n3

I've taken a look at this , but in making a few changes one thing I wasn't sure of from a design perspective, was whether it would be more (or less) deisrable to have the `bip32derivs` bool always toggle _both_ input and output derivations, or might be preferable to have an option for doing each independently?

I think having them both effected by the same option probably makes more sense (in terms of what a user would "expect"), and I think it would actually still be possible
...

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653478731)
> It doesn't quite work, because it would return:
> ...
> // Unix-like: ~/.bitcoin

Don't trust the comment, it gives me the absolute path. Tested on Linux with:
```C++
const fs::path absolutePath = GetDefaultDataDir() / fs::path("specialWallet") + fs::path::preferred_separator;
```