💬 dergoegge commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457572196)
Why is it ok to remove this assertion?
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457572196)
Why is it ok to remove this assertion?
💬 dergoegge commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457573883)
Would be cool if we could use `infoAll` since that doesn't require the lock externally but we need the sequence to be in sync so we can't, oh well...
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457573883)
Would be cool if we could use `infoAll` since that doesn't require the lock externally but we need the sequence to be in sync so we can't, oh well...
💬 glozow commented on pull request "[26.x] more backports":
(https://github.com/bitcoin/bitcoin/pull/29209#issuecomment-1898664679)
fixed commit messages.
(https://github.com/bitcoin/bitcoin/pull/29209#issuecomment-1898664679)
fixed commit messages.
💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1898668856)
> However it made no sense to me to store a `CKey` as plain text
I guess that by "plain text" here you mean `std::string`, right? `std::string` can store arbitrary non-ASCII characters, including `'\0'`, so it is technically ok to use it for binary data.
More relevant in this case is that `CKey` stores sensitive data and takes care to wipe it from memory when freed. In https://github.com/bitcoin/bitcoin/pull/28983 `Read/WriteBinaryData()` is used in a way that defeats that - the sensitive
...
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1898668856)
> However it made no sense to me to store a `CKey` as plain text
I guess that by "plain text" here you mean `std::string`, right? `std::string` can store arbitrary non-ASCII characters, including `'\0'`, so it is technically ok to use it for binary data.
More relevant in this case is that `CKey` stores sensitive data and takes care to wipe it from memory when freed. In https://github.com/bitcoin/bitcoin/pull/28983 `Read/WriteBinaryData()` is used in a way that defeats that - the sensitive
...
💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457586467)
Is this needed? Won't this be skipped already by default if the worker is offline?
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457586467)
Is this needed? Won't this be skipped already by default if the worker is offline?
💬 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).
(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
```
(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.
(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, ...)
(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};
(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.
(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.
(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.
(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.
(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
...
(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
...
(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
...
(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?
(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&) {
```
(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
...
(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
...