📝 maflcko opened a pull request: "ci: Clear unused /msan/llvm-project"
(https://github.com/bitcoin/bitcoin/pull/30369)
Could help to fix the `no space left on device` that are sometimes seen.
(https://github.com/bitcoin/bitcoin/pull/30369)
Could help to fix the `no space left on device` that are sometimes seen.
💬 maflcko commented on pull request "ci: Clear unused /msan/llvm-project":
(https://github.com/bitcoin/bitcoin/pull/30369#issuecomment-2200225800)
Ref: https://cirrus-ci.com/task/5394417295556608?logs=ci#L8647
`Error: committing container for step {Env:[FILE_ENV=./ci/test/00_setup_env_native_msan.sh PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin] Command:run Args:[bash -c cd /ci_container_base/ && set -o errexit && source ./ci/test/00_setup_env.sh && ./ci/test/01_base_install.sh] Flags:[] Attrs:map[json:true] Message:RUN bash -c cd /ci_container_base/ && set -o errexit && source ./ci/test/00_setup_env.sh && ./ci/test/
...
(https://github.com/bitcoin/bitcoin/pull/30369#issuecomment-2200225800)
Ref: https://cirrus-ci.com/task/5394417295556608?logs=ci#L8647
`Error: committing container for step {Env:[FILE_ENV=./ci/test/00_setup_env_native_msan.sh PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin] Command:run Args:[bash -c cd /ci_container_base/ && set -o errexit && source ./ci/test/00_setup_env.sh && ./ci/test/01_base_install.sh] Flags:[] Attrs:map[json:true] Message:RUN bash -c cd /ci_container_base/ && set -o errexit && source ./ci/test/00_setup_env.sh && ./ci/test/
...
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661103537)
So `inline` has two different effects in compilers:
* 1. It allows the function to appear in multiple compilation units (which makes it mandatory here). Templated entities automatically have this behavior (with or without explicit `inline`).
* 2. It suggests to the compiler that its implementation should be inlined in call sites. Because this inlining has no observable effect on compiler behavior, compilers can do this (or not do this) with or without `inline`. It has become less important as
...
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661103537)
So `inline` has two different effects in compilers:
* 1. It allows the function to appear in multiple compilation units (which makes it mandatory here). Templated entities automatically have this behavior (with or without explicit `inline`).
* 2. It suggests to the compiler that its implementation should be inlined in call sites. Because this inlining has no observable effect on compiler behavior, compilers can do this (or not do this) with or without `inline`. It has become less important as
...
💬 maflcko commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200234829)
> How do I find small changes that are needed, welcome, not so controversial, and don't need 3 years of up-front studying?
You can spend time on in-depth review of open pull requests and compare your review with the review done by others. This may be time-consuming (Yes, some pull requests may need up-front studying, but there are many that do not). Ideally the process will teach you relevant skills for this project and for yourself to apply elsewhere. Moreover, on some pull requests there re
...
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200234829)
> How do I find small changes that are needed, welcome, not so controversial, and don't need 3 years of up-front studying?
You can spend time on in-depth review of open pull requests and compare your review with the review done by others. This may be time-consuming (Yes, some pull requests may need up-front studying, but there are many that do not). Ideally the process will teach you relevant skills for this project and for yourself to apply elsewhere. Moreover, on some pull requests there re
...
💬 andrewtoth commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200237685)
> How do I find small changes that are needed, welcome, not so controversial, and don't need 3 years of up-front studying?
@paplorinc I know you are past your first PR and issue, but most of the issues with this label would fit that criteria https://github.com/bitcoin/bitcoin/labels/good%20first%20issue.
Also, browsing open issues I'm sure you could find others that fulfill most of the criteria as well.
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200237685)
> How do I find small changes that are needed, welcome, not so controversial, and don't need 3 years of up-front studying?
@paplorinc I know you are past your first PR and issue, but most of the issues with this label would fit that criteria https://github.com/bitcoin/bitcoin/labels/good%20first%20issue.
Also, browsing open issues I'm sure you could find others that fulfill most of the criteria as well.
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661115104)
@andrewtoth The [C++ Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Re-noexcept) suggest adding `noexcept` whenever you know a function won't throw. Bitcoin Core treats failure to allocate memory as an unrecoverable condition, so having dynamic memory allocations in a function is not a reason not to add it.
It may or may not have a performance benefit (especially when it's a function called from a different compilation unit - and we're not compiling with LTO ena
...
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661115104)
@andrewtoth The [C++ Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Re-noexcept) suggest adding `noexcept` whenever you know a function won't throw. Bitcoin Core treats failure to allocate memory as an unrecoverable condition, so having dynamic memory allocations in a function is not a reason not to add it.
It may or may not have a performance benefit (especially when it's a function called from a different compilation unit - and we're not compiling with LTO ena
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661123105)
@andrewtoth are you asking because of the hasher involved in caching, i.e. https://github.com/bitcoin/bitcoin/blob/master/src/util/hasher.h#L41-L50?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661123105)
@andrewtoth are you asking because of the hasher involved in caching, i.e. https://github.com/bitcoin/bitcoin/blob/master/src/util/hasher.h#L41-L50?
💬 brunoerg commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200269329)
I'm not 100% convinced of the change in this PR, I mean, just removing the second part of the expected log.
The log (from https://github.com/bitcoin/bitcoin/issues/30030) says:
```
test 2024-04-28T17:46:07.437000Z TestFramework (INFO): Ensure we try to connect if port=0 and get an error due to missing I2P proxy
node0 2024-04-28T17:46:07.437631Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:37638
node0 2024-04-28T17:46:07.437669Z [h
...
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200269329)
I'm not 100% convinced of the change in this PR, I mean, just removing the second part of the expected log.
The log (from https://github.com/bitcoin/bitcoin/issues/30030) says:
```
test 2024-04-28T17:46:07.437000Z TestFramework (INFO): Ensure we try to connect if port=0 and get an error due to missing I2P proxy
node0 2024-04-28T17:46:07.437631Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:37638
node0 2024-04-28T17:46:07.437669Z [h
...
💬 hebasto commented on pull request "ci: Clear unused /msan/llvm-project":
(https://github.com/bitcoin/bitcoin/pull/30369#issuecomment-2200277172)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30369#issuecomment-2200277172)
Concept ACK.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661133862)
@sipa thank you for the explanation.
@paplorinc that was an example where it made a difference to performance that came to my mind, yes. I was just curious whether there would be any performance benefit of adding `noexcept` on these methods. If the asm generated with or without is the same then the answer is no. But the advice for adding them for human documentation makes sense.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661133862)
@sipa thank you for the explanation.
@paplorinc that was an example where it made a difference to performance that came to my mind, yes. I was just curious whether there would be any performance benefit of adding `noexcept` on these methods. If the asm generated with or without is the same then the answer is no. But the advice for adding them for human documentation makes sense.
💬 paplorinc commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200300155)
@andrewtoth, that was the first thing I checked, couldn't find any reasonable "good first issue" - or other issue that piqued my interest. The problem has to be personal, otherwise the solution won't be honest.
@maflcko, you already suggested that multiple times, but my question wasn't how to review PRs.
I'll check if this PR improved `listunspent`, as suggested, if not, I'll just close this.
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200300155)
@andrewtoth, that was the first thing I checked, couldn't find any reasonable "good first issue" - or other issue that piqued my interest. The problem has to be personal, otherwise the solution won't be honest.
@maflcko, you already suggested that multiple times, but my question wasn't how to review PRs.
I'll check if this PR improved `listunspent`, as suggested, if not, I'll just close this.
💬 maflcko commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1661142027)
This is reverted in https://github.com/bitcoin/bitcoin/pull/30345/files (as of now)
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1661142027)
This is reverted in https://github.com/bitcoin/bitcoin/pull/30345/files (as of now)
💬 andrewtoth commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200316763)
@paplorinc https://github.com/bitcoin/bitcoin/pull/28945 was abandoned and I don't think @fjahr has taken it over. Perhaps that would be interesting to you.
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200316763)
@paplorinc https://github.com/bitcoin/bitcoin/pull/28945 was abandoned and I don't think @fjahr has taken it over. Perhaps that would be interesting to you.
💬 brunoerg commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1661167148)
left a comment there
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1661167148)
left a comment there
📝 fjahr opened a pull request: "sync: improve CCoinsViewCache ReallocateCache - 2nd try"
(https://github.com/bitcoin/bitcoin/pull/30370)
Rebased and reopened #28945
Original description:
> TL;DR: this change improves sync time on my PC by ~6%.
>
> While syncing when the CCoinsViewCache gets full it is flushed to disk and then memory is freed. Depending on available cache size, this happens several times. This PR removes the unnecessary reallocations for temporary CCoinsViewCache and reserves the cacheCoins's bucket array to its previous size.
>
> I benchmarked the change on an AMD Ryzen 9 7950X with this command:
>
...
(https://github.com/bitcoin/bitcoin/pull/30370)
Rebased and reopened #28945
Original description:
> TL;DR: this change improves sync time on my PC by ~6%.
>
> While syncing when the CCoinsViewCache gets full it is flushed to disk and then memory is freed. Depending on available cache size, this happens several times. This PR removes the unnecessary reallocations for temporary CCoinsViewCache and reserves the cacheCoins's bucket array to its previous size.
>
> I benchmarked the change on an AMD Ryzen 9 7950X with this command:
>
...
💬 fjahr commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200359855)
I have reopened https://github.com/bitcoin/bitcoin/pull/28945 now
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200359855)
I have reopened https://github.com/bitcoin/bitcoin/pull/28945 now
💬 andrewtoth commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200364198)
Ahh sorry thanks.
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200364198)
Ahh sorry thanks.
🤔 glozow reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2151586199)
concept + light code review ACK 064a401e10f2b2da8f31f682929431d06c671d93
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2151586199)
concept + light code review ACK 064a401e10f2b2da8f31f682929431d06c671d93
💬 glozow commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1661166403)
maybe a comment to document that we intend to make a copy of the pre-initialized hasher?
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1661166403)
maybe a comment to document that we intend to make a copy of the pre-initialized hasher?
💬 glozow commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1661174570)
why `GetRandHash` instead of `ConsumeUint256`?
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1661174570)
why `GetRandHash` instead of `ConsumeUint256`?