💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109325757)
If we're already checking equality, what's the point of checking that it's not equal to something else?
This should also enable us inlining `empty_cache`
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109325757)
If we're already checking equality, what's the point of checking that it's not equal to something else?
This should also enable us inlining `empty_cache`
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109331421)
I really dislike that we're exposing the loop variable just to see if we finished without transitioning.
We seem to exit at worst around `32415` for me locally, maybe we can reduce the attempt count and extract it to a variable:
```C++
constexpr size_t MAX_ATTEMPTS{50'000}; // runaway-loop safety cap
```
And we can add a worst-case condition for the very last iteration which should always be `LARGE`, e.g.:
```C++
// OK → LARGE
for (size_t i{1}; i <= MAX_ATTEMPTS; ++i) {
...
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109331421)
I really dislike that we're exposing the loop variable just to see if we finished without transitioning.
We seem to exit at worst around `32415` for me locally, maybe we can reduce the attempt count and extract it to a variable:
```C++
constexpr size_t MAX_ATTEMPTS{50'000}; // runaway-loop safety cap
```
And we can add a worst-case condition for the very last iteration which should always be `LARGE`, e.g.:
```C++
// OK → LARGE
for (size_t i{1}; i <= MAX_ATTEMPTS; ++i) {
...
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109336914)
I find the `100/98` a weird way to express a percentage - if we simply want to check that it's roughly 256 KiB, we could check their ratio:
```suggestion
BOOST_CHECK_LT(view.DynamicMemoryUsage() / (256 * 1024.0), 1.1);
```
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109336914)
I find the `100/98` a weird way to express a percentage - if we simply want to check that it's roughly 256 KiB, we could check their ratio:
```suggestion
BOOST_CHECK_LT(view.DynamicMemoryUsage() / (256 * 1024.0), 1.1);
```
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109518426)
https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L219 already claims to account for 1-2 pointers, do we need any other change to synchronize it with the current PR?
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109518426)
https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L219 already claims to account for 1-2 pointers, do we need any other change to synchronize it with the current PR?
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109257463)
Indeed, please resolve the comment
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109257463)
Indeed, please resolve the comment
🤔 BrandonOdiwuor reviewed a pull request: "test: Add missing ipc subtree to lint"
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871591979)
ACK fa2bf6bdb7e5f8940fe4bce1aaab29bf2e063b49
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871591979)
ACK fa2bf6bdb7e5f8940fe4bce1aaab29bf2e063b49
💬 l0rinc commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#issuecomment-2913060959)
@pinheadmz, that's the first thing I though of, but unfortunately the header is as mutable as it gets (see e.g. `while (!CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus())) ++block.nNonce;`), so it would be dangerous to cache the hash, given that all fields are accessible from the outside and there's no simple way to invalidate the pre-computed hash, unless we expose them all to getter/setters which would invalidate the hash on change (and which would slow down `nNonce` expl
...
(https://github.com/bitcoin/bitcoin/pull/32487#issuecomment-2913060959)
@pinheadmz, that's the first thing I though of, but unfortunately the header is as mutable as it gets (see e.g. `while (!CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus())) ++block.nNonce;`), so it would be dangerous to cache the hash, given that all fields are accessible from the outside and there's no simple way to invalidate the pre-computed hash, unless we expose them all to getter/setters which would invalidate the hash on change (and which would slow down `nNonce` expl
...
💬 l0rinc commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2109545287)
I didn't want to make it that verbose, people can check the source code if they see a failure here :)
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2109545287)
I didn't want to make it that verbose, people can check the source code if they see a failure here :)
💬 l0rinc commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2913073569)
@andrewtoth, if you've closed it on purpose, do you want to explain the reason? Is it because you agree with @maflcko that making it a built-in functionality might be simpler? I'm also leaning in that direction, especially after https://github.com/bitcoin/bitcoin/pull/31144, where obfuscation should become a lot cheaper.
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2913073569)
@andrewtoth, if you've closed it on purpose, do you want to explain the reason? Is it because you agree with @maflcko that making it a built-in functionality might be simpler? I'm also leaning in that direction, especially after https://github.com/bitcoin/bitcoin/pull/31144, where obfuscation should become a lot cheaper.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2913209698)
A bigger reason to do this in bitcoind is that it may be able to run in the background and lead to a better user experience.
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2913209698)
A bigger reason to do this in bitcoind is that it may be able to run in the background and lead to a better user experience.
💬 luke-jr commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2913210853)
Is this actually intended behaviour?
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2913210853)
Is this actually intended behaviour?
🤔 hebasto reviewed a pull request: "deps: Bump lief to 0.16.5"
(https://github.com/bitcoin/bitcoin/pull/32431#pullrequestreview-2871819728)
> > I think it'd be worth investigating any issues with 0.16.5, rather than deferring.
>
> Sounds good, marking the PR as draft again while I investigate.
>
> I've bisected to this commit in `lief`: [lief-project/LIEF@f23ced2](https://github.com/lief-project/LIEF/commit/f23ced2f4ffc170d0a6f40ff4a1bee575e3447cf)
>
> Seems related to scikit-build 0.10 [changing](https://iscinumpy.dev/post/scikit-build-core-0-10/#other-changes) `cmake.targets` to `build.targets`, and defining `build.target
...
(https://github.com/bitcoin/bitcoin/pull/32431#pullrequestreview-2871819728)
> > I think it'd be worth investigating any issues with 0.16.5, rather than deferring.
>
> Sounds good, marking the PR as draft again while I investigate.
>
> I've bisected to this commit in `lief`: [lief-project/LIEF@f23ced2](https://github.com/lief-project/LIEF/commit/f23ced2f4ffc170d0a6f40ff4a1bee575e3447cf)
>
> Seems related to scikit-build 0.10 [changing](https://iscinumpy.dev/post/scikit-build-core-0-10/#other-changes) `cmake.targets` to `build.targets`, and defining `build.target
...
💬 hebasto commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2109683378)
Is `python-tomli` really necessary?
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2109683378)
Is `python-tomli` really necessary?
💬 hebasto commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2109682830)
```suggestion
lief==0.16.5 \
```
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2109682830)
```suggestion
lief==0.16.5 \
```
👍 hebasto approved a pull request: "test: Add missing ipc subtree to lint"
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871843422)
re-ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f, only a [comment](https://github.com/bitcoin/bitcoin/pull/32623#discussion_r2109243775) has been addressed since my recent [review](https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871162934).
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871843422)
re-ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f, only a [comment](https://github.com/bitcoin/bitcoin/pull/32623#discussion_r2109243775) has been addressed since my recent [review](https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2871162934).
💬 l0rinc commented on pull request "test: Add missing ipc subtree to lint":
(https://github.com/bitcoin/bitcoin/pull/32623#issuecomment-2913287986)
ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f
(https://github.com/bitcoin/bitcoin/pull/32623#issuecomment-2913287986)
ACK fa4b8b16c37830bf7ee1419aae94e99f294f0e9f
🚀 hebasto merged a pull request: "guix: accomodate migration to codeberg"
(https://github.com/bitcoin/bitcoin/pull/32439)
(https://github.com/bitcoin/bitcoin/pull/32439)
💬 hebasto commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#discussion_r2109748436)
Guix's `cmake-minimal` remains at version [3.24.2](https://codeberg.org/guix/guix/src/commit/b868ae741e989d56dbaf8dd481c66a1c00e45e25/gnu/packages/cmake.scm#L179).
(https://github.com/bitcoin/bitcoin/pull/32595#discussion_r2109748436)
Guix's `cmake-minimal` remains at version [3.24.2](https://codeberg.org/guix/guix/src/commit/b868ae741e989d56dbaf8dd481c66a1c00e45e25/gnu/packages/cmake.scm#L179).
👍 pinheadmz approved a pull request: "blocks: avoid recomputing block header hash in `ReadBlock`"
(https://github.com/bitcoin/bitcoin/pull/32487#pullrequestreview-2871920816)
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
Built and tested on macos/arm64 and reviewed code. This is a simple optimization that deduplicates a call to `Block.GetHash()` when reading a block from disk by `BlockManager`. The hash is checked against the expected result from a `CBlockIndex`. Covered by a new unit test.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
-----BEGIN PGP SIGNATURE-
...
(https://github.com/bitcoin/bitcoin/pull/32487#pullrequestreview-2871920816)
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
Built and tested on macos/arm64 and reviewed code. This is a simple optimization that deduplicates a call to `Block.GetHash()` when reading a block from disk by `BlockManager`. The hash is checked against the expected result from a `CBlockIndex`. Covered by a new unit test.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
-----BEGIN PGP SIGNATURE-
...
🤔 theStack reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2871820534)
Reviewed thoroughly up to ff7476ec32f4dbee07bf04169e741e2aea5a9ab7, looks good to me (with one refactoring suggestion below). Still need to go again through the parsing code as there is a lot going on with all the conditions on what is allowed and what not w.r.t. derivations.
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2871820534)
Reviewed thoroughly up to ff7476ec32f4dbee07bf04169e741e2aea5a9ab7, looks good to me (with one refactoring suggestion below). Still need to go again through the parsing code as there is a lot going on with all the conditions on what is allowed and what not w.r.t. derivations.