Bitcoin Core Github
44 subscribers
121K links
Download Telegram
πŸ’¬ hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3520921384)
Here is my Guix build on RISC-V machine:
```
riscv64
451630ecff800ab320a9d5ad8062758df9d700310690110e764c916ae4c8121e guix-build-527acc5ee497/output/dist-archive/bitcoin-527acc5ee497.tar.gz
bf19a3b8e9e9cf609102c38cd6c00dca4d2645f66ae3af591f29fdeceef7b6cf guix-build-527acc5ee497/output/x86_64-w64-mingw32/SHA256SUMS.part
b69dc956f6fd6bb9b4e20e5f4d29eb4ef74e450d250346bf0450cc051b5cfa95 guix-build-527acc5ee497/output/x86_64-w64-mingw32/bitcoin-527acc5ee497-win64-codesigning.tar.gz
9e44c7635
...
πŸ€” frankomosh reviewed a pull request: "transaction: Adding script witness to ToString for CTxIn"
(https://github.com/bitcoin/bitcoin/pull/33711#pullrequestreview-3452460486)
I think the goal for this is reasonable, but doesn't it remove witness data from `CTransaction::ToString()` and break existing debug output?
πŸ’¬ frankomosh commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2517605640)
Though I have not done a thorough test, I think that completely removing this might affect debug workflow. Before removing it, `Ctransaction::ToString()` is able to print all inputs, then a separate block with all witnesses, then all outputs. I don’t think this is possible anymore.
πŸ’¬ frankomosh commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2517569031)
I think there might be an inconsistency in formatting here. `scriptWitness.ToString()` is called directly, while `scriptSig` uses `HexStr().substr(0, 24)`. Shouldn't there be some symmetry ?
πŸ’¬ quapka commented on issue "Fuzz: compare our AES implementation to AES-NI ":
(https://github.com/bitcoin/bitcoin/issues/27548#issuecomment-3521038218)
> Since cryptofuzz is not alive anymore

There is a bit of activity in [Mozilla's fork](https://github.com/MozillaSecurity/cryptofuzz). It's also still present in [OSS-Fuzz](https://github.com/google/oss-fuzz/tree/master/projects/cryptofuzz), but references the non-existent original repository, so probably is not ran anymore.
πŸ‘ hebasto approved a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3452644296)
ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97.
πŸ’¬ willcl-ark commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3521153141)
I get a guix build failure @ 758da5a5732:

```
g++ -pie -Wl,-z,now -g -O2 -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Wconditionally-supported -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -DHAVE_CONFIG_H -pie -Wl,-z,now -static-libstdc++ -static-libgcc -o Tcollect2 \
collect2.o collect2-aix.o vec.o ggc
...
πŸ’¬ maflcko commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3521179236)
I am not using GCC, and I am not sure how many devs are using GCC-15. Is there even a ci task using gcc-15? Maybe Alpine 3.23 later this month? I haven't looked at the build system changes, otherwise this seems fine to do.
πŸ’¬ hebasto commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3521187261)
> and I am not sure how many devs are using GCC-15.

I am :)
πŸ’¬ l0rinc commented on pull request "validation: fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3521247943)
> Developers doing benchmarks/other work who either have sufficient block data or are okay with lowering -minimumchainwork temporarily

I don't know anyone who does `-reindex` benchmarks anymore - `-reindex-chainstate` is usually enough, `-reindex` isn't representative of IBD so we have stopped relying on it.

> reindex while offline wouldn't work anymore

I haven't checked the code yet (am planning on doing that later), but would it be possible to just issue a warning if we're offline and
...
⚠️ maflcko opened an issue: "depends: native_capnp picks system gcc, not CC/CXX"
(https://github.com/bitcoin/bitcoin/issues/33859)
`native_capnp` seems to ignore the CC/CXX settings. This leads to compile failures on distros that come with an older GCC, such as RHEL-based Linux 8, or the latest OpenSuse Leap:

```
# ( cd depends && make CC=clang CXX=clang++ -j $(nproc) )

Configuring native_capnp...
-- The CXX compiler identification is GNU 7.5.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - failed
-- Check for working CXX compiler: /usr/bin/g++
-- Check for working CXX compiler: /usr/bin/g++ -
...
πŸ’¬ l0rinc commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3521252813)
> how many devs are using GCC-15

lately I'm also doing most of my benchmarks with 15
πŸš€ fanquake merged a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842)
πŸ’¬ hebasto commented on issue "depends: native_capnp picks system gcc, not CC/CXX":
(https://github.com/bitcoin/bitcoin/issues/33859#issuecomment-3521322346)
Native compilers are managed by `build_CC` and `build_CXX` variables, no?
πŸš€ fanquake merged a pull request: "build: add `-W*-whitespace`"
(https://github.com/bitcoin/bitcoin/pull/32482)
πŸ’¬ maflcko commented on issue "depends: native_capnp picks system gcc, not CC/CXX":
(https://github.com/bitcoin/bitcoin/issues/33859#issuecomment-3521392898)
thx, i guess some docs about those settings could help. Otherwise, users will have to read the source code.
πŸ’¬ frankomosh commented on pull request "test: add unit test coverage for the empty leaves path in MerkleComputation":
(https://github.com/bitcoin/bitcoin/pull/33858#issuecomment-3521419961)
> I don't think those links work.
>
I'm not sure why they didn't render correctly here.

> Also, this is redundant to the corecheck result anyway?
>
I included them mainly to visually show % increase, but you’re right that they may be redundant.
(also, I think you have been against adding tests without showing any proof of coverage)
πŸ’¬ l0rinc commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#issuecomment-3521439654)
> Since when is this "unused"?

Did some digging, https://github.com/bitcoin/bitcoin/commit/1f0e7ca09c9d7c5787c218156fa5096a1bdf2ea8#diff-706988c23877f8a557484053887f932b2cafb3b5998b50497ce7ff8118ac85a3L135-L165 inlined `ComputeMerkleRoot` in one case, leaving the `proot` and `pmutated` values unused in `MerkleComputation` (cc: @sipa, @Sjors).
And `BlockWitnessMerkleRoot` was introduced in https://github.com/bitcoin/bitcoin/commit/8b49040854be2e26b66366aeae1cba4716f93d93 where it was already
...
πŸ’¬ yancyribbens commented on pull request "test: add case where `TOTAL_TRIES` is exceeded yet solution remains":
(https://github.com/bitcoin/bitcoin/pull/33701#issuecomment-3521446053)
@murchandamus ; would appreciate your input on this.
πŸ’¬ TheCharlatan commented on pull request "ci: Enable experimental kernel stuff in most CI tasks":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2517943925)
Might we be able to keep this `ON` too, if the windows job also copies over the dll?
```diff
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index d433646292..e2bd3a6940 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -369,0 +370 @@ jobs:
+ ${{ env.BASE_BUILD_DIR }}/bin/*.dll
```