💬 maflcko commented on pull request "test: use context managers and add file existence checks in feature_fee_estimation.py":
(https://github.com/bitcoin/bitcoin/pull/31030#issuecomment-2454151996)
Closing for now, due to lack of progress. If you still want to work on this, please leave a comment to have it re-opened. Alternatively, you can open a new pull request with the outstanding feedback addressed.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827403107)
Yeah just dropping `HasCollision`, keeping `HasCollisionInternal`. The former is used only in tests. And since its implementation is trivial, i think testing for collisions through `AddToSet` is sufficient.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827404289)
Right, please resolve this issue.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1827406620)
My impression was that there are very-very few `reviewers already familiar with the original approach` at the moment, so it's an extra burden for most to understand that former code you drop anyway.
But yeah it's ok. Perhaps a commit message saying it will be dropped will work.

💬 fanquake commented on pull request "depends, doc: List packages required to build `qt` package separately":
(https://github.com/bitcoin/bitcoin/pull/31192#issuecomment-2454215099)
> 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.

🤔 hodlinator reviewed a pull request: "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues"
(https://github.com/bitcoin/bitcoin/pull/30349#pullrequestreview-2412639608)
Concept ACK 42066f45ff5d48e78a317eda63c035809bd657c6

More realistic to also change `k0` and `val`. (The modification of the `uint256 val` only happens for one byte at a time, so benchmark time shouldn't be that affected by some unrelated expensive instructions / memory access :+1: ).

### UBSAN Repro

Managed to repro [maflcko's concern](https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1824325029) on older push c88b1bde7500ace70a694f36a82521f92221936c:
```
₿ UBSAN_OPTIONS="supp
...

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

Duplicates the comment around `MAX_RECONSET_SIZE`. One of them we better dop.

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

"Would fail" is very confusing. "Failed reconciliation" will have a technical meaning in the latter commits. Here, i'd rather be more explicit "two transactions mapped to the same short-id won't be announced between these two peers".

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