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

📝 achow101 opened a pull request: "Update secp256k1 subtree to v0.6.0"
(https://github.com/bitcoin/bitcoin/pull/31216)
v0.6.0 was just released, main change is that it has the musig module which #29675 needs.

🤔 mzumsande reviewed a pull request: "consensus: fix `OP_1NEGATE` handling in `CScriptOp`"
(https://github.com/bitcoin/bitcoin/pull/29589#pullrequestreview-2413945142)
I didn’t see earlier versions of this PR, but It’s not clear to me what the goal of this PR is from the OP / Title. Should the title be updated, is this still supposed to fix a bug?

As mentioned above, `DecodeOP_N` / `EncodeOP_N` currently don't need to handle `OP_1NEGATE` in any of the current use cases - so I think that there should be some explanation why they should be changed to support it.

I noticed that `EncodeOP_N` is currently called from `PushAll` in `sign.cpp` with an `unsigned
...

💬 mzumsande commented on pull request "consensus: fix `OP_1NEGATE` handling in `CScriptOp`":
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1828276434)
This would mean that C++ (`IsSmallInteger` in solver.cpp) would have a different notion of a small interger.

💬 achow101 commented on pull request "netinfo: add peer services column and outbound-only option":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2455664500)
ACK 87532fe55856efc063cf81244800da37a015ba75

💬 murchandamus commented on pull request "[WIP] wallet: standardize change output detection process":
(https://github.com/bitcoin/bitcoin/pull/25979#issuecomment-2455667569)
Hey @furszy, is this ready for review?

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2455676532)
The questionable patch has been dropped.

My Guix build:
```
aarch64
73c719eba2956cefdad25d796b8c2a43c5831f39e3120c477f645d4aa6c95c6d guix-build-8c4cfec49fae/output/aarch64-linux-gnu/SHA256SUMS.part
15cc088261e6b823a14ccbde8f606d4c7da857af9bdb3ca98820847f25eb4982 guix-build-8c4cfec49fae/output/aarch64-linux-gnu/bitcoin-8c4cfec49fae-aarch64-linux-gnu-debug.tar.gz
4e2c83139e5bd2ed56b64143e258ea4e02ac29bb5bc64629bf63b2a8f9d32a82 guix-build-8c4cfec49fae/output/aarch64-linux-gnu/bitcoin-8c4
...

🚀 achow101 merged a pull request: "netinfo: add peer services column and outbound-only option"
(https://github.com/bitcoin/bitcoin/pull/30930)