Bitcoin Core Github
44 subscribers
120K links
Download Telegram
💬 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.
🤔 fjahr reviewed a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2594288643)
I left comments there but I don't want to bikeshed the release notes too much, so feel free to ignore them unless you have to retouch or don't want to address them.
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942035850)
nit

```suggestion
Before this pull request, the policy default for the maximum block weight was `3,996,000 WU`, calculated by
```
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1942039833)
Shouldn't this now be this?

```suggestion
const auto block_adjusted_max_weight = MAX_BLOCK_WEIGHT - MINIMUM_BLOCK_RESERVED_WEIGHT;
```