💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1661030282)
Note that this would only be reached if `m_by_priority` contains txid that does not have an entry in `m_by_txid` which is a gross bug in the `PrivateBroadast` class. IIRC this was an assert before but somebody suggested to use `Assume()` instead.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1661030282)
Note that this would only be reached if `m_by_priority` contains txid that does not have an entry in `m_by_txid` which is a gross bug in the `PrivateBroadast` class. IIRC this was an assert before but somebody suggested to use `Assume()` instead.
💬 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-2200101421)
What's the point of the benchmarks in that case exactly?
Anyway, I can make the code slightly simpler and similarly performant, would that be welcome?
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200101421)
What's the point of the benchmarks in that case exactly?
Anyway, I can make the code slightly simpler and similarly performant, would that be welcome?
💬 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-2200102096)
Ok, the reason for https://github.com/bitcoin/bitcoin/pull/7656#issue-139438690 was an improvement in `listunspent`. Seems fine, if this is still the case. But this will need to be checked first.
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200102096)
Ok, the reason for https://github.com/bitcoin/bitcoin/pull/7656#issue-139438690 was an improvement in `listunspent`. Seems fine, if this is still the case. But this will need to be checked first.
🚀 glozow merged a pull request: "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`"
(https://github.com/bitcoin/bitcoin/pull/30237)
(https://github.com/bitcoin/bitcoin/pull/30237)
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661038246)
We don't want to throw an error if `fOk` is false. This happens in tests when using mock parent caches that just return `false` and don't unset any flags.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661038246)
We don't want to throw an error if `fOk` is false. This happens in tests when using mock parent caches that just return `false` and don't unset any flags.
👍 maflcko approved a pull request: "Show maximum mempool size in information window"
(https://github.com/bitcoin-core/gui/pull/825#pullrequestreview-2151372594)
lgtm
(https://github.com/bitcoin-core/gui/pull/825#pullrequestreview-2151372594)
lgtm
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661061816)
Yeah, my suggestion was completely off here, thanks for checking.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661061816)
Yeah, my suggestion was completely off here, thanks for checking.
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661067593)
Does it make sense to check what the main compilers do in this case (the ones I checked via godbolt automatically inline), or was that just a waste of time, since seemingly unrelated changes could change the outcome (but if that's the case, shouldn't we let the compiler decide)?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661067593)
Does it make sense to check what the main compilers do in this case (the ones I checked via godbolt automatically inline), or was that just a waste of time, since seemingly unrelated changes could change the outcome (but if that's the case, shouldn't we let the compiler decide)?
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1661067868)
Replaced with `NodeClock::now()`.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1661067868)
Replaced with `NodeClock::now()`.
🤔 Sjors reviewed a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2151386338)
I'm still a bit confused on how to use this, e.g. in #30332.
The first step is to override `CreateSocket()` in `sv2_template_provider_tests.cpp`, so that when the `TemplateProvider` calls it we get a mock socket.
But how? Something like:
```cpp
CreateSock = [](int, int, int) {
return std::make_unique<DynSock>(...);
};
```
The test contains a `TPTester` helper which has `std::unique_ptr<DynSock> m_peer_socket;` to represent the other side of the connection.
This is inialit
...
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2151386338)
I'm still a bit confused on how to use this, e.g. in #30332.
The first step is to override `CreateSocket()` in `sv2_template_provider_tests.cpp`, so that when the `TemplateProvider` calls it we get a mock socket.
But how? Something like:
```cpp
CreateSock = [](int, int, int) {
return std::make_unique<DynSock>(...);
};
```
The test contains a `TPTester` helper which has `std::unique_ptr<DynSock> m_peer_socket;` to represent the other side of the connection.
This is inialit
...
💬 Sjors commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1661049774)
06b21ab6cb868f6b1e41e36a1f3a4d4c9c51558b: is this still useful now that you have `GetBytes`? It seems it's the job for `Transport` to turn bytes into a `CNetMessage`.
Same question for `PushNetMsg` below.
See earlier discussion: https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1494319538
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1661049774)
06b21ab6cb868f6b1e41e36a1f3a4d4c9c51558b: is this still useful now that you have `GetBytes`? It seems it's the job for `Transport` to turn bytes into a `CNetMessage`.
Same question for `PushNetMsg` below.
See earlier discussion: https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1494319538
💬 sipa 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-2200192720)
@paplorinc The reason you're getting some pushback on your PRs is because review is the biggest bottleneck for our project, which makes contributors and maintainers weary about proposed changes that demand review (which could be spent elsewhere) but don't result in observable benefits (even if they improve microbenchmarks). I hope you don't conclude from this that we don't care about performance, but it may take some experience with the codebase and the project to figure out what people consider
...
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200192720)
@paplorinc The reason you're getting some pushback on your PRs is because review is the biggest bottleneck for our project, which makes contributors and maintainers weary about proposed changes that demand review (which could be spent elsewhere) but don't result in observable benefits (even if they improve microbenchmarks). I hope you don't conclude from this that we don't care about performance, but it may take some experience with the codebase and the project to figure out what people consider
...
💬 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-2200208477)
Thanks @sipa, appreciate your comment!
How do I find small changes that are needed, welcome, not so controversial, and don't need 3 years of up-front studying?
That's why I like optimizations, since the desired behavior is already in the code and test and benchmarks and history, and I can have visible progress while learning about the project.
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200208477)
Thanks @sipa, appreciate your comment!
How do I find small changes that are needed, welcome, not so controversial, and don't need 3 years of up-front studying?
That's why I like optimizations, since the desired behavior is already in the code and test and benchmarks and history, and I can have visible progress while learning about the project.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661090068)
I think not inlining would mean I would also have to add these methods to the implementation file instead of the header.
Since you're checking assembly, is there any difference if I add `noexcept` to these methods? I can't find a definitive answer on if that will make any difference.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1661090068)
I think not inlining would mean I would also have to add these methods to the implementation file instead of the header.
Since you're checking assembly, is there any difference if I add `noexcept` to these methods? I can't find a definitive answer on if that will make any difference.
📝 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
...