💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457664231)
My previous comment here wasn't addressed?

💬 hebasto commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#discussion_r1457712659)
To fix the MSVC CI job, suggesting:

```suggestion
} catch (const std::exception&) {
```

💬 eragmus commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898833448)
Concept NACK.

> Since CVE ID is used to validate this as a [vulnerability](https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1848850110), I wanted to share that [cve.org](https://cve.org) has added "disputed" tag for CVE-2023-50428. This tag is used when there are differences of opinion about whether its a vulnerability based on the CVE program's definition.
>
>
>
> 
>
>
>
> A note h
...

💬 real-or-random commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1457717119)
Shouldn't this be covered by https://github.com/hebasto/bitcoin/blob/e39bae122c79b8dfaa5f7e02ff199dc8c2051d8a/test/sanitizer_suppressions/ubsan#L22-L28 already?

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457717925)
Added a comment above.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457717961)
Aaargh, afaik Github CI doesn't support custom ENV variables for forks. So this is solved, but on Github forks are unconditionally skipped.

💬 hebasto commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1457718743)
It expects exactly "implicit-signed-integer-truncation,implicit-integer-sign-change".

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457666682)
Seems better to just require it in the other deps above (next to `screen`)?

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457668127)
```suggestion
- git --version || bash -c "$PACKAGE_MANAGER_INSTALL git"
```

Maybe without a config flag?

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457721848)
But that doesn't help if two tasks are run at the same time for the same user

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457728654)
I don't think you're supposed to do that. One user == one worker.

If we want to support multiple workers per user, we also have to randomise the container names, since you might be running CI for two pull requests at the same time.

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457728802)
For reference, I said to use `CONTAINER_NAME` in the pull request you closed, which should be a more complete and correct fix?

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457730915)
Oh, I assume it that explicit install was there for a reason, the comment "Unconditionally install git (used in fingerprint_script)." is a bit vague.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457732083)
That's indeed nicer, if the line can't be dropped entirely.

💬 petertodd commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1898859416)
> I may be missing something here, can you describe in more details how _anyone_ could turn them into large pinning txs? They do require signatures from _both_ participants, and one of them is going to be `SIGHASH_ALL`.

Thanks. Yes, that's a mistake on my part and you are correct. I hide my comment as "outdated" to reflect that.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457735631)
I missed that.

But the file is owned by the user. In a multi-user setup each user builds its own containers. So this would break afaik. So random UUID could work though.

💬 real-or-random commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1457736462)
I see. I think it still makes sense to move the line up so that it appears below `# Suppressions in dependencies that are developed outside this repository.`

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457736806)
> Could be a separate pull request, because this is a bug fix?

I might split it out if this PR takes too long on its own.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457739319)
Preserving @maflcko's comment on the previous PR:

> unrelated: Generally I'd find it better if we get rid of the commit range completely. For example, when running the whitespace linter, it seems easier to just lint all code at `HEAD`, instead of trying to guess which code was modified and only lint that.

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457739745)
When running locally, usually only one user exists, and someone may be interested in running *different* tasks for the same (local) code.

Running the *same* task in parallel is indeed not supported right now.