📝 vasild opened 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.
💬 brunoerg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1918646257)
88b67c95cbc9c3ae75ccbc39e4aba215b9910b8b: Since you're checking the "descriptors" field into `migrate_and_get_rpc`, you could also assert the wallet is sqlite and then remove the duplicated code along the test. e.g.:
```py
_, multisig0 = self.migrate_and_get_rpc("multisig0")
assert_equal(multisig0.getwalletinfo()["descriptors"], True)
self.assert_is_sqlite("multisig0")
```
```py
_, multisig0 = self.migrate_and_get_rpc("multisig0")
assert_equal(
...
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1918646257)
88b67c95cbc9c3ae75ccbc39e4aba215b9910b8b: Since you're checking the "descriptors" field into `migrate_and_get_rpc`, you could also assert the wallet is sqlite and then remove the duplicated code along the test. e.g.:
```py
_, multisig0 = self.migrate_and_get_rpc("multisig0")
assert_equal(multisig0.getwalletinfo()["descriptors"], True)
self.assert_is_sqlite("multisig0")
```
```py
_, multisig0 = self.migrate_and_get_rpc("multisig0")
assert_equal(
...
💬 jonatack commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2595869832)
Concept ACK if this can be a reliable, useful metric and help pave the path to https://github.com/bitcoin/bitcoin/issues/31033.
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2595869832)
Concept ACK if this can be a reliable, useful metric and help pave the path to https://github.com/bitcoin/bitcoin/issues/31033.
💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2595871882)
Updated for @darosior's suggestions.
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2595871882)
Updated for @darosior's suggestions.
💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918705629)
clang-tidy complains about this:
> [09:51:32.084] [584/666][11.5s] clang-tidy-19 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/test/pcp_tests.cpp
[09:51:32.084] /ci_container_base/src/test/pcp_tests.cpp:96:5: error: use '= default' to define a trivial destructor [modernize-use-equals-default,-warnings-as-errors]
[09:51:32.084] 96 | ~PCPTestSock() override { }
[09:51:32.084] | ^
...
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918705629)
clang-tidy complains about this:
> [09:51:32.084] [584/666][11.5s] clang-tidy-19 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/test/pcp_tests.cpp
[09:51:32.084] /ci_container_base/src/test/pcp_tests.cpp:96:5: error: use '= default' to define a trivial destructor [modernize-use-equals-default,-warnings-as-errors]
[09:51:32.084] 96 | ~PCPTestSock() override { }
[09:51:32.084] | ^
...
💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918717853)
Yes, this is really silly. Linters are useful for preventing constructs that have high risk of bugs but this just seems like busywork.
Anyhow, removed the destructor completely and re-pushed.
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918717853)
Yes, this is really silly. Linters are useful for preventing constructs that have high risk of bugs but this just seems like busywork.
Anyhow, removed the destructor completely and re-pushed.
🚀 fanquake merged a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483)
(https://github.com/bitcoin/bitcoin/pull/31483)
💬 maflcko commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918740367)
I added this in 3333bae9b2a, because it allows compiler optimizations according to the docs (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html). Obviously the performance doesn't matter in most cases (like in the context of this pull). Though, it is also one less thing to bikeshed about. Still, I am happy to have it taken out again.
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918740367)
I added this in 3333bae9b2a, because it allows compiler optimizations according to the docs (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html). Obviously the performance doesn't matter in most cases (like in the context of this pull). Though, it is also one less thing to bikeshed about. Still, I am happy to have it taken out again.
💬 Sjors commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `void(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2596004758)
Concept ACK. This seems like a step in the right direction.
I tried updating the multisig tutorial to take advantage of this in https://github.com/Sjors/bitcoin/commit/7bb9a172c6539de5c02ac70f9e38724a0e3e82dc. It also uses `gethdkeys` and the new `<0;1>` notation.
But it's still a massive pain for two reasons:
1. There's no way to get the xpub at `m/87'/1'/0'`; I create a workaround which inserts a dummy `pk()` with that derivation, but this is ugly and requires passing the master xprv a
...
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2596004758)
Concept ACK. This seems like a step in the right direction.
I tried updating the multisig tutorial to take advantage of this in https://github.com/Sjors/bitcoin/commit/7bb9a172c6539de5c02ac70f9e38724a0e3e82dc. It also uses `gethdkeys` and the new `<0;1>` notation.
But it's still a massive pain for two reasons:
1. There's no way to get the xpub at `m/87'/1'/0'`; I create a workaround which inserts a dummy `pk()` with that derivation, but this is ugly and requires passing the master xprv a
...
💬 maflcko commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1918744397)
Looks like this fails CI https://cirrus-ci.com/task/4584274617171968?logs=ci#L2784:
```
[15:13:20.370] test 2025-01-16T15:13:19.950000Z TestFramework (ERROR): Assertion failed
[15:13:20.370] Traceback (most recent call last):
[15:13:20.370] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 135, in main
[15:13:20.370] self.run_test()
[15:13:20.37
...
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1918744397)
Looks like this fails CI https://cirrus-ci.com/task/4584274617171968?logs=ci#L2784:
```
[15:13:20.370] test 2025-01-16T15:13:19.950000Z TestFramework (ERROR): Assertion failed
[15:13:20.370] Traceback (most recent call last):
[15:13:20.370] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 135, in main
[15:13:20.370] self.run_test()
[15:13:20.37
...
💬 instagibbs commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1918746739)
wait until?
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1918746739)
wait until?
💬 maflcko commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2596012787)
The msan CI failure seems unrelated, so I've rerun it and left a comment here: https://github.com/bitcoin/bitcoin/pull/31397/files#r1918744397
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2596012787)
The msan CI failure seems unrelated, so I've rerun it and left a comment here: https://github.com/bitcoin/bitcoin/pull/31397/files#r1918744397
💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1918750243)
> In thread [#31483 (comment)](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915356827):
>
> > I see. So the max amount you can safely shift is without overflowing is not `std::numeric_limits<W>::digits - 1` but actually `std::numeric_limits<W>::digits - std::numeric_limits<T>::digits` since that is how much free space there is to the right of T's bits in W.
>
> Would appreciate if someone were to ELI5 this explanation. I'm thinking it must be `clamp(widen(i) << shift)` that d
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1918750243)
> In thread [#31483 (comment)](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915356827):
>
> > I see. So the max amount you can safely shift is without overflowing is not `std::numeric_limits<W>::digits - 1` but actually `std::numeric_limits<W>::digits - std::numeric_limits<T>::digits` since that is how much free space there is to the right of T's bits in W.
>
> Would appreciate if someone were to ELI5 this explanation. I'm thinking it must be `clamp(widen(i) << shift)` that d
...
💬 adlai commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596016591)
> If this change is made, `depends/README.md` should be updated as well. Same for `ci/test/00_setup_env.sh` and `.github/workflows/ci.yml`.
I have force-pushed to include updates to those two files, along with a few more places; there are still several results to `git grep pkg-config`: mostly from old release notes, which should probably be left untouched; a few from other distros, where I've not tested things; and a few invocations of pkg-config which should work with either version, so I've
...
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596016591)
> If this change is made, `depends/README.md` should be updated as well. Same for `ci/test/00_setup_env.sh` and `.github/workflows/ci.yml`.
I have force-pushed to include updates to those two files, along with a few more places; there are still several results to `git grep pkg-config`: mostly from old release notes, which should probably be left untouched; a few from other distros, where I've not tested things; and a few invocations of pkg-config which should work with either version, so I've
...
💬 sipa commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2596019759)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2596019759)
Concept ACK
💬 maflcko commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596024083)
> along with a few more places
You can't change the others, as they are fixed subtrees. (See the CI failure).
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596024083)
> along with a few more places
You can't change the others, as they are fixed subtrees. (See the CI failure).
💬 adlai commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596030806)
The automated checks are now reporting two instances of:
```
FAIL: subtree directory was touched without subtree merge
```
from the places were I fixed a typo in a comment. I'm not familiar with this specific linter setup, and this isn't supposed to be a merge commit, so could someone please point me towards some documentation or just explain directly why this check fails?
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596030806)
The automated checks are now reporting two instances of:
```
FAIL: subtree directory was touched without subtree merge
```
from the places were I fixed a typo in a comment. I'm not familiar with this specific linter setup, and this isn't supposed to be a merge commit, so could someone please point me towards some documentation or just explain directly why this check fails?
💬 l0rinc commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2596035012)
I'm not sure these arguments would hold in other cases (e.g. no compiler safety needed since the tests would fail otherwise), but I've removed the consts as well, basically only leaving @ryanofsky's changes.
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2596035012)
I'm not sure these arguments would hold in other cases (e.g. no compiler safety needed since the tests would fail otherwise), but I've removed the consts as well, basically only leaving @ryanofsky's changes.
💬 adlai commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596041403)
> > along with a few more places
>
> You can't change the others, as they are fixed subtrees. (See the CI failure).
I force-pushed again to remove the modifications to the two Dockerfile scripts.
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596041403)
> > along with a few more places
>
> You can't change the others, as they are fixed subtrees. (See the CI failure).
I force-pushed again to remove the modifications to the two Dockerfile scripts.
👍 hodlinator approved a pull request: "doc: Improve dependencies documentation"
(https://github.com/bitcoin/bitcoin/pull/31634#pullrequestreview-2556503320)
re-ACK 05a008c1d07a38f589536661822d5450aa4e9438
- Thanks to @rkrux for [spotting the formatting issue](https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1913559740) I missed.
- SQLite now categorized as `(wallet)`. :+1:
(https://github.com/bitcoin/bitcoin/pull/31634#pullrequestreview-2556503320)
re-ACK 05a008c1d07a38f589536661822d5450aa4e9438
- Thanks to @rkrux for [spotting the formatting issue](https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1913559740) I missed.
- SQLite now categorized as `(wallet)`. :+1: