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

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

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

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

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

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

👍 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

💬 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)")

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

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

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

💬 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

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

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

💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941983133)
I think it is fine. It is the short version of "The span objects must be 32 bytes each."

💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941986868)
I find the suggestions helpful, I'm fine with moving them to doc separately.
The `span's` should likely be adjusted here as well, maybe to `instances of std::span` or something similar.

💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941987358)
Thanks, please resolve it

💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2635223309)
> Rebased. The commit messages have been amended.
>
> @theuni
>
> > Wait, I take that back. It seems like we could still get 2 separate `cs_main`s here: one in the lib and the other in the binary. ACK retracted while I investigate.
>
> That's an interesting observation. Could you elaborate on how you think the two separate `cs_main` instances might arise?
>
> If there's a relevant section in the documentation or standard that you believe applies, pointing it out would be helpful for
...

💬 theuni commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1941993667)
This looks like a good use-case for a static extent :)

(as a follow-up)

💬 maflcko commented on issue "bitcoind debug log file is growing without limit":
(https://github.com/bitcoin/bitcoin/issues/31795#issuecomment-2635297607)


https://bitcoin.stackexchange.com/questions/18857/how-to-limit-the-debug-log-file-while-running-bitcoind

Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.

General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.