💬 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`?
💬 glozow commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2200381417)
> Re [#30344 (comment)](https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2199482117)
>
> > lgtm, iiuc you'd want #30141 merged before this one?
>
> Mmh, I don't think this was said anywhere? Looks like the two are not really related either.
It seems they have a couple acks and conflict with each other? Was taking a guess based on https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2192453113
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2200381417)
> Re [#30344 (comment)](https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2199482117)
>
> > lgtm, iiuc you'd want #30141 merged before this one?
>
> Mmh, I don't think this was said anywhere? Looks like the two are not really related either.
It seems they have a couple acks and conflict with each other? Was taking a guess based on https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2192453113
💬 theuni commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2200413138)
@glozow We both noticed this independently. @TheCharlatan Had it queued up as part of his next batch of changes (not yet PR'd), but I didn't know that when I PR'd this one. He's since agreed that it makes sense to merge this on its own.
#30141 is unrelated, he's just waiting for that to be merged before PR'ing his next batch of kernel changes.
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2200413138)
@glozow We both noticed this independently. @TheCharlatan Had it queued up as part of his next batch of changes (not yet PR'd), but I didn't know that when I PR'd this one. He's since agreed that it makes sense to merge this on its own.
#30141 is unrelated, he's just waiting for that to be merged before PR'ing his next batch of kernel changes.
💬 ishaanam commented on pull request "#27307 follow-up: update mempool conflict tests + docs":
(https://github.com/bitcoin/bitcoin/pull/30365#discussion_r1661201441)
Done
(https://github.com/bitcoin/bitcoin/pull/30365#discussion_r1661201441)
Done
💬 ishaanam commented on pull request "#27307 follow-up: update mempool conflict tests + docs":
(https://github.com/bitcoin/bitcoin/pull/30365#discussion_r1661201934)
Done, thanks
(https://github.com/bitcoin/bitcoin/pull/30365#discussion_r1661201934)
Done, thanks
💬 glozow commented on pull request "kernel: remove mempool_persist":
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2200424459)
> https://github.com/bitcoin/bitcoin/pull/30141 is unrelated, he's just waiting for that to be merged before PR'ing his next batch of kernel changes.
I can see that they aren't related, just trying to clarify which one to merge first because the PRs conflict. I figured this would be an ok place to ask that since it's the same people. Apologies, I wasn't trying to imply that they're related at all, my previous comment was to provide an explanation for why I was asking which one to prioritize.
(https://github.com/bitcoin/bitcoin/pull/30344#issuecomment-2200424459)
> https://github.com/bitcoin/bitcoin/pull/30141 is unrelated, he's just waiting for that to be merged before PR'ing his next batch of kernel changes.
I can see that they aren't related, just trying to clarify which one to merge first because the PRs conflict. I figured this would be an ok place to ask that since it's the same people. Apologies, I wasn't trying to imply that they're related at all, my previous comment was to provide an explanation for why I was asking which one to prioritize.
📝 maflcko opened a pull request: "fuzz: Mutate -max_len= during generation phase"
(https://github.com/bitcoin/bitcoin/pull/30371)
This revives https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1927243781, because it helps to find https://github.com/bitcoin/bitcoin/issues/30367, which failed to be found for more than a year on any of the existing fuzz servers.
Locally, I can find the bug with `-max_len=84 -use_value_profile=1` on a single thread on a laptop. The reason is likely that a smaller max_len results in a faster fuzzing speed (iterations). As a side-effect it may also be effective at reducing the size o
...
(https://github.com/bitcoin/bitcoin/pull/30371)
This revives https://github.com/bitcoin/bitcoin/pull/28178#issuecomment-1927243781, because it helps to find https://github.com/bitcoin/bitcoin/issues/30367, which failed to be found for more than a year on any of the existing fuzz servers.
Locally, I can find the bug with `-max_len=84 -use_value_profile=1` on a single thread on a laptop. The reason is likely that a smaller max_len results in a faster fuzzing speed (iterations). As a side-effect it may also be effective at reducing the size o
...