💬 luke-jr commented on pull request "subprocess: Let shell parse command on non-Windows systems":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2912964132)
Probably should really go through the shell on Windows too...
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2912964132)
Probably should really go through the shell on Windows too...
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109501741)
Yes, but it loses the error information so you'd silently continue even if write failed leading to data corruption.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109501741)
Yes, but it loses the error information so you'd silently continue even if write failed leading to data corruption.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109507090)
On the next run it'll crash failing to convert into `[u8; 8]`. While not catastrophic, it makes the program more annoying for end users.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2109507090)
On the next run it'll crash failing to convert into `[u8; 8]`. While not catastrophic, it makes the program more annoying for end users.
🤔 l0rinc requested changes to a pull request: "improve MallocUsage() accuracy"
(https://github.com/bitcoin/bitcoin/pull/28531#pullrequestreview-2871139248)
Thanks for getting back to this change.
We should synchronize it with `CCoinsMap`, chich already claims to account for an "overhead of 1 or 2 pointers".
Also, the test should pass before the change as well, so it would be helpful to add it as a separate commit before the change to make it easy to see how it behaves before and after the change as well.
The test still contains a lot of repetition, please see my suggestion on how to compact it a bit more.
There are still uncovered parts of the
...
(https://github.com/bitcoin/bitcoin/pull/28531#pullrequestreview-2871139248)
Thanks for getting back to this change.
We should synchronize it with `CCoinsMap`, chich already claims to account for an "overhead of 1 or 2 pointers".
Also, the test should pass before the change as well, so it would be helpful to add it as a separate commit before the change to make it easy to see how it behaves before and after the change as well.
The test still contains a lot of repetition, please see my suggestion on how to compact it a bit more.
There are still uncovered parts of the
...
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109307830)
Instead of empirically, can we add documentation or source code links here?
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109307830)
Instead of empirically, can we add documentation or source code links here?
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109228294)
@LarryRuane I think you should probably be the author of the commit, given that you've already added @theStack as a co-author
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109228294)
@LarryRuane I think you should probably be the author of the commit, given that you've already added @theStack as a co-author
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109304532)
```suggestion
// The memory used by an unordered_set or unordered_map is the sum of the
```
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109304532)
```suggestion
// The memory used by an unordered_set or unordered_map is the sum of the
```
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109380457)
it makes sense to add the `/*max_mempool_size_bytes=*` for primitives - but we don't usually do it for self-explanatory variables:
```suggestion
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, MAX_MEMPOOL_CACHE_BYTES),
```
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2109380457)
it makes sense to add the `/*max_mempool_size_bytes=*` for primitives - but we don't usually do it for self-explanatory variables:
```suggestion
chainstate.GetCoinsCacheSizeState(MAX_COINS_CACHE_BYTES, MAX_MEMPOOL_CACHE_BYTES),
```
💬 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
...