π¬ 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 :)
(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
...
(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++ -
...
(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
(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)
(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?
(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)
(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.
(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)
(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
...
(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.
(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
```
(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
```
π¬ stickies-v commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2517974921)
The fact that `CBlockIndex` equality is currently defined as pointer equality seems like an implementation detail, tightly coupled with our block tree implementation. Exposing that as part of our public interface seems brittle and unergonomic?
Logically, I think two `BlockTreeEntry` objects are equal if they point to the same `Block`. I've force-pushed a slight doc update to reflect that.
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2517974921)
The fact that `CBlockIndex` equality is currently defined as pointer equality seems like an implementation detail, tightly coupled with our block tree implementation. Exposing that as part of our public interface seems brittle and unergonomic?
Logically, I think two `BlockTreeEntry` objects are equal if they point to the same `Block`. I've force-pushed a slight doc update to reflect that.
π¬ TheCharlatan commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2518001181)
> Logically, I think two BlockTreeEntry objects are equal if they point to the same Block. I've force-pushed a slight doc update to reflect that.
I would contest that - a block tree entry has a bunch of additional properties besides the block it points to that could make an equality operator by block confusing. Lets say we eventually introduce a way for the user to construct their own block tree entries. Would two such entries be equal if they have a bunch of different values in their fields,
...
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2518001181)
> Logically, I think two BlockTreeEntry objects are equal if they point to the same Block. I've force-pushed a slight doc update to reflect that.
I would contest that - a block tree entry has a bunch of additional properties besides the block it points to that could make an equality operator by block confusing. Lets say we eventually introduce a way for the user to construct their own block tree entries. Would two such entries be equal if they have a bunch of different values in their fields,
...
π€ rkrux reviewed a pull request: "wallet: remove redundant sighash calculation in Musig2 signing flow"
(https://github.com/bitcoin/bitcoin/pull/33665#pullrequestreview-3453129469)
> Each commit should leave the code in a valid state, so it would make sense to move https://github.com/bitcoin/bitcoin/commit/000c685bcdd395d61a58ad9698563cab41e2d7d0 up.
Makes sense, done.
(https://github.com/bitcoin/bitcoin/pull/33665#pullrequestreview-3453129469)
> Each commit should leave the code in a valid state, so it would make sense to move https://github.com/bitcoin/bitcoin/commit/000c685bcdd395d61a58ad9698563cab41e2d7d0 up.
Makes sense, done.
π¬ rkrux commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518072408)
Done
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518072408)
Done
π¬ rkrux commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518068172)
I recall doing it intentionally based on the response I received on a similar suggestion in the MuSig2 signing PR: https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355476052.
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518068172)
I recall doing it intentionally based on the response I received on a similar suggestion in the MuSig2 signing PR: https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2355476052.
π¬ rkrux commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518068751)
`ComputeSchnorrSignatureHash` function of the class `BaseSignatureCreator` had to be made public so that it can be called from the `SignMuSig2` function that makes an implementation in the `DummySignatureCreator` class necessary.
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2518068751)
`ComputeSchnorrSignatureHash` function of the class `BaseSignatureCreator` had to be made public so that it can be called from the `SignMuSig2` function that makes an implementation in the `DummySignatureCreator` class necessary.
β
maflcko closed an issue: "`test_kernel` fails to build on Ubuntu 22.04"
(https://github.com/bitcoin/bitcoin/issues/33846)
(https://github.com/bitcoin/bitcoin/issues/33846)
π¬ stickies-v commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2518136714)
> Lets say we eventually introduce a way for the user to construct their own block tree entries. Would two such entries be equal if they have a bunch of different values in their fields, but point to the same block?
I'm not sure what that would mean conceptually. If we're using the block tree to store validation status as we do now, that would mean we'd let the user define conflicting validation states about a block in the same index?
My conceptual understanding of a `BlockTreeEntry` is th
...
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2518136714)
> Lets say we eventually introduce a way for the user to construct their own block tree entries. Would two such entries be equal if they have a bunch of different values in their fields, but point to the same block?
I'm not sure what that would mean conceptually. If we're using the block tree to store validation status as we do now, that would mean we'd let the user define conflicting validation states about a block in the same index?
My conceptual understanding of a `BlockTreeEntry` is th
...