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

💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#issuecomment-2102986141)
cc: @virtu @fjahr

📝 fanquake opened a pull request: "contrib: use ENV flags in get_arch"
(https://github.com/bitcoin/bitcoin/pull/30074)
This isn't an issue right now (because the get_arch check is simple), but becomes one as soon as we want to use `lld` for linking, and need LDFLAGS (otherwise we call `ld` and fail, see it's usage in #21778). So I've split this out for review. It also makes sense to use the same flags for all compilation in these checks.

Also drops some dead code in test-symbol-check.

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2102992159)
Redone this to be less convoluted. No-longer dropping our native LLVM Clang in here; just switching to LLD, dropping cctools & libtapi. Will do Clang in a followup, it gets too messy otherwise. Currently based on #29739 and #30074. Can split the other cctools changes if wanted, but they are pretty simple. The qrencode AR change likely needs to be applied globally in depends.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595684897)
done

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595685042)
added comment

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595685102)
done

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595701850)
fwiw that test is a single tx rbf, so even without this line it should pass. With package RBF it might result in multiple `RemoveStaged` calls on same conflicts, though I'm unsure what exactly that would do.

👍 theuni approved a pull request: "build: swap cctools otool for llvm-objdump"
(https://github.com/bitcoin/bitcoin/pull/29739#pullrequestreview-2048381271)
ACK 7f5ac4520d1553170b1053a9ffcd58179386a6d2. Tested `make deploy` on native macOS. Looks good.