👍 TheCharlatan approved a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2527313983)
Re-ACK faf7eac364fb7f421a649b483286ac8681d92b31
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2527313983)
Re-ACK faf7eac364fb7f421a649b483286ac8681d92b31
💬 l0rinc commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900726674)
The title clams that only `Reads` were changed - but since we have adapted the writes as well, would it make sense to change the writes in the example `ChaCha` as well to demonstrate that they're correct?
```patch
diff --git a/src/crypto/chacha20.cpp b/src/crypto/chacha20.cpp
--- a/src/crypto/chacha20.cpp (revision fa83bec78ef3e86445e522afa396c97b58eb1902)
+++ b/src/crypto/chacha20.cpp (date 1735812552447)
@@ -59,7 +59,7 @@
inline void ChaCha20Aligned::Keystream(Span<std::byte> outpu
...
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900726674)
The title clams that only `Reads` were changed - but since we have adapted the writes as well, would it make sense to change the writes in the example `ChaCha` as well to demonstrate that they're correct?
```patch
diff --git a/src/crypto/chacha20.cpp b/src/crypto/chacha20.cpp
--- a/src/crypto/chacha20.cpp (revision fa83bec78ef3e86445e522afa396c97b58eb1902)
+++ b/src/crypto/chacha20.cpp (date 1735812552447)
@@ -59,7 +59,7 @@
inline void ChaCha20Aligned::Keystream(Span<std::byte> outpu
...
💬 kelvinator07 commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2567569305)
@adyshimony, thank you for your helpful response! I was able to resolve the issue using the following steps:
1. Installed LLVM via Homebrew `brew install llvm`
2. Clean build / delete dir to clean cache via `git clean -fxd`
3. Updated the build command to use the Homebrew-installed LLVM path, and also adding the LLVM library path:
```
cmake -DCMAKE_C_COMPILER=/opt/homebrew/opt/llvm/bin/clang \
-DCMAKE_CXX_COMPILER=/opt/homebrew/opt/llvm/bin/clang++ \
-DCMAKE_EXE_LINKER_FLAG
...
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2567569305)
@adyshimony, thank you for your helpful response! I was able to resolve the issue using the following steps:
1. Installed LLVM via Homebrew `brew install llvm`
2. Clean build / delete dir to clean cache via `git clean -fxd`
3. Updated the build command to use the Homebrew-installed LLVM path, and also adding the LLVM library path:
```
cmake -DCMAKE_C_COMPILER=/opt/homebrew/opt/llvm/bin/clang \
-DCMAKE_CXX_COMPILER=/opt/homebrew/opt/llvm/bin/clang++ \
-DCMAKE_EXE_LINKER_FLAG
...
👍 hebasto approved a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2527366864)
Re-ACK faf7eac364fb7f421a649b483286ac8681d92b31. The commit, which modifies CMake scripts, has been replaced with the one from https://github.com/bitcoin/bitcoin/pull/31547, and a formatting commit has been added since my recent [review](https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517189261).
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2527366864)
Re-ACK faf7eac364fb7f421a649b483286ac8681d92b31. The commit, which modifies CMake scripts, has been replaced with the one from https://github.com/bitcoin/bitcoin/pull/31547, and a formatting commit has been added since my recent [review](https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517189261).
🤔 fjahr reviewed a pull request: "test: raise explicit error if any of the needed release binaries is missing"
(https://github.com/bitcoin/bitcoin/pull/31462#pullrequestreview-2526930619)
tACK e8795f4c11622c26cf303b759ba717351a8fba45
Would be good to fix the comment but it's not a big deal.
Here is the output I see, I think it would great if the backtrace could be suppressed in this case because it's mostly noise distracting from the helpful logs. But it might be too much hassle, so not a must.
<details>
<summary>Output</summary>
```
2025-01-01T21:24:42.885000Z TestFramework (INFO): Initializing test directory /var/folders/9z/n7rz_6cj3bq__11k5kcrsvvm0000gn/T/bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/31462#pullrequestreview-2526930619)
tACK e8795f4c11622c26cf303b759ba717351a8fba45
Would be good to fix the comment but it's not a big deal.
Here is the output I see, I think it would great if the backtrace could be suppressed in this case because it's mostly noise distracting from the helpful logs. But it might be too much hassle, so not a must.
<details>
<summary>Output</summary>
```
2025-01-01T21:24:42.885000Z TestFramework (INFO): Initializing test directory /var/folders/9z/n7rz_6cj3bq__11k5kcrsvvm0000gn/T/bitcoin
...
💬 fjahr commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1900462956)
``` suggestion
# Fail test if any of the needed release binaries is missing
```
(https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1900462956)
``` suggestion
# Fail test if any of the needed release binaries is missing
```
💬 fjahr commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1900752082)
nit: Could also been put into the assert message IMO
``` suggestion
raise AssertionError("At least one release binary is missing. Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.")
```
(https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1900752082)
nit: Could also been put into the assert message IMO
``` suggestion
raise AssertionError("At least one release binary is missing. Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.")
```
💬 l0rinc commented on pull request "Don't wipe coins cache when full and instead evict LRU clean entries":
(https://github.com/bitcoin/bitcoin/pull/31102#issuecomment-2567634389)
> I think https://github.com/bitcoin/bitcoin/pull/31132 is a better approach, because cache misses are fetched in parallel much faster so the misses are not as important.
I see the two changes as orthogonal - the main goal of this change as far as I understand is to avoid `ReallocateCache` calls (deleting unlikely entries to avoid filling the cache), while the https://github.com/bitcoin/bitcoin/pull/31132 is mostly about cache warming (adding likely ones to the cache in parallel).
But as y
...
(https://github.com/bitcoin/bitcoin/pull/31102#issuecomment-2567634389)
> I think https://github.com/bitcoin/bitcoin/pull/31132 is a better approach, because cache misses are fetched in parallel much faster so the misses are not as important.
I see the two changes as orthogonal - the main goal of this change as far as I understand is to avoid `ReallocateCache` calls (deleting unlikely entries to avoid filling the cache), while the https://github.com/bitcoin/bitcoin/pull/31132 is mostly about cache warming (adding likely ones to the cache in parallel).
But as y
...
⚠️ Prabhat1308 opened an issue: "Fuzz: Runtime errors when running fuzz tests on MacOs"
(https://github.com/bitcoin/bitcoin/issues/31591)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Whenever running any fuzz test on any fuzz target on MacOs according to the steps provided [here](https://github.com/bitcoin/bitcoin/issues/31049). There is a big error log being thrown before starting the fuzz tests. To be noted that the fuzz tests run fine afterwards.
### Expected behaviour
No error is thrown and fuzz tests run as expected.
### Steps to reproduce
Set up environment
...
(https://github.com/bitcoin/bitcoin/issues/31591)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Whenever running any fuzz test on any fuzz target on MacOs according to the steps provided [here](https://github.com/bitcoin/bitcoin/issues/31049). There is a big error log being thrown before starting the fuzz tests. To be noted that the fuzz tests run fine afterwards.
### Expected behaviour
No error is thrown and fuzz tests run as expected.
### Steps to reproduce
Set up environment
...
💬 maflcko commented on issue "Fuzz: Runtime errors when running fuzz tests on MacOs":
(https://github.com/bitcoin/bitcoin/issues/31591#issuecomment-2567660283)
Does it work with the `libfuzzer-nosan` preset?
(https://github.com/bitcoin/bitcoin/issues/31591#issuecomment-2567660283)
Does it work with the `libfuzzer-nosan` preset?
💬 maflcko commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2567675519)
> version: 16.2
Thanks. Though, I can't reproduce in GHA (run: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/12582079644/job/35067062801#step:3:14 ). Though, I also get a different output for the clang installed dir:
```
sudo xcode-select --switch /Applications/Xcode_16.2.app
clang --version
Apple clang version 16.0.0 (clang-1600.0.26.6)
Target: arm64-apple-darwin24.2.0
Thread model: posix
InstalledDir: /Applications/Xcode_16.2.app/Contents/Developer/Toolchains/X
...
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2567675519)
> version: 16.2
Thanks. Though, I can't reproduce in GHA (run: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/12582079644/job/35067062801#step:3:14 ). Though, I also get a different output for the clang installed dir:
```
sudo xcode-select --switch /Applications/Xcode_16.2.app
clang --version
Apple clang version 16.0.0 (clang-1600.0.26.6)
Target: arm64-apple-darwin24.2.0
Thread model: posix
InstalledDir: /Applications/Xcode_16.2.app/Contents/Developer/Toolchains/X
...
💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822246)
thx, may do if I have to re-touch, or in a follow-up
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822246)
thx, may do if I have to re-touch, or in a follow-up
💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822308)
No, I don't want to affect performance in one way or another on some compiler (optimization level)s
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822308)
No, I don't want to affect performance in one way or another on some compiler (optimization level)s
💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822388)
I am not touching this line of code, and don't want to, so maybe a separate issue or pull request seems more appropriate?
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822388)
I am not touching this line of code, and don't want to, so maybe a separate issue or pull request seems more appropriate?
💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822444)
No, signed chars are explicitly disallowed. See also: `git grep delete ./src/serialize.h`
```
src/serialize.h:template <typename Stream, CharNotInt8 V> void Serialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t
src/serialize.h:template <typename Stream, CharNotInt8 V> void Unserialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822444)
No, signed chars are explicitly disallowed. See also: `git grep delete ./src/serialize.h`
```
src/serialize.h:template <typename Stream, CharNotInt8 V> void Serialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t
src/serialize.h:template <typename Stream, CharNotInt8 V> void Unserialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t
💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822481)
No, std::memcpy is not `constexpr`, so adding it serves no purpose.
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822481)
No, std::memcpy is not `constexpr`, so adding it serves no purpose.
💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822539)
I am not touching this line of code, and don't want to, so maybe a separate issue or pull request seems more appropriate?
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822539)
I am not touching this line of code, and don't want to, so maybe a separate issue or pull request seems more appropriate?
💬 glozow commented on pull request "[28.x] Finalize 28.1":
(https://github.com/bitcoin/bitcoin/pull/31582#discussion_r1900830981)
Should this be 2025?
(https://github.com/bitcoin/bitcoin/pull/31582#discussion_r1900830981)
Should this be 2025?
🤔 glozow reviewed a pull request: "[28.x] Finalize 28.1"
(https://github.com/bitcoin/bitcoin/pull/31582#pullrequestreview-2527491597)
ACK 898db487f3f4b4ec131e46cb66ab01b35de2c4c7 unless copyright year should be changed
(https://github.com/bitcoin/bitcoin/pull/31582#pullrequestreview-2527491597)
ACK 898db487f3f4b4ec131e46cb66ab01b35de2c4c7 unless copyright year should be changed
✅ maflcko closed a pull request: "Debug Console implementation of generate method"
(https://github.com/bitcoin-core/gui/pull/692)
(https://github.com/bitcoin-core/gui/pull/692)