💬 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

💬 willcl-ark commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/29888#discussion_r1595528448)
In: a995902d604c701be4f46087057b907de9a0ecca

This also appears to pull in fast_fixed_dta_no_optimise.patch and I think that may not be intentional based on the commit it's rebasing?:


💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1595548061)
Ah, I see. In my head, there is nothing about `H` that requires it to be an XOnlyPubKey, but you're right that we are setting it that way (and thats the only way its used in our codebase).

Seems fine to make it a member, altho needing to call something like `XOnlyPubKey{XOnlyPubKey::NUMS_H}` to use it seems kinda weird to me?

💬 Sjors commented on pull request "build: swap cctools otool for llvm-objdump":
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2102819828)
AMD Ubuntu:

```
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
e794a50c8aeebefa61
...

💬 willcl-ark commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/29888#discussion_r1595565010)
Sorry for the noise, this is just my script misbehaving.
Script confused as fast_fixed_dtoa_no_dtoa_no_optimse was removed from master in the interim which confused the diffs.

👍 ryanofsky approved a pull request: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2048184415)
Code review ACK 96378fe734e5fb6167eb20036d7170572a647edb. Just suggested comment changes since last review.

💬 ajtowns commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1595587217)
You'd write `builder.Finalize(XOnlyPubKey::NUMS_H);` which seems fine?

💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1595598118)
I will leave as is for now.