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

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827945717)
https://en.cppreference.com/w/cpp/container/vector uses
```
template<
class T,
class Allocator = [std::allocator](http://en.cppreference.com/w/cpp/memory/allocator)<T>
> class vector
```
so let's go with those names

💬 maflcko commented on issue "intermittent issue in wallet_upgradewallet.py: AssertionError: bdb magic does not match bdb btree magic":
(https://github.com/bitcoin/bitcoin/issues/31210#issuecomment-2455067581)
Steps to reproduce on a fresh Ubuntu 24.04 on that machine:

`export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./b-c && cd b-c && apt install build-essential cmake pkg-config libevent-dev libboost-dev libsqlite3-dev libdb++-dev python3 -y && cmake -B ./bld-cmake -DBUILD_TESTS=OFF -DWITH_BDB=ON -DENABLE_WALLET=ON -DBUILD_GUI=OFF -DBUILD_FUZZ_BINARY=OFF -DBUILD_BENCH=OFF -DWIT
...

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827968959)
> I understood the memory calculations will be incorrect if this would fail - is that not the case?

Yes, unless we add the logic to `memusage.h`. i guess i've always seen memusage more as a "best effort estimate" than something that needs to be correct to the byte, because it's so hard to guarantee across implementations, we don't really want to extend our scope to C++ library implementation specifics. But i dunno.

BTW: It looks like currently. memusage.h doesn't handle `std::string` (nor
...

📝 hebasto opened a pull request: "guix: Do not set `C{PLUS}_INCLUDE_PATH` variables for Darwin builds"
(https://github.com/bitcoin/bitcoin/pull/31214)
When cross-building for macOS, the `C{PLUS}_INCLUDE_PATH` environment variables modify the default include directories for both native GCC and cross Clang compilers, which is overkill and undesirable for the latter.

This change avoids setting the `C{PLUS}_INCLUDE_PATH` environment variables and instead sets the required `-isystem` flags for the native compiler explicitly.

Additionally, this PR helps avoid non-trivial [patching](https://github.com/bitcoin/bitcoin/blob/add3067e766db6f6732836
...

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1827978920)
Please see https://github.com/bitcoin/bitcoin/pull/31214 as a more clean (as I believe) alternative.