💬 laanwj commented on pull request "rpc: increase the defaults for -rpcthreads and -rpcworkqueue":
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828088319)
i've always been kind of hestitant about adapting these values to the system automatically; it doesn't only depend on the CPU, but also the expected usage.

For a "fair" work queue it would indeed make sense to pick the number of CPU cores. However, bitcoin core's RPC threads often sleep for various reasons. Eg waiting for I/O, waiting for a lock in another thread, etc. So it's fine to have it a bit higher than the number of cores.
i'd definitely cap the minimum at 4, even for single-core CPU
...

👍 storopoli approved a pull request: "rpc: increase the defaults for -rpcthreads and -rpcworkqueue"
(https://github.com/bitcoin/bitcoin/pull/31215#pullrequestreview-2413651250)
Concept ACK

💬 laanwj commented on pull request "rpc: increase the defaults for -rpcthreads and -rpcworkqueue":
(https://github.com/bitcoin/bitcoin/pull/31215#issuecomment-2455273805)
ACK on increasing the default work queue size. i don't think the RPC work queue uses that much memory. It's fine to set the default a lot higher.

Not sure about the threads.

💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1828105140)
It also causes double-validation for single transaction package that is missing inputs.

Could special case this but not sure I love making things even harder to understand:
```
diff --git a/src/validation.cpp b/src/validation.cpp
index 3e48335255..c89050e895 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1790,10 +1790,16 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
// some of them may still be valid.

...

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828133259)
Done

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828135621)
note: with clang I measured ~24: https://tc-imba.github.io/posts/cpp-sso

🤔 l0rinc reviewed a pull request: "net: Use actual memory size in receive buffer accounting"
(https://github.com/bitcoin/bitcoin/pull/31164#pullrequestreview-2413727212)
Concept ACK

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828153937)
Could you please add a test in this commit that demonstrates the intended behavior? Or would that tie the implementation down too much?

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828152759)
Since I think these are related pointers (unlike https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827884684), can we use simple pointer comparisons here? Or do I misunderstand how this works?
```C++
if (s.data() >= s_ptr && s.data() + s.size() <= s_ptr + sizeof(s)) {
```

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828173154)
interesting!
that also suggests that on 32-bit platforms, the size would still be 15 bytes for g++ (because it's the minimum bound), but 10 (3*4 minus 2 for size byte and zero byte) for clang
so it would trigger an allocation for message types of 11/12 chars there 😄

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828177808)
as i understand it, in the case that sso is not used, these are not related pointers

💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828177895)
In case the small string optimization is in effect, then `s.data()` and `s_ptr` are possibly related (the real question is whether they point into the same allocated array), but if sso is not in effect then they are definitely unrelated.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828180248)
Thanks for clarifying.

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828184257)
i guess we could do some basic generalizable test like "the memory usage increases or stays equal when the string becomes longer"
but i'm not sure how useful that is, and there are currently no memusage tests to add it to

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828195177)
fwiw the underlying reason for this pointer absurdity is to support platforms with segmented memory (such as GPUs), where pointers to different types of allocations might refer to different address spaces and won't be comparable
probably, `std::greater` adds some address space prefix to give them a defined order
this is a super edge case and unlikely to come up for any platform people compile bitcoind for, but better to be safe i guess...

💬 instagibbs commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#discussion_r1828204231)
are there any easy ways to verify these lower and higher target values?

💬 instagibbs commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#discussion_r1828200914)
some pedantic assertions to catch hypothetical issues with the function earlier
```suggestion
assert(min + result >= min);
assert(min + result <= max);
return min + result;
```

💬 darosior commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#issuecomment-2455474172)
Concept ACK

💬 darosior commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1828281626)
Done.

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1828284824)
i've tested it manually (gcc, x86_64) with the following patch:
<details>
<summary>patch</summary>

```patch
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 8a4c1746161c30c04dee1f41a811740d0038df0b..f8c688dd3617123548af13611519adba59eef775 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -23,6 +23,7 @@
#include <util/strencodings.h>
#include <util/time.h>
#include <util/translation.h>
+#include <memusage.h>

#include <algorithm>
#include <chrono>
...

✅ Abdulkbk closed a pull request: "Improve lockunspent validation for vout"
(https://github.com/bitcoin/bitcoin/pull/31205)