💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1284207518)
Does apple still ship Intel silicon for new macOS machines? Regardless, I don't really see a benefit in running the same task twice when there is no reason to believe it will find more bugs or is even useful in the long term.

💬 Sjors commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284212036)
I understand descriptors, but Python less well :-) What is this function trying to do?

💬 ChrisCho-H commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1665337473)
Seems no one's against @vostrnad suggestion, I convert all single lines to block comments, add textual description with notation and reference for OP_SUCCESS opcodes in case of tapscript.
I didn't include history much. In my opinion, description only for function itself looks clean and explicit which fits with main purpose of this.

📝 MarcoFalke opened a pull request: "ci: Move tidy and macOS-cross to persistent workers"
(https://github.com/bitcoin/bitcoin/pull/28214)
Cirrus CI will be capping the free compute soon. For now, switch more tasks to persistent worker, as recommended by Cirrus CI.

(See slightly related discussion in https://github.com/bitcoin/bitcoin/issues/28098)

Also, add more docs.

💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284233075)
If I didn't bump the fee rate I would run into the two error codes mentioned in the PR message:
```
Had to bump fee rate to resolve assertions in sendtoaddress and psbt methods:

code -5: assert rpc_online.gettransaction(txid)["confirmations"] > 0
code -26: min relay fee not met
```

The previous `fee_rate = 200` limit seems to be sufficient up until `n ~ 500`. I assume this is because the script size increases with higher numbers of `n`.

💬 hebasto commented on pull request "ci: Move tidy and macOS-cross to persistent workers":
(https://github.com/bitcoin/bitcoin/pull/28214#issuecomment-1665359618)
macOS task has not been started:

💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284239102)
I guess that would do the trick, however I'm unsure if by doing so we modify the test to test the assertion, which seems a bit incomplete/unintuitive to me, rather than testing the edge cases in k-of-N wallets? 🤔 If you understand what I mean 😄 If you don't think it's an issue, I'm all for keeping it simple and just test 1-of-1000.

💬 Sjors commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284246711)
Fee rate is the fee per vbyte. But this may be related to #26573

💬 MarcoFalke commented on pull request "ci: Move tidy and macOS-cross to persistent workers":
(https://github.com/bitcoin/bitcoin/pull/28214#issuecomment-1665375160)
> UPD: Scheduled in 14 minutes:

When there are pushes to many pull requests at the same time, scheduling may take a few minutes. This is similar to the Cirrus CI hosted open source compute cluster.

💬 Sjors commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284249102)
Well the "edge" here is n = 999, but this test is varying `k`, not `n`.

💬 Sjors commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1665379268)
> Probably worth just squashing this down to 1 commit.

Indeed.

💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284252603)
It's a function that allows setup and testing of any k-of-n wallet. It builds on the idea of the existing test that @darosior pointed to, which starts on L495 https://github.com/bitcoin/bitcoin/blob/64440bb733896a7a2caf902825e0406cb993e666/test/functional/wallet_taproot.py#L495 where a random number of `n` is used.

The part that differs is how the `[KEY_1,...,KEY_N]` array inside the `multi_a` is built - where the previous function uses `([H_POINT] * rnd_pos)` up until `k1` and then `([H_POIN
...

💬 stickies-v commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#issuecomment-1665383160)
Rebased to address merge conflict from https://github.com/bitcoin/bitcoin/pull/28161.

💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284253877)
Ah you're right. I'll simplify the test to only do 1-of-1000.

💬 pinheadmz commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#discussion_r1284255067)
When I set CIRRUS_CI to true locally I end up failing installing the kernel headers. I see this error running this branch locally on both Ubuntu and macOS hosts. But the "generic" version there is coming from inside the container right (`uname --kernel-release`)? So I'm confused why this works on actual CI but not locally.

```
#8 7.217 E: Unable to locate package linux-headers-5.15.0-78-generic
#8 7.217 E: Couldn't find any package by glob 'linux-headers-5.15.0-78-generic'
#8 7.217 E: Coul
...

💬 MarcoFalke commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#discussion_r1284261678)
When you want to run the exact same env like the Cirrus worker does, you will also need the exact same machine:

A "machine running the Linux kernel shipped with Ubuntu Lunar 23.04. The machine is recommended to have 4 CPUs and 16 GB of memory."

You can check the version locally with `cat /etc/os-release`.

However, be warned that when you set CIRRUS_CI to true locally, you'll pass `--privileged -v /sys/kernel:/sys/kernel:rw` to docker, which allows the CI system more privileges.

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1665396151)
https://github.com/bitcoin/bitcoin/pull/28214 is the second step

💬 pinheadmz commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#discussion_r1284264509)
Ah 🤦‍♂️ the comment you added in response to my first review. Thanks!

💬 MarcoFalke commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#discussion_r1284267996)
I think if one wants to not run the CI again, they'll need to use `rm` instead of `prune`, no? The goal of prune is to only remove dangling/stale/intermediate image layers?

💬 stickies-v commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#discussion_r1284283773)
Yeah, would you prefer it like this?

````diff
diff --git a/ci/README.md b/ci/README.md
index ab5e6ce866..16310b7703 100644
--- a/ci/README.md
+++ b/ci/README.md
@@ -26,13 +26,19 @@ To run the test stage with a specific configuration,
FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh
```

-The ci automatically prunes all dangling docker images every time it runs. If you want to perform
-the cleanup manually (e.g. if you won't be running the ci again), you can run
+The
...