💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1616978938)
Does it make sense to skip this call if `std::is_trivially_destructible_v<T>`?

💬 Sjors commented on pull request "ci: Reintroduce fixed "test-each-commit" job":
(https://github.com/bitcoin/bitcoin/pull/28497#discussion_r1616980946)
@ryanofsky fixed a simpler case of this in #28573, where PR B has 1 commit. Though I'm confused why he ran into it while #28201 doesn't.

💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1616990466)
This was tried: https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491140321

Here is a draft that changes to `std::string_view` the following:

* `NetMsgType::*`
* `ALL_NET_MESSAGE_TYPES`
* `CMessageHeader:pchCommand` (renamed to `CMessageHeader:m_command`)
* `CNetMessage::m_type`
* `V2_MESSAGE_IDS`
* `V2MessageMap::m_map`
* `V2Transport::m_send_type`
and the functions that handle message types.

The scope of this can be reduced by converting the `std::string_view` to `std:
...

👍 stickies-v approved a pull request: "[27.x] Backports and rc1"
(https://github.com/bitcoin/bitcoin/pull/30092#pullrequestreview-2082377630)
re-ACK f30e23e4b35b52728ebaa1a63ec9a6feac51ebb4, I'm getting the same translation update results as in 80032d691702f1e5eccb27571066bfc9b2415742

💬 stickies-v commented on pull request "[27.x] Backports and rc1":
(https://github.com/bitcoin/bitcoin/pull/30092#discussion_r1616987943)
nit but text is not updated to "271.rc1" in latest force push (link is fine now)

💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1616992405)
Both functions `clear()` and `ResizeDown()` could be declared `noexcept`.

💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1617005071)
The move assignment operator preserves the capacity. Maybe do the same here for consistency?

💬 Sjors commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2134950628)
Rebased now that #29612 landed. Will do some final testing before marking this ready to review.

👍 vasild approved a pull request: "build, test, doc: Temporarily remove Android-related stuff"
(https://github.com/bitcoin/bitcoin/pull/30049#pullrequestreview-2082526292)
ACK 5deb0b024e14c7c63d405c651d1ca323560a1c21

Here are the remaining mentions of "android" in non-source code files:

```
.github/workflows/ci.yml:138: CI_QT_CONF: '-release -silent -opensource -confirm-license -opengl desktop -static -static-runtime -mp -qt-zlib -qt-pcre -qt-libpng -nomake examples -nomake tests -nomake tools -no-angle -no-dbus -no-gif -no-gtk -no-ico -no-icu -no-libjpeg -no-libudev -no-sql-sqlite -no-sql-odbc -no-sqlite -no-vulkan -skip qt3d -skip qtactiveqt -skip qt
...

💬 ryanofsky commented on pull request "ci: Reintroduce fixed "test-each-commit" job":
(https://github.com/bitcoin/bitcoin/pull/28497#discussion_r1617080675)
> @ryanofsky fixed a simpler case of this in #28573, where PR B has 1 commit. Though I'm confused why he ran into it while #28201 doesn't.

I think there shouldn't be a problem here because even if a pull request is based on another pull request, the `github.event.pull_request.commits` number will include the number of commits in both pull requests, because github treats pull requests as being independent and isn't aware of relationships between them.

The case I was fixing in https://github
...

💬 cbergqvist commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1613223825)
nit: Superfluous `I()`

💬 cbergqvist commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1617040989)
nit: `ptr` could be passed as a reference to signify non-nullness. (But the member still needs to be a pointer). Not at all significant as it is a private function.

💬 cbergqvist commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1613105680)
Have you considered using `__int128`, `__int256` etc when available, before falling back to `MultiIntBitSet`? Maybe something for a follow-up PR.

🤔 cbergqvist reviewed a pull request: "util: add BitSet"
(https://github.com/bitcoin/bitcoin/pull/30160#pullrequestreview-2076257466)
Looked at 3bc131a85ce826aa171f0328c014ea51b8740fed

Concept: Bummer that `std::countr_zero` and `std::countl_zero` aren't implemented for `std::bitset`. And for iterating it seems like one would be using `std::vector<bool>` and hoping for the best in terms of implementation. So makes sense to introduce `BitSet`.

Summary still mentions `operator/` instead of `operator-`.

(Fuzz-code is too new to me to comment).

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1617110168)
In commit "ci: increase fetch depth for 'test each commit'" (6398efbe145fb297957060600615262d59f2b47c)

I think the following would be a more direct fix. I didn't test it but it just omits the `^${MERGE_BASE}^@` argument when there is no merge base, which should be fine because there should be no need to exclude merge base ancestors in that case:

```diff
- echo "TEST_BASE=$(git rev-list -n$((${{ env.MAX_COUNT }} + 1)) --reverse HEAD ^$(git rev-list -n1 --merges HEAD)^@ | head -1)"
...

💬 cbergqvist commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1617135964)
Appears to be unused? Compiled without it on Clang.

💬 cbergqvist commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1617126230)
*`FUZZ_TARGET(vecdeque)`

🤔 theStack reviewed a pull request: "util: add BitSet"
(https://github.com/bitcoin/bitcoin/pull/30160#pullrequestreview-2082445138)
Concept ACK

💬 theStack commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1617026499)
It's a bit scary that `MultiIntBitSet::{First,Last}` lead to buffer over/under-reads if no bit is set. I understand that we wouldn't want to do assert for `Any` in production for performance reasons, but it's maybe worth it do it for debug builds, e.g. with somethink like the `ASSERT_IF_DEBUG` construct used in src/span.h.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1617148751)
I have not benchmarked this, but I have a hard time imagining how that can be faster, as common hardware doesn't have general-purpose registers bigger than `size_t`, so `__int128` operations get simulated using multiple variables anyway. Some architectures do have vector extensions (SSE, AVX, ...) which introduce native 128-, 256-, or 512-bit registers, but those generally only support operations of units up to 32 to 64 bits in them (including leading-zero counting, or popcount, which we need he
...