💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1457418125)
nit: in the commit message:

> ReadBinaryFile and WriteBinaryFile current work with std::string

s/current/currently/

Would be nice to wrap lines in the commit message at [72 columns](https://duckduckgo.com/?q=commit+message+50%2F72).

💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1457430349)
nit:
```suggestion
* @return result if successful, std::nullopt otherwise
```

💬 glozow commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457586628)
maybe replace this with a call to something else and keep the test. The assertion is just checking that the size has shrunk after removing something.

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457587805)
No objection, but I presume this will cause issues on forks (Inquisition, Knots, ...)

💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1898681016)
> Can we change `Read/WriteBinaryData()` to operate directly on `CKey` in such a way that data goes directly from `CKey` to the disk?

I haven't looked in detail, but writing bytes to a file can be achieved with one line of code:

```cpp
CAutoFile{...} << Span{data};

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457612699)
It stays pending, at least in the Cirrus interface.

💬 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`)?