💬 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:
💬 hodlinator commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1918760332)
The plot thickens - reference to lower version than minimum under *depends/*:
https://github.com/bitcoin/bitcoin/blob/df8bf657450d5383870d40790ea9f4fdb03c360d/depends/hosts/linux.mk#L44-L45
I agree with the author in https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589465760 that it's fine to limit the scope of the current PR.
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1918760332)
The plot thickens - reference to lower version than minimum under *depends/*:
https://github.com/bitcoin/bitcoin/blob/df8bf657450d5383870d40790ea9f4fdb03c360d/depends/hosts/linux.mk#L44-L45
I agree with the author in https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589465760 that it's fine to limit the scope of the current PR.
📝 adlai opened a pull request: "doc: fix minor typos in comments"
(https://github.com/bitcoin/bitcoin/pull/31673)
In the unrelated PR #31621 the linter reported a few typos, that are fixed in this commit. I used the "doc" prefix as it only modifies comments, so none of the more significant prefixes seem appropriate.
(https://github.com/bitcoin/bitcoin/pull/31673)
In the unrelated PR #31621 the linter reported a few typos, that are fixed in this commit. I used the "doc" prefix as it only modifies comments, so none of the more significant prefixes seem appropriate.