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

🤔 Sjors reviewed a pull request: "net: Replace libnatpmp with built-in PCP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2048184349)
Regarding gateway discovery on macOS and Windows, the NatPMP C code is probably as good a start as any: https://github.com/miniupnp/libnatpmp/blob/master/getgateway.c

I tested that `-natpmp` works (before this PR) on macOS 14.4.1.

You already implemented the `USE_PROC_NET_ROUTE` approach, which unfortunately is the shortest of them.

💬 achow101 commented on pull request "test: Assumeutxo: ensure failure when importing a snapshot twice":
(https://github.com/bitcoin/bitcoin/pull/29973#issuecomment-2102932857)
ACK b259b0e8d360726b062c4b0453d1cf5a68e1933f

🚀 achow101 merged a pull request: "test: Assumeutxo: ensure failure when importing a snapshot twice"
(https://github.com/bitcoin/bitcoin/pull/29973)

💬 achow101 commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-2102947274)
ACK 6abe772a17e09fe96e68cd3311280d5a30f6378b

💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595524133)
Worth commenting that even though clearing happens in `ClearSubPackageState`, this is cleared so that we don't process the vector over and over again for each tx in the package.

fwiw here's a test for it, doing an RBF (but not package RBF) within package eval: https://github.com/glozow/bitcoin/blob/2024-05-review-30072/src/test/txpackage_tests.cpp#L940-L996

🤔 glozow reviewed a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2048080004)
ACK ff7c71f2105. Code review + checked that the members get cleared properly in between subpackage evals.

💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595529719)
Don't think the other transactions help
```suggestion
# ...or if it's submitted with other transactions
```

💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595521008)
```suggestion
CAmount m_total_modified_fees{0};
/** Aggregated virtual size of all transactions, used to calculate package feerate. */
int64_t m_total_vsize{0};
```

🚀 achow101 merged a pull request: "contrib: Add asmap-tool"
(https://github.com/bitcoin/bitcoin/pull/28793)

💬 achow101 commented on pull request "test: Assumeutxo: snapshots with less work should not be loaded":
(https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-2102949311)
Needs rebase (for some reason Drahtbot isn't detecting this)

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595654005)
True, it's only a superset of Rule 3 👍