💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827414092)
e942f3a7493908988eea08640d87b2ae420c5eef

This entire paragraph currently seems to me more confusing than useful. I suggest dropping it altogether.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827487325)
c790aa0e7ebfcf0b03b4d0ef5fcc0ddfa4199943
This is probably the most important log in the entire file, and I think it's better to distinguish two set sizes here (preferably other places the size is logged too).

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827451812)
Let's provide a hint why fanouting the descendants (or at least send a reader to where the dependency issue is explained better — we should have a one place explaining it)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827477226)
c790aa0e7ebfcf0b03b4d0ef5fcc0ddfa4199943

I think "upon trickle" or "see `m_next_inv_send_time`" is much less confusing.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827463923)
Something worth documenting too :) Again, at least one place where we talk about handling dependencies.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827467881)
There could be mempool parents, but none are in reconciliation sets (say, they were already reconciled). For all these cases, it would be nice to not choose the peer sitting first in `m_states`?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827454877)
The fix should be done in ` p2p: Deal with shortid collisions for reconciliation sets `, please move it if you retouch.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827481810)
I thought `size_t 0` will be cast to `bool false` on return, and just suggested the exact same behavior (1==true, 0==false) but without forcing the reader to think about casting and cause this exact confusion between us.
Am i wrong?

-------

Ahhh, so you saying `2==true` is also important? Then I'll be fine with explicit `return removed >= 1`, and comment that `2` should not normally happen.

💬 hebasto commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2454415368)
> Should probably remove this line from the docs:

Thanks! Amended.

💬 hebasto commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#issuecomment-2454426896)
> > Additionally, the `cmake` has been removed from the required packages,
> > as it is no longer specific to depends.
>
> If `cmake` is needed to build depends it should remain listed. The same as `make`, or a compiler.

Thanks! Reworked per feedback.

💬 fanquake commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1827573255)
If you're adding `g++` here, you can remove it from the macOS section below.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2454451603)
@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.

Shall we proceed with that?

📝 fanquake opened a pull request: "doc: archive release notes for v27.2"
(https://github.com/bitcoin/bitcoin/pull/31208)
Archive `v27.2`.

💬 hebasto commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1827577630)
Thanks! Removed.

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1827586383)
Ok. Then can you update the patch with all the missing context? This still seems to just be working around an unidentified bug, which I'm assuming Qt would want to fix. i.e moving `#include <experimental/source_location>` down 1 line. I assume this "fixes" something be shifting the include into a non-macOS-cross taken path, but that isn't obvious from the patch, and if this patch needed rebasing (on top of changed code) in the future, it's not clear how to fix what will likely be obscure compile
...

💬 hebasto commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2454476561)
Rebased.

Addressed https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2446497505.

Undrafted.

👋 hebasto's pull request is ready for review: "build: Unify `-logsourcelocations` format"
(https://github.com/bitcoin/bitcoin/pull/30811)

💬 willcl-ark commented on pull request "ci: Place datadirs for tests under tmpfs ":
(https://github.com/bitcoin/bitcoin/pull/31182#issuecomment-2454477097)
That is odd, and a little disappointing. The code appears correct to me.

I was wondering whether `tmpfs` according to docker might just mean "ephemeral" (but still disk-based), but it appears that it it **is** both ephemeral and ramdisk based as per the underlying filesystem `tmpfs`.

I will run the CI on my own self-hosted runner, and see what sort of numbers I get from it...

💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2454487414)
re-ACK c189eec848e3c31f438151d4d3422718a29df3a3

Only change is to remove mention of BIP-125 in `doc/bips.md`.

💬 laanwj commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#discussion_r1827601126)
Its... imo a bit strange to mention skipping g++ as it's so clearly a dependency for building bitcoin core itself. While correct in a strict sense, having it not installed is going to fail later.
Also the multiprocess lib (and deps) require a C++ compiler.