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

🤔 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)