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

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1665456319)
Updated b0beb4c504da29c27358d4602a045aaab39305f6 -> 9e80d0b754a28733c79a52c8e0431616c31d071c ([`pr/bresult2.38`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.38) -> [`pr/bresult2.39`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.39), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.38..pr/bresult2.39)) to fix `operator>>` compile error that seemed to happen with newer versions of clang: https://cirrus-ci.com/task/4622529512341504?logs=ci#L2370

💬 Retropex commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665460988)
Concept NACK.

We already have enough problems with inscriptions (That the developer still seems to ignore). In addition, why delete the `-datacarrier` option? This is an important option that gives nodes runner the choice, you could have simply removed the default limit.

Instead, focus on giving nodes runner the choice by implementing more options to customize the mempool policy.

💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284321897)
Would it make sense to test the other edge, 1-of-0?

📝 darosior opened a pull request: "fuzz: fix a couple incorrect assertions in the `coins_view` target"
(https://github.com/bitcoin/bitcoin/pull/28215)
I ran into this while introducing a new target with an in-memory `CCoinsViewDB` as the backend view which made the code code paths with those assertions actually reachable.

See commits messages for details.

👍 stickies-v approved a pull request: "scripted-diff: Specify Python major version explicitly on Windows"
(https://github.com/bitcoin/bitcoin/pull/28213#pullrequestreview-1562734864)
utACK 6a7686b44618eabd2f8ee9f1d357cfeb1bce6d91

💬 iBeizsley commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1665503510)
> The 40 or 80B limit was based on storing a hash or two, surprisingly nobody thought at that time that you must store a signed hash as @ChristopherA and myself wants to do

Any hash you include in an op return is a signed hash. It's signed by the keys controlling the inputs.