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

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1595669798)
In commit "blockstorage: split up FindBlockPos function" (2b6d274af050ea21e39ce59b6a6b3b7fb61e8cbd)

I think this comment is not really accurate in this context. There should be no need to reset `BlockfileCursor::undo_height` here, only to set a new `BlockfileCursor::file_num` value so `MaxBlockfileNum()` returns the right thing. So the comment could be changed to something like "update the cursor so it points to the last file".

Maybe it would also make sense to make this a little more robu
...