💬 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.

🤔 marcofleon reviewed a pull request: "test: adds outbound eviction functional tests, updates comment in ConsiderEviction"
(https://github.com/bitcoin/bitcoin/pull/29122#pullrequestreview-2048227292)
Re ACK. Reviewed the code and the tests looks good to me. I ran `p2p_outbound_ eviction.py` individually and along with all the other functional tests and everything looks good.

👍 ryanofsky approved a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2048211642)
Code review ACK 6a22eede2083616ecc7558a16d8189c22b46403d. Just some suggested changes were made since the last review: adding more Assume checks, renaming a function, and moving a declaration. Everything still looks good.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1595600259)
re: https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1594531344

That makes sense. I didn't realize that. I can see how it makes the next commit more straightforward, at the cost of introducing a slightly mysterious change to this commit and adding a little more churn to the PR as a whole. Could be a good thing, as the next commit is the most complicated one, so either approach seems fine.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1595583401)
6cf4809c6b93e1720dfdfe4e3320cfd8939686b6: This is `::Warning` worthy.