💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2454530700)
> maflcko
>
> > They'd have to be cleared on all machines. Let me known if you want that to happen.
>
> I believe this PR is in quite good shape, and it might be worth clearing the depends caches for CI and DrahtBot's Guix builder machines.

Sorry for the delay. I installed DrahtBot from scratch, so that should be clear. The CI may still take a few days. For now, reviewers can ignore the last commit (and just drop it from their review).

🤔 maflcko reviewed a pull request: "build: increase minimum supported Windows to 10.0"
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2412892609)
If the build system changes are too cumbersome, maybe for now, only the docs can be updated, so that at least one place reflects the reality that Bitcoin Core on Windows 7 isn't "extensively tested"?

💬 maflcko commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#discussion_r1827628041)
CI is only testing one of Windows 10, or Windows 11. So I think claiming that both are "extensively" tested may not be true, as limited feedback is available about local testing efforts. (Same for the Linux kernel and macOS).

Maybe just drop "extensively"?

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1827628848)
> In [54d81ca](https://github.com/bitcoin/bitcoin/commit/54d81cab53a07fc47ddd38d3be0384651d785558): You're removing this patch, but looking at the upstream source for 6.7.3, it's not immediately clear where this was fixed upstream, and the commit doesn't explain why it's no-longer needed. Can you elaborate?

The patched files are part of the legacy qmake-based build system and are unrelated to the new CMake-based build system.

> I'm wondering if new patches you are introducing are just a re
...

💬 naumenkogs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1827633761)
Won't we redundantly call `AcceptSubPackage` twice for a single transaction, if `individual_results_nonfinal.emplace(wtxid, single_res);` stuff is triggered?

🚀 fanquake merged a pull request: "doc: archive release notes for v27.2"
(https://github.com/bitcoin/bitcoin/pull/31208)

⚠️ fanquake opened an issue: "intermittent issue in wallet_upgradewallet.py: AssertionError: bdb magic does not match bdb btree magic"
(https://github.com/bitcoin/bitcoin/issues/31210)
https://cirrus-ci.com/task/5232872305459200:
```bash
067] node0 2024-11-04T11:58:48.064759Z [httpworker.0] [src/wallet/wallet.h:936] [WalletLogPrintf] [default wallet] m_address_book.size() = 2
[06:58:49.067] test 2024-11-04T11:58:48.065000Z TestFramework (INFO): Can upgrade to HD
[06:58:49.067] test 2024-11-04T11:58:48.066000Z TestFramework (ERROR): Assertion failed
[06:58:49.067] Traceback (most recent call last):
[06:58:49.067]
...

💬 brianacaley commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2454593535)
7


Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: Gleb Naumenko ***@***.***>
Sent: Monday, November 4, 2024 4:23:27 AM
To: bitcoin/bitcoin ***@***.***>
Cc: Subscribed ***@***.***>
Subject: Re: [bitcoin/bitcoin] p2p: Fill reconciliation sets (Erlay) attempt 2 (PR #30116)


@naumenkogs commented on this pull request.

________________________________

In src/node/txreconciliation.cpp<https://github.com/bitcoin/bitcoin/pull/30116#discussion_r
...

💬 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-2454599900)
This only reproduces on a specific machine. I'll try to take a closer look.

💬 Abdulkbk commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454613258)
> `getInt` is used in many RPCs. What makes this call to `getInt` in this RPC special?

Thank you, @maflcko, for your feedback. I believe that `getInt` in `lockunspent` is special because the vout param specifically represents transaction output indexes, which are always whole numbers. Since this value comes directly from user input, providing a descriptive error message would be very helpful.

I also updated the PR description to add motivation for this change.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827415495)
Nit: Given that `MallocUsage` warns that it `is *not* recursive`, would have been great to add a
```c++
requires std::is_trivial_v<X>
```
here, but apparently it *is* used for nested structures - nothing to do here, just documenting my journey.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827342209)
Q: `v.capacity()` can be significantly more here than `v.size()`, right?
Is it because we can't size them properly in https://github.com/bitcoin/bitcoin/blob/master/src/streams.h#L82?

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827424352)
Q: we assume that `sizeof(v)` (the vector's fixed memory) was already included in the calculation before this call, right?

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827341320)
Nit: Could we find better names than X and Y (e.g. to avoid readers thinking Y is the size), maybe something like `template<typename ElementType, typename AllocatorType>`

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827573203)
Slightly unrelated, but `MemUsage` could be generalized to explain the alignment and rounding:

<details>
<summary>Details</summary>

```C++
static inline size_t MallocUsage(const size_t alloc)
{
if (alloc == 0) return 0;
constexpr size_t alignment{2 * sizeof(void*)};
constexpr size_t round_up{alignment * 2 - 1};
constexpr size_t mask{~(alignment - 1)};
return alloc + round_up & mask;
}
```
and
```C++
BOOST_AUTO_TEST_CASE(malloc_usage)
{
auto referenceM
...

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827588117)
I don't actually understand why we're ignoring `m_type`, the comment doesn't help.

Is it because of https://github.com/bitcoin/bitcoin/blob/master/src/netmessagemaker.h#L17, i.e. the string being moved around instead of being owned?

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827592846)
we still have a few `m_raw_message_size` usages, can you please explain why that's still needed?

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827589976)
given that we're ignoring `m_type`, is it still fair to say that we're counting `total memory usage`?

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827709219)
The comment in the referred function explains it.
```
// Don't count the dynamic memory used for the m_type string, by assuming it fits in the
// "small string" optimization area (which stores data inside the object itself, up to some
// size; 15 bytes in modern libstdc++).
```
It has to do with the internal implementation of std::string, where it has a fixed-size buffer that it will use for short strings (such as the message types) before allocating memory on the heap.

💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827714339)
i do agree that over-counting is probably better than under-counting here, but i just took this over from `CSerializedNetMsg` as i assumed this was the right thing to do. If we change it it needs to be changed in both places.