💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1424023270)
this should use std::cerr
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1424023270)
this should use std::cerr
💬 fanquake commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1852088669)
Concept ACK. We can also check the benchmarks in https://corecheck.dev/bitcoin/bitcoin/pulls/29057 when they become available.
> I can either add a commit on top of this PR to do the switch after the c-i has run or do it as a follow-up PR, I have no preference.
I think the changes here should be straightforward enough, that you could push another commit doing the cleanup.
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1852088669)
Concept ACK. We can also check the benchmarks in https://corecheck.dev/bitcoin/bitcoin/pulls/29057 when they become available.
> I can either add a commit on top of this PR to do the switch after the c-i has run or do it as a follow-up PR, I have no preference.
I think the changes here should be straightforward enough, that you could push another commit doing the cleanup.
💬 fanquake commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1424034141)
Have dropped this change.
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1424034141)
Have dropped this change.
💬 furszy commented on pull request "wallet: tx creation, don't select outputs from txes that are being replaced":
(https://github.com/bitcoin/bitcoin/pull/26732#issuecomment-1852094839)
> Is there an outcome? Seems like nothing has happened on the pull since, other than it needing rebase.
Not yet. Still need to rework #27601 first. Other wallet works have gotten more priority than this working path. This two PRs are still bugs within the wallet but they could wait a bit in favor of other PRs.
I don't have a fixed time to get back to this work, but will try to do it after new year. We should be able to merge few of the ongoing big PRs by then.
(https://github.com/bitcoin/bitcoin/pull/26732#issuecomment-1852094839)
> Is there an outcome? Seems like nothing has happened on the pull since, other than it needing rebase.
Not yet. Still need to rework #27601 first. Other wallet works have gotten more priority than this working path. This two PRs are still bugs within the wallet but they could wait a bit in favor of other PRs.
I don't have a fixed time to get back to this work, but will try to do it after new year. We should be able to merge few of the ongoing big PRs by then.
💬 fanquake commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1424036835)
> Maybe in the future the upstream build could do a better job of determining whether PIC code is needed itself, and this setting could be dropped.
I agree with your comments, and will probably followup with some improvements. I think historically, the usage of `--with-pic`, has been a bit whack-a-mole esqu, where it's been added as issues have arrison / as people have tested things of various platforms, leading to the inconsistent state we have today.
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1424036835)
> Maybe in the future the upstream build could do a better job of determining whether PIC code is needed itself, and this setting could be dropped.
I agree with your comments, and will probably followup with some improvements. I think historically, the usage of `--with-pic`, has been a bit whack-a-mole esqu, where it's been added as issues have arrison / as people have tested things of various platforms, leading to the inconsistent state we have today.
💬 maflcko commented on pull request "ci: Use Ubuntu 24.04 Noble for asan,tsan,tidy,fuzz":
(https://github.com/bitcoin/bitcoin/pull/28992#issuecomment-1852101216)
Will be fixed in `17.0.6-2` once noble-proposed is released, see https://bugs.launchpad.net/ubuntu/+source/llvm-toolchain-17/1%3A17.0.6-2/+publishinghistory
(https://github.com/bitcoin/bitcoin/pull/28992#issuecomment-1852101216)
Will be fixed in `17.0.6-2` once noble-proposed is released, see https://bugs.launchpad.net/ubuntu/+source/llvm-toolchain-17/1%3A17.0.6-2/+publishinghistory
💬 aureleoules commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852115397)
I was not aware that this issue existed but I've started working on monitoring benchmark results on pull requests on [corecheck](https://corecheck.dev/). For example: https://corecheck.dev/bitcoin/bitcoin/pulls/28674.
It is still experimental and I am still working on reducing the noise between runs, but as of today I usually don't see more than 5-6% difference between identical bench runs.
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852115397)
I was not aware that this issue existed but I've started working on monitoring benchmark results on pull requests on [corecheck](https://corecheck.dev/). For example: https://corecheck.dev/bitcoin/bitcoin/pulls/28674.
It is still experimental and I am still working on reducing the noise between runs, but as of today I usually don't see more than 5-6% difference between identical bench runs.
💬 fanquake commented on pull request "Add missing byteswap functions for MSVC":
(https://github.com/bitcoin/bitcoin/pull/29036#issuecomment-1852122243)
cc @sipsorcery.
> Also we should remember that MSVC build uses no optimisation
Could rebase on master now that https://github.com/bitcoin/bitcoin/pull/29045 has gone in.
(https://github.com/bitcoin/bitcoin/pull/29036#issuecomment-1852122243)
cc @sipsorcery.
> Also we should remember that MSVC build uses no optimisation
Could rebase on master now that https://github.com/bitcoin/bitcoin/pull/29045 has gone in.
💬 epompeii commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852127121)
@aureleoules that looks really nice!
Would you be interesting in plotting those data over time? If so I can work on ingesting your results into Bencher, similar to how `rustls` is doing it: https://bencher.dev/perf/rustls-821705769
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852127121)
@aureleoules that looks really nice!
Would you be interesting in plotting those data over time? If so I can work on ingesting your results into Bencher, similar to how `rustls` is doing it: https://bencher.dev/perf/rustls-821705769
💬 glozow commented on pull request "mempool / miner: regularly flush <=0-fee entries and mine everything in the mempool":
(https://github.com/bitcoin/bitcoin/pull/27018#issuecomment-1852127137)
Planning to resurrect this after #28948.
(https://github.com/bitcoin/bitcoin/pull/27018#issuecomment-1852127137)
Planning to resurrect this after #28948.
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1852127685)
@sdaftuar I took your suggestions, reordered and slightly reworded. Matches my implementation.
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1852127685)
@sdaftuar I took your suggestions, reordered and slightly reworded. Matches my implementation.
⚠️ aureleoules opened an issue: "Benchmark WalletCreateTxUsePresetInputsAndCoinSelection due to #"
(https://github.com/bitcoin/bitcoin/issues/29061)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The WalletCreateTxUsePresetInputsAndCoinSelection benchmarks crashes on master due to the commit 758501b71391136c33b525b1a0109b990d4f463e introduced in #25273 (I used git bisect to verify).
```
valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000
==34277== Memcheck, a memory error detector
==34277== Copyright (C) 2002-2017, and GNU
...
(https://github.com/bitcoin/bitcoin/issues/29061)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The WalletCreateTxUsePresetInputsAndCoinSelection benchmarks crashes on master due to the commit 758501b71391136c33b525b1a0109b990d4f463e introduced in #25273 (I used git bisect to verify).
```
valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000
==34277== Memcheck, a memory error detector
==34277== Copyright (C) 2002-2017, and GNU
...
💬 fanquake commented on issue "Benchmark WalletCreateTxUsePresetInputsAndCoinSelection due to #":
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852135389)
Was this fixed in #29055?
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852135389)
Was this fixed in #29055?
💬 aureleoules commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852136094)
> Would you be interesting in plotting those data over time?
Yes I plan to display on the homepage the plot of benchmarks and test coverage ratio of master over time!
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852136094)
> Would you be interesting in plotting those data over time?
Yes I plan to display on the homepage the plot of benchmarks and test coverage ratio of master over time!
💬 maflcko commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852136752)
Agree that a plot over time would be useful. They were on https://codespeed.bitcoinperf.com/timeline/ , but it hasn't run for some years now.
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852136752)
Agree that a plot over time would be useful. They were on https://codespeed.bitcoinperf.com/timeline/ , but it hasn't run for some years now.
💬 fanquake commented on issue "./contrib/guix/guix-build does not work on riscv64":
(https://github.com/bitcoin/bitcoin/issues/29020#issuecomment-1852139445)
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=67697
Apparently the fix has now landed, but I don't yet see it in https://git.savannah.gnu.org/cgit/guix.git/log/ (or core-updates).
(https://github.com/bitcoin/bitcoin/issues/29020#issuecomment-1852139445)
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=67697
Apparently the fix has now landed, but I don't yet see it in https://git.savannah.gnu.org/cgit/guix.git/log/ (or core-updates).
💬 maflcko commented on issue "Benchmark WalletCreateTxUsePresetInputsAndCoinSelection due to #":
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852140217)
Why was this not caught by CI?
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852140217)
Why was this not caught by CI?
💬 fanquake commented on issue "Benchmark WalletCreateTxUsePresetInputsAndCoinSelection due to #":
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852145959)
> Why was this not caught by CI?
Looks like that is a "low priority" benchmark, meaning it's not run in the CI, which only run high?
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852145959)
> Why was this not caught by CI?
Looks like that is a "low priority" benchmark, meaning it's not run in the CI, which only run high?
💬 epompeii commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852146955)
Sounds great!
If you want them to be live updating, you can embed Bencher plots. Just go to the `Share` button on the Perf Page and copy the Embed Perf Plot Link for the current plot. [This is an example](https://bencher.dev/learn/benchmarking/rust/libtest-bench/#catch-performance-regressions-in-ci) of what that could look like.
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852146955)
Sounds great!
If you want them to be live updating, you can embed Bencher plots. Just go to the `Share` button on the Perf Page and copy the Embed Perf Plot Link for the current plot. [This is an example](https://bencher.dev/learn/benchmarking/rust/libtest-bench/#catch-performance-regressions-in-ci) of what that could look like.
💬 aureleoules commented on issue "Benchmark WalletCreateTxUsePresetInputsAndCoinSelection due to #25273":
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852149860)
> Was this fixed in https://github.com/bitcoin/bitcoin/pull/29055?
Unfortunately no, it still crashes on master as of a7484be65f7617d77aff92ecf6f5fb26015d27a8
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852149860)
> Was this fixed in https://github.com/bitcoin/bitcoin/pull/29055?
Unfortunately no, it still crashes on master as of a7484be65f7617d77aff92ecf6f5fb26015d27a8