💬 dergoegge commented on pull request "Revert "build: Fix regression in "ARMv8 CRC32 intrinsics" test"":
(https://github.com/bitcoin/bitcoin/pull/29226#issuecomment-1898634211)
Added documentation about the potential bug

💬 t-bast commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1898641197)
Thanks for the clarification @ariard, I think I understand your scenario better. But there's one point I don't understand: in step 6 (`Alice cannot over-pay in absolute fees to replace Mallory state due to "max at stake" 440k sats`), Alice doesn't want to replace Mallory's revoked commitment, she wants it to confirm to spend all the output to herself?

It's easy for Alice to make the revoked commitment confirm, she can use any output of that transaction to CPFP and she doesn't need to add any
...

📝 maflcko opened a pull request: "refactor: Fix prevector iterator concept issues"
(https://github.com/bitcoin/bitcoin/pull/29275)
Currently prevector iterators have many issues:

* Forward iterators (and stronger) must be default constructible (https://eel.is/c++draft/forward.iterators#1.2). Otherwise, some functions can not be instantiated, like `std::minmax_element`.
* Various `const` issues with random access iterators. For example, a `const iterator` is different from a `const_iterator`, because the first one holds a mutable reference and must also return it without `const`. Also, `operator+` must be callable regard
...

💬 Xaspr commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1898650298)
I had to stop the sync for a while due to traveling and now starting it up again. I'm now at block height 720.000 without issues.

The CPU temperature is pretty high, with one core going up to a peak of 95 degrees Celsius. Most cores go to max 80 Celsius.

What I've tried to get it working up to now:

- Reinstalled v26.0.0.
- Deleted antivirus software Bitdefender.
- Enabled Micros
...

🤔 dergoegge reviewed a pull request: "refactor: remove CTxMemPool::queryHashes()"
(https://github.com/bitcoin/bitcoin/pull/29260#pullrequestreview-1829896476)

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

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

💬 glozow commented on pull request "[26.x] more backports":
(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
...

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

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