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

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1617153576)
I don't believe this is superfluous. If `I` is smaller than an `int` (e.g. `I = uint16_t`) then `~I{0}` undergoes [integer promotion](https://en.cppreference.com/w/c/language/conversion) to `int`, and right-shifting that would sign-extend, which would incorrect here. The `I(...)` around it forces it to be unsigned before the right-shift.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1617175155)
I've added `Assume()`s in `First()` and `Last()` to avoid this.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1617175296)
Done.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2135121750)
FWIW, i'm not seeing that problem on TurrisOS (openwrt) with miniupnpd 2.3.3. Renewals work fine, both with 20 minute and 5 minute reannounce period. So it's either something that was solved in the meantime, or something different in your networking situation.

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1617187166)
Fixed.

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1617189498)
It's an invariant, but it was poorly described, which I've hopefully addressed now (see `m_offset` docstring).

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1617190116)
I don't think so, since that case doesn't let us avoid the loop.

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1617190282)
Good catch, done.

💬 cbergqvist commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1617190868)
I'd consider SSE supporting CPUs quite common. Seems like using such instructions can offer efficiency gains but are sufficiently complex to write papers about - https://stackoverflow.com/a/42675620, something for the future maybe.

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1617193263)
I think it's better to match `std::vector` behavior here (technically, spec doesn't say anything about the capacity of a copied element, but common implementations make it just big enough to hold the copied data, I believe).

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1617193395)
Fixed.

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1617197710)
Fixed!

💬 Sjors commented on pull request "doc, rpc: Release notes and follow-ups for #29612":
(https://github.com/bitcoin/bitcoin/pull/30167#issuecomment-2135163999)
I checked that efc1b5be8a4696c0db19ba18316b2d4ed09e10f2 still gives me the same snapshot as the last the time I reviewed #29612.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1617216061)
Thanks, will try.

💬 Sjors commented on pull request "ci: Reintroduce fixed "test-each-commit" job":
(https://github.com/bitcoin/bitcoin/pull/28497#discussion_r1617224001)
> because github treats pull requests as being independent and isn't aware of relationships between them.

On the fork I tested on the pull request wasn't against `master` but against branch `A`.