💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1459279757)
> ensure future ValidationSignalsImpl methods can access all private members of the class, not just some of them.

I don't follow here. `ValidationSignalsImpl` is not a friend of `ValidationSignals`, so I'm not sure which access you are talking about here. Moving it just seems more SOLID to me and I am not sure how this is violating the pimpl pattern, but I feel like I am missing something.

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459309396)
Suggested change that (I believe) avoids the need for suppressions (and may be more readable?):

```c++
auto add_fn = [&](uint64_t v, int pos) {
uint64_t accum{0};
for (int i = 0; i + pos < 4; ++i) {
// Add current value at limb pos in ret.
accum += ret[3 - pos - i];
// Add low or high half of v.
if (i == 0) accum += v & 0xffffffff;
if (i == 1) accum += v >> 32;
// Store lower half of result in
...

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459315118)
Self-nit: no need for this `namespace` anymore.

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459264030)
Nit: perhaps this function belongs more in util/feefrac (as the building of the diagram also lives there)

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459282903)
Nit: EMBRACE THE SPACESHIP

```c++
if (a[i] != b[i]) return a[i] <=> b[i];
```

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459312210)
Avoid the need for a suppression:

```c++
// Compute absolute values.
uint64_t abs_a1 = static_cast<uint64_t>(a1), abs_a2 = static_cast<uint64_t>(a2);
uint64_t abs_b1 = static_cast<uint64_t>(b1), abs_b2 = static_cast<uint64_t>(b2);
// Use (~x + 1) instead of the equivalent (-x) to silence the linter; mod 2^64 behavior is
// intentional here.
if (a1 < 0) abs_a1 = ~abs_a1 + 1;
if (a2 < 0) abs_a2 = ~abs_a2 + 1;
if (b1 < 0) abs_b1 = ~abs_b1 + 1;
if (b2 < 0) abs_b2 = ~abs_b2 + 1;
```

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459283849)
Self-nit: `2^64` -> `2^32`

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459316281)
Self-nit: this operator can be dropped (C++20 will automatically generate it as negation of `operator==`).

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459317381)
Self-nit: `operator<`, `operator>`, `operator<=` and `operator>=` can be dropped (C++20 will autogenerate them using `operator<=>` (inspecting the compiled code, it's marginally less efficient, but all the actually performance-critical uses use `operator<<`, `operator>>`, or `FeeRateCompare` anyway).

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459279118)
I can think of one use case where negative sizes are actually useful, though it's pretty far fetched.

Currently the code hardcodes a "tail feerate" (the feerate of which we assume there is an infinite supply) of 0 sat/vB for mempool improvement checks, but in theory this can be configurable. In that case, in addition to the diagram check between old and new package, we want the property that `fee_new >= fee_old + (size_new - size_old) * feerate_tail`. If `old`, `new`, and `tail` are CFeeFrac
...

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1459281387)
Nit: std::array.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1459324535)
Leaving this for now. The pattern was suggested here https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455681352 and I think it makes sense to apply it to the scheduler as well.

👍 stickies-v approved a pull request: "[26.x] more backports"
(https://github.com/bitcoin/bitcoin/pull/29209#pullrequestreview-1833242405)
ACK 11f3a7e6baf145360190635f47b1fb371fb38912

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1900772624)
Updated 492ff2c504d1da3f4ae8704a5485a286ee76f0d9 -> f442a3a5b2a0a58bc263fbb9c87e8e4715de103a ([noGlobalSignals_6](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_6) -> [noGlobalSignals_7](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_6..noGlobalSignals_7))

* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1459116286), using appropriate variable f
...

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1900781426)
CI is green btw :green_circle:

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1459376293)
as much a sense as "1". Could have minimal relay size here instead?

🤔 glozow reviewed a pull request: "refactor: remove CTxMemPool::queryHashes()"
(https://github.com/bitcoin/bitcoin/pull/29260#pullrequestreview-1833296999)
ACK 282b12ddb0aeb1f0991dd9f45c6b5c5c079652ec. Looks like there's no conflicts.

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1459420366)
should be mitigated?

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1459420447)
done

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1459420556)
done