💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457616250)
Good point, this probably needs an ENV flag.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457621988)
Knots pull requests seem to be typically made against the `25.x-knots` branch. Those would run normally. But with the above code CI would not run when e.g. a future `26.x.knots` is created (or rebased).

Inquisition seems to follow the some pattern.

💬 theuni commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-1898731456)
Seems the ctz builtins here can be switched to c++20's `countr_zero` ?
See the libc++ [here](https://github.com/llvm/llvm-project/blob/main/libcxx/include/__bit/countr.h#L41), for an example of how it maps to builtins.

🤔 pablomartin4btc reviewed a pull request: "test: Remove all-lint.py script"
(https://github.com/bitcoin/bitcoin/pull/29228#pullrequestreview-1830015256)
tACK fa2b95cf3f5148d27a8fd4fb3763ca1fc139bdd9

<details>
<summary>I also used to run the <code>all-int.py</code> but I've tried the docker <code>linter</code> locally following the instructions from the link provided in the description of the PR and I'll use that going forward.</summary>


```

DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter
[+] Building 169.7s (12/12) FINISHED
...

💬 edilmedeiros commented on issue "build: multiprocess compile failure on macOS":
(https://github.com/bitcoin/bitcoin/issues/29248#issuecomment-1898755858)
I confirm this. I installed libmutiprocess manually; I'm not using what's on `depends`, but it generates the same error.

Moreover, I tried on the current master head (03c5b006) and build fails in more places.

```
CXX ipc/capnp/libbitcoin_ipc_a-echo.capnp.o
CXX ipc/capnp/libbitcoin_ipc_a-init.capnp.o
CXX ipc/capnp/libbitcoin_ipc_a-echo.capnp.proxy-client.o
CXX ipc/capnp/libbitcoin_ipc_a-init.capnp.proxy-client.o
CXX ipc/capnp/libbitcoin_ipc_a-echo.cap
...

💬 eragmus commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898759099)
Concept NACK.

Bitcoin is a value-transfer system across space and time.

Value is subjective.

Value, in Bitcoin, is subjectively determined by the fee-rate of the tx.

Bitcoin's anti-DoS mechanism is not about Bitcoin Core sending political messages to dictate to the economy what it should or should not do.

Bitcoin's anti-DoS mechanism is based on fee-rate of txs.

Therefore, there should be no prejudice on the mempool layer, as far as which kind of value is "good" or "bad", due to Bitcoin's
...

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