💬 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

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

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1459420739)
did the minimal suggested work to give knapsack a fighting chance

💬 murchandamus commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1459477415)
At second glance, lemme retract my comment. It doesn’t really hurt to allow values down to 0, even if that is less than a sensible use of that parameter. Might be helpful in tests to be able to restrict it fairly low.

💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1900883607)
> I do not fully get it. Can somebody elaborate what is that? With an example of what could go wrong if this "safety buffer" is ignored.

I think one reason is that the estimation in `IsInitialBlockDownload()` (or now `ApproximateBestBlockDepth()`) is not precise because block header timestamps could be wrong (within limits).
So if we used something very close to 2 days, we could think we are just out of IBD and connect to lots of `NODE_NETWORK_LIMITED` peers, which would not serve us the nex
...

🤔 murchandamus reviewed a pull request: "Add max_tx_weight to transaction funding options"
(https://github.com/bitcoin/bitcoin/pull/29264#pullrequestreview-1833400688)
ACK 069c1652ed22b5e22537f63946cedd6c68487b22

Thanks for cleanly distinguishing `max_tx_weight` and `max_input_weight`!

💬 murchandamus commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1459483691)
Ah, yeah `max_input_weight` is a much better name. Thanks!