💬 furszy commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2634957076)
Test failure is unrelated.
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2634957076)
Test failure is unrelated.
💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2634970443)
Squashed.
> Just as a note, there's also `test/functional/feature_taproot.py` which produces [taproot unit tests](https://raw.githubusercontent.com/bitcoin-core/qa-assets/refs/heads/main/unit_test_data/script_assets_test.json). I'm not sure if those were meant to be extended in the case of Tapscript updates but that seems a bit daunting anyway compared to what you suggest here.
Yes, the idea here is to have the nice script unittest tests work for Tapscript. These tests are much more lightw
...
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2634970443)
Squashed.
> Just as a note, there's also `test/functional/feature_taproot.py` which produces [taproot unit tests](https://raw.githubusercontent.com/bitcoin-core/qa-assets/refs/heads/main/unit_test_data/script_assets_test.json). I'm not sure if those were meant to be extended in the case of Tapscript updates but that seems a bit daunting anyway compared to what you suggest here.
Yes, the idea here is to have the nice script unittest tests work for Tapscript. These tests are much more lightw
...
💬 maflcko commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2634991591)
Interesting that this did not result in a compile or link failure instead.
I switched to clang in commit fad0f21c3caba129106799fe6c14aff323ef99f2 to avoid OOM with g++, (which was before switching to 32-bit in commit fae0295a799499268caca9c385ac4d7061543980), but now that the CI machines have more memory, it should be fine if this CI task takes more memory and time.
Happy to review a pull, if someone submits one.
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2634991591)
Interesting that this did not result in a compile or link failure instead.
I switched to clang in commit fad0f21c3caba129106799fe6c14aff323ef99f2 to avoid OOM with g++, (which was before switching to 32-bit in commit fae0295a799499268caca9c385ac4d7061543980), but now that the CI machines have more memory, it should be fine if this CI task takes more memory and time.
Happy to review a pull, if someone submits one.
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2635037422)
> C.I failure seems to be unrelated.
It does indeed but so far 5 jobs have failed so it might be better to kick the CI since it could be that the unrelated (early) fail masks an actual issue in one of them. I've just asked for this on IRC.
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2635037422)
> C.I failure seems to be unrelated.
It does indeed but so far 5 jobs have failed so it might be better to kick the CI since it could be that the unrelated (early) fail masks an actual issue in one of them. I've just asked for this on IRC.
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941880894)
Previously it was possible to make a mutable uchar span, so I wanted to preserve the feature with the new (unused) helper.
Before:
```cpp
std::vector<std::byte> mut;
Span<uint8_t> mut_sp{MakeUCharSpan(mut)};
```
After:
```cpp
std::vector<std::byte> mut;
std::span<uint8_t> mut_sp{MakeWritableUCharSpan(mut)};
```
But it seems fine to remove, given that it is unused and one could use `UCharSpanCast` on a span that points to mutable data.
Howev
...
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941880894)
Previously it was possible to make a mutable uchar span, so I wanted to preserve the feature with the new (unused) helper.
Before:
```cpp
std::vector<std::byte> mut;
Span<uint8_t> mut_sp{MakeUCharSpan(mut)};
```
After:
```cpp
std::vector<std::byte> mut;
std::span<uint8_t> mut_sp{MakeWritableUCharSpan(mut)};
```
But it seems fine to remove, given that it is unused and one could use `UCharSpanCast` on a span that points to mutable data.
Howev
...
⚠️ ismaelsadeeq opened an issue: "CI: Failed pulls from docker.io causing jobs to fail"
(https://github.com/bitcoin/bitcoin/issues/31797)
```terminal
[15:50:52.950] STEP 1/7: FROM docker.io/debian:bookworm
[15:50:52.956] Trying to pull docker.io/library/debian:bookworm...
[15:51:23.200] time="2025-02-04T15:51:23-05:00" level=warning msg="Failed, retrying in 2s ... (1/3). Error: initializing source docker://debian:bookworm: reading manifest bookworm in docker.io/library/debian: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-lim
...
(https://github.com/bitcoin/bitcoin/issues/31797)
```terminal
[15:50:52.950] STEP 1/7: FROM docker.io/debian:bookworm
[15:50:52.956] Trying to pull docker.io/library/debian:bookworm...
[15:51:23.200] time="2025-02-04T15:51:23-05:00" level=warning msg="Failed, retrying in 2s ... (1/3). Error: initializing source docker://debian:bookworm: reading manifest bookworm in docker.io/library/debian: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-lim
...
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941900848)
I think it is clearer to keep span-serialization documented here. Previously, the data was contained in a vector, which made this span constructor required (`std::vector<uint8_t> block413567`).
This changed in commit faecca9a85c1c1917d024f55cca34d19cc94f3b9, which forgot to update this comment. So I think a separate commit could update the comment here.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941900848)
I think it is clearer to keep span-serialization documented here. Previously, the data was contained in a vector, which made this span constructor required (`std::vector<uint8_t> block413567`).
This changed in commit faecca9a85c1c1917d024f55cca34d19cc94f3b9, which forgot to update this comment. So I think a separate commit could update the comment here.
💬 theuni commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941913821)
No preference, whatever's easiest here.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941913821)
No preference, whatever's easiest here.
💬 maflcko commented on issue "CI: Failed pulls from docker.io causing jobs to fail":
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2635114974)
This should only happen when a new ci image build is triggered without the images being cached from previous builds. So it should be rare enough to not happen often. When it happens, it should clear itself after an hour or two.
An alternative would be to use (or run) a mirror that has a higher rate limit, but I am not sure if this is worth it.
I've re-run the CI tasks for now.
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2635114974)
This should only happen when a new ci image build is triggered without the images being cached from previous builds. So it should be rare enough to not happen often. When it happens, it should clear itself after an hour or two.
An alternative would be to use (or run) a mirror that has a higher rate limit, but I am not sure if this is worth it.
I've re-run the CI tasks for now.
💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2635116588)
Can confirm switching from clang to gcc does seem to fix this. `-Wno-error=documentation` also had to be dropped because gcc does not support it.
```c++
--- a/ci/test/00_setup_env_i686_multiprocess.sh
+++ b/ci/test/00_setup_env_i686_multiprocess.sh
@@ -10,15 +10,12 @@ export HOST=i686-pc-linux-gnu
export CONTAINER_NAME=ci_i686_multiprocess
export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04"
export CI_IMAGE_PLATFORM="linux/amd64"
-export PACKAGES="llvm clang g++-multilib"
-export DEP_OPTS="DEBU
...
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2635116588)
Can confirm switching from clang to gcc does seem to fix this. `-Wno-error=documentation` also had to be dropped because gcc does not support it.
```c++
--- a/ci/test/00_setup_env_i686_multiprocess.sh
+++ b/ci/test/00_setup_env_i686_multiprocess.sh
@@ -10,15 +10,12 @@ export HOST=i686-pc-linux-gnu
export CONTAINER_NAME=ci_i686_multiprocess
export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04"
export CI_IMAGE_PLATFORM="linux/amd64"
-export PACKAGES="llvm clang g++-multilib"
-export DEP_OPTS="DEBU
...
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2635118110)
Rebased and added one doc-only commit to address all feedback.
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2635118110)
Rebased and added one doc-only commit to address all feedback.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941956974)
> Previously, the data was contained in a vector
A span is generated now:
```C++
inline constexpr std::span block413567{detail_block413567_raw};
```
So I don't see the reason for wrapping it in another span now.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941956974)
> Previously, the data was contained in a vector
A span is generated now:
```C++
inline constexpr std::span block413567{detail_block413567_raw};
```
So I don't see the reason for wrapping it in another span now.
👍 theuni approved a pull request: "depends: Avoid hardcoding `host_prefix` in toolchain file"
(https://github.com/bitcoin/bitcoin/pull/31358#pullrequestreview-2594173161)
Neat. utACK d9c8aacce38ab593ea9277976eb64ccadd7d062f
(https://github.com/bitcoin/bitcoin/pull/31358#pullrequestreview-2594173161)
Neat. utACK d9c8aacce38ab593ea9277976eb64ccadd7d062f
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941961333)
Done, you can resolve it (except that "[As a general rule, we never use an apostrophe in writing plural forms](https://www.sussex.ac.uk/informatics/punctuation/apostrophe/plurals)")
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941961333)
Done, you can resolve it (except that "[As a general rule, we never use an apostrophe in writing plural forms](https://www.sussex.ac.uk/informatics/punctuation/apostrophe/plurals)")
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941977560)
Not sure how to express it better, but I am happy to push any diff.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941977560)
Not sure how to express it better, but I am happy to push any diff.
🤔 l0rinc reviewed a pull request: "refactor: Use std::span over Span"
(https://github.com/bitcoin/bitcoin/pull/31519#pullrequestreview-2594178508)
Docker seems to complain with:
> You have reached your pull rate limit
But locally all tests were passing for me.
I think this is an important change, I'd just a like a few minor changes (like remaining "Spans" references), and I'd prefer doing the unrelated header changes in a different PR for transparency.
(https://github.com/bitcoin/bitcoin/pull/31519#pullrequestreview-2594178508)
Docker seems to complain with:
> You have reached your pull rate limit
But locally all tests were passing for me.
I think this is an important change, I'd just a like a few minor changes (like remaining "Spans" references), and I'd prefer doing the unrelated header changes in a different PR for transparency.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941963898)
shouldn't these go to the [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?plain=1#L859-L863) now? We don't have files for other std types locally which explain their behavior.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941963898)
shouldn't these go to the [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md?plain=1#L859-L863) now? We don't have files for other std types locally which explain their behavior.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941965691)
While I find it a good idea to finally get rid of these, I think it may be cleaner to do these in a separate PR, this one's quite heavy already
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941965691)
While I find it a good idea to finally get rid of these, I think it may be cleaner to do these in a separate PR, this one's quite heavy already
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941982026)
I can see it both ways. The `span.h` will remain to hold `std::span` helpers, so it seems fine to keep it there. It is also fine to move it the dev notes, but this should be a separate commit, to make review easier. Finally, it is also fine to remove it, as the std lib has documentation and doesn't need additional ones in this repo?
I don't really mind either way and I think anything is fine, so I just try to make it as easy to review for reviewers.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941982026)
I can see it both ways. The `span.h` will remain to hold `std::span` helpers, so it seems fine to keep it there. It is also fine to move it the dev notes, but this should be a separate commit, to make review easier. Finally, it is also fine to remove it, as the std lib has documentation and doesn't need additional ones in this repo?
I don't really mind either way and I think anything is fine, so I just try to make it as easy to review for reviewers.
💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941982796)
Maybe something like:
```suggestion
// don't work. This makes it possible to use the std::span constructor in a SFINAE
// contexts like in the Spannable function above to detect whether types are or
// aren't compatible with std::span at compile time.
```
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941982796)
Maybe something like:
```suggestion
// don't work. This makes it possible to use the std::span constructor in a SFINAE
// contexts like in the Spannable function above to detect whether types are or
// aren't compatible with std::span at compile time.
```