💬 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-2200056770)


I don't think this is used in a critical path, to warrant a speedup?

💬 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-2200078041)
> I don't think this is used in a critical path, to warrant a speedup?

It's literally the example from the [benchmarking docs](https://github.com/bitcoin/bitcoin/blob/master/doc/benchmarking.md):
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/822e347b-0064-4343-8292-bf753530a7e9">

which was specifically optimized in various other commits before: https://github.com/bitcoin/bitcoin/commit/bcab47bc1b2bfdd29f8b89f8a211755299938aea, https://github.com/bitcoin/bitcoin/commit/159ed
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1661016997)
Because we want to ignore incoming messages after a successful handshake. Ignoring the `VERACK` would disturb the handshake.

💬 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-2200092884)
What you link is supporting my point, is it not? https://github.com/bitcoin/bitcoin/pull/12704#issuecomment-374835228

> Stop wasting time on discussing the performance. This does not matter. Decoding an address could take 50 us and I don't think anyone would notice.

> If the resulting code looks better, go for it. Otherwise, don't.

> -0

💬 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.

💬 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?

💬 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.

🚀 glozow merged a pull request: "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`"
(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.

👍 maflcko approved a pull request: "Show maximum mempool size in information window"
(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.

💬 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)?

💬 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()`.

🤔 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
...

💬 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

💬 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
...

💬 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.

💬 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.

📝 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.

💬 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/
...