💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827825362)
Lol, that was a stupid mistake, lemme' try again:
```C++
BOOST_AUTO_TEST_CASE(sso_size_test)
{
auto is_sso{[](const std::string& s, const CSerializedNetMsg& msg) {
const auto msg_start = reinterpret_cast<const char*>(&msg);
const auto msg_end = msg_start + sizeof(msg);
return msg_start <= s.data() && s.data() <= msg_end;
}};

const CSerializedNetMsg empty_msg;
BOOST_CHECK(is_sso(empty_msg.m_type, empty_msg));

CSerializedNetMsg msg_with_type;
msg_with_
...

✅ knst closed an issue: "guix: failure on Kubuntu 24-10: error: mount: mount "none" on "/tmp/guix-directory.VEMlin": Permission denied"
(https://github.com/bitcoin/bitcoin/issues/31202)

💬 knst commented on issue "guix: failure on Kubuntu 24-10: error: mount: mount "none" on "/tmp/guix-directory.VEMlin": Permission denied":
(https://github.com/bitcoin/bitcoin/issues/31202#issuecomment-2454882552)
Removing app-armor and re-installing guix + reboot helped, thanks for quick help!

P.S. I guess I'm moving to Manjaro; a broken AppArmor package out of the box for 2 continuous releases is disappointing.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1827857362)
Oof, this was making it a lot harder to hit the various cases! Fixed!

💬 l0rinc commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#issuecomment-2454914258)
> I don't understand why M1 Macs would have more ops after

My understand is that when we've reused the hash as input, it could only run a single instance since each one depended on the previous one. The new way is more predictable and clang often optimizes more aggressively, I guess it unrolls the instances. I can investigate if you think it's important.

> refresh the results anyway after latest push

Update the description, thanks.

💬 Abdulkbk commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454922674)
> Every use of `getInt` represents a whole number. This is not special. If you disagree, it would be good to explain and to provide an example. Also, What real-world end-user-facing issue is this trying to solve?
>
> Tend toward NACK for now, unless there is a reason.

I am new to the codebase and learned that every use of `getInt` represents a whole number after you mentioned it.

For the end users facing this issue, one example I can give, those using `bitcoin-cli lockunspent`, may enco
...

💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827872630)
Done. Let me know if you're not happy with the wording.

💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827873746)
Thanks, removed the ExpandDescriptor bench as well. Will comment on how I picked the benches soon.

📝 marcofleon opened a pull request: "fuzz: Fix difficulty target generation in `p2p_headers_presync`"
(https://github.com/bitcoin/bitcoin/pull/31213)
In the `p2p_headers_presync` fuzz target, this assertion failed:
```
assert(total_work < chainman.MinimumChainWork());
```
The test previously used `ConsumeIntegralInRange` to generate header difficulty targets within a hardcoded range. The fuzzer found specific values in that range that correspond to very low thresholds due to how [`SetCompact`][setcompact-link] works. The total work of a long enough test chain ended up exceeding `MinimumChainWork`.

Fix this by adding a new `ConsumeAri
...

💬 instagibbs commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#issuecomment-2454954020)
To be clear, `nBits` is "deterministic" in that the value, given the chain history, is static, right?

💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827884684)
@laanwj If you were to actually incorporate that into this PR, be aware you'll need to use `!std::greater{}(a, b)` instead of `a <= b` when comparing unrelated pointers like this (using `<=` for that is technically unspecified behavior, though it works fine in every real compiler IIRC).

💬 maflcko commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454965521)
> For the end users facing this issue, one example I can give, those using `bitcoin-cli lockunspent`, may encounter confusion when they un/intentionally pass a floating-point vout and receive the error "JSON integer out of range."

If the error message is confusing, it would be a reason to improve the error message. However, this applies to all RPCs and changing a single RPC seems inconsistent and confusing.

💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2454986180)
I'm not sure if using the priority levels is a good alternative to deleting the benches. Afaict, the priority level was introduced to avoid running slow benchmarks in the CI as apposed to indicating how "important" they are. As a result, we're not catching issues with these tests until someone executes them (e.g. https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852140217) and I'd prefer not to expand on that.

💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1827906033)
you're right, relatively harmless, but pushed a change which means it won't try package evaluation unless `txns_package_eval` is larger than size 1.

💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2455019735)
rebased on master due to test conflict

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827932382)
> Lol, that was a stupid mistake, lemme' try again:

i'm kind of hestitant to add a test like that because i'm not sure we want to require this behavior from the C++ library. It's perfectly valid to compile bitcoind with a library that doesn't do it, it should just take it into account.

> If you were to actually incorporate that into this PR, be aware you'll need to use !std::greater{}(a, b) instead of a <= b

pointer arithmetic is so scary nowadays...

Would this be safe?
```
bool da
...

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827934472)
Thanks for the hints, @sipa!

According to https://en.cppreference.com/w/cpp/utility/compare/compare_three_way, this also seems safe to use:
```c++
return (msg_start <=> s.data()) <= 0 && (s.data() <=> msg_end) <= 0;
```
or
```c++
return (msg_start <=> s.data()) != std::strong_ordering::greater
&& (s.data() <=> msg_end) != std::strong_ordering::greater;
```

💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827938447)
@laanwj My understanding is that that is correct (see https://en.cppreference.com/w/cpp/language/operator_comparison#Pointer_total_order; `std::greater{}` constructs an object of type `std::greater<void>`).

💬 marcofleon commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#issuecomment-2455038583)
> To be clear, `nBits` is "deterministic" in that the value, given the chain history, is static, right?

Hmm I'm not entirely sure what you're asking. `nBits` does always start with the genesis block value, yes. Then it'll either change or stay the same according to the fuzzer. Is that what you meant?

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827943096)
> i'm kind of hestitant to add a test like that because i'm not sure we want to require this behavior from the C++ library.

I understood the memory calculations will be incorrect if this would fail - is that not the case?