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