💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1941751254)
Fixed!
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1941751254)
Fixed!
💬 hebasto commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2634831978)
> ... then found another issue:
>
> ```
> /home/runner/work/_temp/src/ipc/libmultiprocess/src/mp/gen.cpp:633:26: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
> 633 | for (size_t i = 4; i < argc; ++i) {
> | ~ ^ ~~~~
> 1 error generated.
> ```
Fixed in https://github.com/chaincodelabs/libmultiprocess/pull/151.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2634831978)
> ... then found another issue:
>
> ```
> /home/runner/work/_temp/src/ipc/libmultiprocess/src/mp/gen.cpp:633:26: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
> 633 | for (size_t i = 4; i < argc; ++i) {
> | ~ ^ ~~~~
> 1 error generated.
> ```
Fixed in https://github.com/chaincodelabs/libmultiprocess/pull/151.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2634860548)
Force-pushed from 6709e185b93d97b9191597acfecb25c33ab43a0a to 386eecff5f14d508688e6e7374b67cb54aaa7249.
See [6709e185b9....386eecff5f14](https://github.com/bitcoin/bitcoin/compare/6709e185b93d97b9191597acfecb25c33ab43a0a..386eecff5f14d508688e6e7374b67cb54aaa7249)
- The changes in this push ensure that the consensus `MAX_BLOCK_WEIGHT` is used in block assembler instead of a policy constant `DEFAULT_BLOCK_MAX_WEIGHT`.
- The policy value is only used when the user has not provided a cus
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2634860548)
Force-pushed from 6709e185b93d97b9191597acfecb25c33ab43a0a to 386eecff5f14d508688e6e7374b67cb54aaa7249.
See [6709e185b9....386eecff5f14](https://github.com/bitcoin/bitcoin/compare/6709e185b93d97b9191597acfecb25c33ab43a0a..386eecff5f14d508688e6e7374b67cb54aaa7249)
- The changes in this push ensure that the consensus `MAX_BLOCK_WEIGHT` is used in block assembler instead of a policy constant `DEFAULT_BLOCK_MAX_WEIGHT`.
- The policy value is only used when the user has not provided a cus
...
💬 willcl-ark commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2634905893)
I seem to have a problem running IBD with this change @ dd2714ed13eea3591162dbade8132ff517ca5509, which I have not investigated yet:
<details>
<summary>tail debug.log</summary>
```log
<snip>
2025-02-04T19:38:24Z init message: Verifying blocks…
2025-02-04T19:38:24Z Block index and chainstate loaded
2025-02-04T19:38:24Z Setting NODE_NETWORK on non-prune mode
2025-02-04T19:38:24Z initload thread start
2025-02-04T19:38:24Z [bench] - Load block from disk: 0.05ms
2025-02-04T19:38:24Z [
...
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2634905893)
I seem to have a problem running IBD with this change @ dd2714ed13eea3591162dbade8132ff517ca5509, which I have not investigated yet:
<details>
<summary>tail debug.log</summary>
```log
<snip>
2025-02-04T19:38:24Z init message: Verifying blocks…
2025-02-04T19:38:24Z Block index and chainstate loaded
2025-02-04T19:38:24Z Setting NODE_NETWORK on non-prune mode
2025-02-04T19:38:24Z initload thread start
2025-02-04T19:38:24Z [bench] - Load block from disk: 0.05ms
2025-02-04T19:38:24Z [
...
💬 ryanofsky commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2634917217)
I thought I tracked down the problem, and some change I made to the test caused it to pass, but rerunning the the fix in a new container, it no longer works, so I have to go back.
Here were notes I was about to post about potential problem & fix. In any case I suspect problem would be avoided if we just consistently used gcc or clang for this build and didn't try to mix them.
---
Test failure seems to be caused by the container using incompatible ABI versions for the depends build and the mai
...
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2634917217)
I thought I tracked down the problem, and some change I made to the test caused it to pass, but rerunning the the fix in a new container, it no longer works, so I have to go back.
Here were notes I was about to post about potential problem & fix. In any case I suspect problem would be avoided if we just consistently used gcc or clang for this build and didn't try to mix them.
---
Test failure seems to be caused by the container using incompatible ABI versions for the depends build and the mai
...
💬 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.