🤔 l0rinc reviewed a pull request: "doc: Amend notes on benchmarking"
(https://github.com/bitcoin/bitcoin/pull/31690#pullrequestreview-2562207286)
Concept ACK - thanks for clarifying the priorities
Could also be useful to clarify that the data used in the benchmark has to be representative, so we should aim to cover real values (such as block413567.raw).
(https://github.com/bitcoin/bitcoin/pull/31690#pullrequestreview-2562207286)
Concept ACK - thanks for clarifying the priorities
Could also be useful to clarify that the data used in the benchmark has to be representative, so we should aim to cover real values (such as block413567.raw).
💬 l0rinc commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922298014)
may not be obvious what "review is too much" means:
```suggestion
The change might also be rejected if the code bloat or review/maintenance burden is
too high to justify the improvement.
```
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922298014)
may not be obvious what "review is too much" means:
```suggestion
The change might also be rejected if the code bloat or review/maintenance burden is
too high to justify the improvement.
```
💬 l0rinc commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922304877)
might want to clarify that CLang produces a very different output
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922304877)
might want to clarify that CLang produces a very different output
💬 l0rinc commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922303642)
Users often complain about slow RPC calls - could we add those to this list?
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922303642)
Users often complain about slow RPC calls - could we add those to this list?
💬 l0rinc commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922297054)
Seems simpler to remove the leading "of":
```suggestion
Benchmarks are ill-suited for testing denial-of-service issues, as they are
```
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922297054)
Seems simpler to remove the leading "of":
```suggestion
Benchmarks are ill-suited for testing denial-of-service issues, as they are
```
💬 l0rinc commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922302174)
very useful list - could you add explicit benchmark examples for further understanding?
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922302174)
very useful list - could you add explicit benchmark examples for further understanding?
💬 l0rinc commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922295926)
```suggestion
tests](/doc/fuzzing.md) are better suited for this purpose, as they are specifically
aimed at exploring the possible input space.
```
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922295926)
```suggestion
tests](/doc/fuzzing.md) are better suited for this purpose, as they are specifically
aimed at exploring the possible input space.
```
💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1922316363)
Could we use an existing block from Mainnet () - to be sure we're measuring a representative case and not one skewed towards our preferences?
One with many schnorr sigs - but since we don't yet have one that only contains them, I don't think it makes sense to measure that (like we do here).
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1922316363)
Could we use an existing block from Mainnet () - to be sure we're measuring a representative case and not one skewed towards our preferences?
One with many schnorr sigs - but since we don't yet have one that only contains them, I don't think it makes sense to measure that (like we do here).
💬 l0rinc commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922318563)
Could we clarify when we should use `benchmark::PriorityLevel::HIGH` and when the priority should be lower?
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922318563)
Could we clarify when we should use `benchmark::PriorityLevel::HIGH` and when the priority should be lower?
💬 vasild commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2602292591)
> Might be worth noting that this is currently only available on POSIX systems (i.e. not available on Windows
Right. [`GetThreadTimes()`](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getthreadtimes) looks like a promising Windows alternative. I will try to implement that here. Switching to draft because I will push some work-in-progress a bunch of times.
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2602292591)
> Might be worth noting that this is currently only available on POSIX systems (i.e. not available on Windows
Right. [`GetThreadTimes()`](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getthreadtimes) looks like a promising Windows alternative. I will try to implement that here. Switching to draft because I will push some work-in-progress a bunch of times.
📝 vasild converted_to_draft a pull request: "rpc: add cpu_load to getpeerinfo"
(https://github.com/bitcoin/bitcoin/pull/31672)
Add a new field `cpu_load` to the output of `getpeerinfo` RPC.
It represents the CPU time spent by the message handling thread for the given peer, weighted for the duration of the connection. That is, for example, if two peers are equally demanding and one is connected longer than the other, then they will have the same `cpu_load` number.
---
Monitoring CPU usage is useful on its own. Also related to https://github.com/bitcoin/bitcoin/issues/31033.
(https://github.com/bitcoin/bitcoin/pull/31672)
Add a new field `cpu_load` to the output of `getpeerinfo` RPC.
It represents the CPU time spent by the message handling thread for the given peer, weighted for the duration of the connection. That is, for example, if two peers are equally demanding and one is connected longer than the other, then they will have the same `cpu_load` number.
---
Monitoring CPU usage is useful on its own. Also related to https://github.com/bitcoin/bitcoin/issues/31033.
💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2602317569)
I briefly tested a Ledger NanoS with Bitcoin Testnet app 2.2.5 by disabling its v0 - v2 conversion:
```diff
diff --git a/hwilib/devices/ledger.py b/hwilib/devices/ledger.py
index d3cf463..f3a8b92 100644
--- a/hwilib/devices/ledger.py
+++ b/hwilib/devices/ledger.py
@@ -219,8 +219,9 @@ class LedgerClient(HardwareWalletClient):
# Make a deepcopy of this psbt. We will need to modify it to get signing to work,
# which will affect the caller's detection for whether signing
...
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2602317569)
I briefly tested a Ledger NanoS with Bitcoin Testnet app 2.2.5 by disabling its v0 - v2 conversion:
```diff
diff --git a/hwilib/devices/ledger.py b/hwilib/devices/ledger.py
index d3cf463..f3a8b92 100644
--- a/hwilib/devices/ledger.py
+++ b/hwilib/devices/ledger.py
@@ -219,8 +219,9 @@ class LedgerClient(HardwareWalletClient):
# Make a deepcopy of this psbt. We will need to modify it to get signing to work,
# which will affect the caller's detection for whether signing
...
💬 maflcko commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922340402)
Why should the output be different?
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922340402)
Why should the output be different?
💬 maflcko commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922341951)
The list is non-exhaustive and exists for illustration, so I'll leave as-is for now.
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922341951)
The list is non-exhaustive and exists for illustration, so I'll leave as-is for now.
💬 maflcko commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922342187)
The list is non-exhaustive and exists for illustration, so I'll leave as-is for now.
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922342187)
The list is non-exhaustive and exists for illustration, so I'll leave as-is for now.
💬 willcl-ark commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1922344525)
I am not convinced that `walletdir` is a problem, do we have any reports of this?
I can add it, but it looks cleanest to first refactor `GetWalletDir` into `ArgsMan`, so that we can fetch this directory here in *common/init.cpp*, which will make this change quite a bit bigger...
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1922344525)
I am not convinced that `walletdir` is a problem, do we have any reports of this?
I can add it, but it looks cleanest to first refactor `GetWalletDir` into `ArgsMan`, so that we can fetch this directory here in *common/init.cpp*, which will make this change quite a bit bigger...
💬 willcl-ark commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1922346953)
added in df1ba101419729aafa382d970ee605e2a6273e26
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1922346953)
added in df1ba101419729aafa382d970ee605e2a6273e26
💬 willcl-ark commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1922353113)
> For GUI, maybe we should check this while they still have the opportunity to change it...
To add this check to the *qt/intro.cpp* flow would require duplicating some of the init checks, as well as:
- first creating the chosen directory
- check what FS Type the created dir is
- if EXFAT prompt the user to change filesystems
- either remove the just-created directories (or keep them) depending on response.
- etc.
If we want that kind of logic then I'm happy to add it, but not sure i
...
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1922353113)
> For GUI, maybe we should check this while they still have the opportunity to change it...
To add this check to the *qt/intro.cpp* flow would require duplicating some of the init checks, as well as:
- first creating the chosen directory
- check what FS Type the created dir is
- if EXFAT prompt the user to change filesystems
- either remove the just-created directories (or keep them) depending on response.
- etc.
If we want that kind of logic then I'm happy to add it, but not sure i
...
💬 l0rinc commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922358366)
See the examples in e.g. https://github.com/bitcoin/bitcoin/pull/31682:
> C++ compiler .......................... AppleClang 16.0.0.16000026
| ns/block | block/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 361,535.93 | 2,765.98 | 0.5% | 11.02 | `CheckBlockBench`
> C++ compiler .......................... GNU 13.3.0
| ns/block | block/s |
...
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922358366)
See the examples in e.g. https://github.com/bitcoin/bitcoin/pull/31682:
> C++ compiler .......................... AppleClang 16.0.0.16000026
| ns/block | block/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 361,535.93 | 2,765.98 | 0.5% | 11.02 | `CheckBlockBench`
> C++ compiler .......................... GNU 13.3.0
| ns/block | block/s |
...
💬 l0rinc commented on pull request "doc: Amend notes on benchmarking":
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922361205)
I'd just like to avoid reviers claiming that they reject a PR because it's not in this list - but you're right, the "non-exhaustive list" should make that clear - you can resolve these
(https://github.com/bitcoin/bitcoin/pull/31690#discussion_r1922361205)
I'd just like to avoid reviers claiming that they reject a PR because it's not in this list - but you're right, the "non-exhaustive list" should make that clear - you can resolve these