📝 instagibbs opened a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072)
First few commits of https://github.com/bitcoin/bitcoin/pull/28984 to set the stage for the package RBF logic.

No behavior changes aside from bailing earlier from failed carve-outs.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2102717549)
suggested by @glozow , can close if this ends up not being helpful

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595480359)
b86487ccf2e3d43da3d3ce8c3d2e2510677eb9ec

I wonder if `PaysForRBF` and `ImprovesFeerateDiagram` should be swapped in the order of checks?

`ImprovesFeerateDiagram` is a more expensive, is a superset of the rules, and a less intuitive error (I'd have an easier time guessing what went wrong with "pays less fees" vs "does not improve feerate diagram". Especially looking at the functional tests which switched out the error strings due to this check being earlier.

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595484318)
```suggestion
return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: new transaction cannot have mempool ancestors");
```

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595472969)
in b86487ccf2e3d43da3d3ce8c3d2e2510677eb9ec

The comments + error string are a bit contradictory ("child only paying anti-DoS" and "parent paying for child anti-DoS") unless maybe I'm misunderstanding?

Also maybe error str "package RBF failed: parent paying for child replacement" and debug str "package feerate is X while parent feerate is Y" ?

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595487038)
Maybe you can mention that this replaces `PaysMoreForConflicts` function from `ReplacementChecks`?

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595494866)
Could add more detailed docs for what the rules are in package RBF?

- all to-be-replaced signal (bip125 or v3)
- package is 1p1c, only in clusters up to size 2
- to-be-replaced all in clusters up to size 2
- no more than 100 total to-be-replaced
- total fees of package > all fees being replaced, at incremental relay feerate
- must improve feerate diagram
- parent feerate must be < package feerate

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595176618)
yeah, I think `PCKG_POLICY` is fine given https://github.com/bitcoin/bitcoin/pull/30012/commits/6119f76ef72c1adc55c1a384c20f8ba9e1a01206. Just need to make sure the `MempoolAcceptResult`s are correct so that we are attributing the failures correctly

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595490481)
I don't think it's very clear which cluster isn't size two in this str
```suggestion
return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package RBF failed: package must be 1-parent-1-child");
```

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-2102723104)
Updated 65a205166b3c7d082a7528ea5036dff551d7eb6e -> 96378fe734e5fb6167eb20036d7170572a647edb ([kernelRmKey_4](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_4) -> [kernelRmKey_5](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmKey_4..kernelRmKey_5))

* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1595470766), moving comments and improving description of `ECC_Co
...

💬 ajtowns commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1595503832)
Mostly that `NUMS_H` is an XOnlyPubkey (rather than a CPubKey or the hex/bytes encoding of the point or something else), so maybe it would make sense to keep it tied to that class. Bit of a knee-jerk "does this really need to be in the global namespace" reaction.

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595504866)
> is a superset of the rules

PaysForRBF has incremental rate on top, so `ImprovesFeerateDiagram` is not a strict superset fwiw.

That said, I don't think swapping the order is a problem and would indeed be cheaper.

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595508385)
IIUC It will leave the "individual failures" in place which should be:
parent: TX_RECONSIDERABLE
child: TX_MISSING_INPUTS

💬 TheCharlatan commented on pull request "build: swap cctools otool for llvm-objdump":
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2102761167)
Guix builds (aarch64):
```
67f4db93b4f985590948dc344306042841ab747b40ad73bf97ba62d77eb7bfd4 guix-build-7f5ac4520d15/output/aarch64-linux-gnu/SHA256SUMS.part
62e019f164d3c8cf8a1a649b2caa500dd85b00a319910feaf97658020b8cd1e3 guix-build-7f5ac4520d15/output/aarch64-linux-gnu/bitcoin-7f5ac4520d15-aarch64-linux-gnu-debug.tar.gz
c2227a058cf91c0bc3eb244cb254c3c6fcecddc21ba619bca8839d9d79bb1961 guix-build-7f5ac4520d15/output/aarch64-linux-gnu/bitcoin-7f5ac4520d15-aarch64-linux-gnu.tar.gz
e794a50c8
...

🤔 rkrux reviewed a pull request: "test: add conflicting topology test case"
(https://github.com/bitcoin/bitcoin/pull/30066#pullrequestreview-2048076616)
I'm still reviewing the PR and gaining some context. For now, make is successful and all the functional tests pass on commit [fba1565](https://github.com/bitcoin/bitcoin/commits/fba1565f4b6bafbf2516f03184cf58aa80d9843f).

💬 rkrux commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1595521218)
Super nit: There's a blank line here b/w the RPC and the assertion & also on line 248, but not b/w 261-262. Consistency here would be nice.

💬 rkrux commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1595519099)
> that even if topologies
that are acceptable are relaxed, like
removing package-not-child-with-unconfirmed-parents,

Where exactly here is this topology relaxed?

💬 instagibbs commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1595528604)
It hasn't been, but has been discussed (having trouble finding the discussion on github sorry).

💬 instagibbs commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1595530688)
can do if I touch test again

🤔 willcl-ark reviewed a pull request: "[27.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/29888#pullrequestreview-2048092284)
Left one question, looks good otherwise