💬 instagibbs 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_r1459253460)
done

💬 instagibbs 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_r1459253539)
added comment

💬 instagibbs 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_r1459253631)
done

💬 instagibbs 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_r1459253686)
done

💬 instagibbs 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_r1459253728)
done

👍 murchandamus approved a pull request: "wallet: fix coin selection tracing to return -1 when no change pos"
(https://github.com/bitcoin/bitcoin/pull/29272#pullrequestreview-1833088422)
tACK with nit: e701982c741edd2182690ddf4628e4c079a5185e

💬 murchandamus commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1459228682)
There were a couple transactions before, each sending 10 BTC to the wallet itself. So the wallet should still have a balance of 50 – fees BTC, and presumably 3 UTXOs of 10 BTC, 10 BTC, and 30 - fees BTC. I‘m not sure why APS (avoid partial spend) would make a difference here. APS just means that we’ll force use of all coins associated with the same scriptpubkey at once rather than a single UTXO if there is address reuse, but it seems to me that funds are always sent to new addresses here.

It
...

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1900694553)
As for the other commits, as mentioned previously, I am not sure if this is really needed. But no objection, of course. (https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895822246)

💬 remyers commented on pull request "wallet: fix coin selection tracing to return -1 when no change pos":
(https://github.com/bitcoin/bitcoin/pull/29272#discussion_r1459266681)
Thanks for the explanation; although the next test after this one does what you suggest, it doesn't trigger the fall-through APS path. I'll see if I can rework this test to better match a real APS situation.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1459267014)
Would have to capture it then in the lambda, which seems less clear to me.

✅ ismaelsadeeq closed an issue: "Enable `maxfeerate` and `maxburnamount` as startup config options."
(https://github.com/bitcoin/bitcoin/issues/29217)

💬 ismaelsadeeq commented on issue "Enable `maxfeerate` and `maxburnamount` as startup config options.":
(https://github.com/bitcoin/bitcoin/issues/29217#issuecomment-1900697766)
Closing this, after @glozow feedback here https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1459131869

💬 ismaelsadeeq commented on pull request "RPC: Wallet: Add `maxfeerate` and `maxburnamount` startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1900699310)
Thanks @glozow for your review.
Will put this PR in draft while addressing Approach feedback

📝 ismaelsadeeq converted_to_draft a pull request: "RPC: Wallet: Add `maxfeerate` and `maxburnamount` startup option"
(https://github.com/bitcoin/bitcoin/pull/29278)
This PR attempts to fix two issues

#29217
#29220

Firstly,
- Both `sendrawtransaction` and `testmempoolaccept` have the optional parameter `maxfeerate`.
When passed, this parameter ensures that any transactions with a fee rate higher than the specified `maxfeerate` value are rejected.

- `maxburnamount` is also an optional parameter for `sendrawtransaction`. When passed, it rejects transactions with unspendable outputs (e.g., 'datacarrier' outputs using the OP_RETURN opcode) greater t
...

💬 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;
```