💬 fanquake commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2175931399)
This is ready for further review.

💬 CharlesCNorton commented on pull request "fix: typo in benchmark documentation":
(https://github.com/bitcoin/bitcoin/pull/30296#issuecomment-2175943555)
> Thanks for your interest in contributing! However, as we have hundreds of pull requests, I am closing this to focus review on the others. (See [contributing guidelines on refactoring](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring)).

Understood. I'll try to get a bigger collection of typos before pulling in the future.

💬 fanquake commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2175946981)
Concept ACK - API fix without a bump seems fine.

> fanquake I see that you have been updating the bitcoin port in macports. Updating miniupnpc will break it. What do you think? Try to hold it until bitcoin-core v28?

If the miniupnpc package gets updated in macports, then pulling in the same patch that ends up being applied here (until it lands in a point release), for the bitcoin port seems like the best approach.

👋 glozow's pull request is ready for review: "doc: use TRUC instead of v3 and add release note"
(https://github.com/bitcoin/bitcoin/pull/30272)

💬 theuni commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2175976362)
Dropped the bump (will PR that separately) and rebased.

💬 fanquake commented on pull request "build: use `-no_exported_symbols` on macOS":
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2175984937)
Guix Build (aarch64):
```bash
45f5a3e38b5e2f8ef7f83f8b1e509d60cb933c52a0110dd6de6e43252869a62f guix-build-81d4dc8e8739/output/arm64-apple-darwin/SHA256SUMS.part
94e4578d894e61d6b96278a293e2c17fc19a8431bd77c996346fc7e126fc6b3e guix-build-81d4dc8e8739/output/arm64-apple-darwin/bitcoin-81d4dc8e8739-arm64-apple-darwin-unsigned.tar.gz
8f94554c9fdf356cbefd926944ece1ac39af26a80039d245e350b9194861af4a guix-build-81d4dc8e8739/output/arm64-apple-darwin/bitcoin-81d4dc8e8739-arm64-apple-darwin-unsign
...

💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1644425136)
I think these were always separated like that. But sounds good to me to move them. I have added a refactoring commit which moves the other two early checks into `ActivateSnapshot()` and then I am adding the new check there as well.

💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1644425662)
taken with minor edits

💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1644425900)
I think it's a bit overkill but it was easy to add and also shouldn't make the test considerably slower, so I added coverage for this as well.

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#issuecomment-2176054949)
> Found a crash in the `mocked_descriptor_parse` fuzz target, to reproduce:
>
> ```
> $ echo "dHIoJUJELHJhd2xlYWYoQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCLEE3LCUyNywlQjcsJTIzLCVCZCwlRkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQk
...

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2176091625)
Trivial rebase after #29015.

💬 fanquake commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2176129689)
Guix Build (x86_64):
```bash
2f879df88862b94c211670a451f442eb951a6ec963d841be683d1ee7c14cf49a guix-build-8acdf6654083/output/aarch64-linux-gnu/SHA256SUMS.part
00f163274f35e955eb5ef00cb1e6e81e828b3b7415d15f594ea61bc921c000b9 guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu-debug.tar.gz
e83b5155ab258164204fd117f7974af90b580c2f0f0da713a1066eb2caecf651 guix-build-8acdf6654083/output/aarch64-linux-gnu/bitcoin-8acdf6654083-aarch64-linux-gnu.tar.gz
21ac9b2
...

💬 paplorinc commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644498191)
I've calculated the maxPayloadSize by serializing the values that definitely overflows, subtracting the op_return data size from it, getting the baseline serialization size, which I can just subtract from (MAX_BLOCK_WEIGHT / WITNESS_SCALE_FACTOR) and get the threshold.

💬 paplorinc commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644498657)
I've done this as well as part of the above change.

💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1635494042)
> No. `LegacyDataSPKM` does not have an `IsMine()` function, so this call needs to be removed otherwise it will not compile.

I think the outcome would be worst. It would compile using the base `ScriptPubKeyMan::IsMine` function. Returning `ISMINE_NO` for all `IsMine` calls.

Still, I think we should state why this assertion removal is ok in the commit description too.

💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1644499042)
Not really an issue but this works because `TopUp` sets the descriptor range_end to 1 on the first run when the descriptor is not ranged.
As this would fail if we ever change that, what if we set the range properly in `WalletDescriptor` constructor?
E.g.

```c++
WalletDescriptor w_desc(std::move(desc), creation_time, /*range_start=*/0, /*range_end=*/1, /*next_index=*/0)
```

Also, this change doesn't seem to be related to c653f4fdbfe06 description?

💬 paplorinc commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644501633)
I would need to construct fully valid transaction for it to pass - since this doesn't have valid inputs it's considered a coinbase transaction which would need some extra data, which would be outside the scope of this validation. Since I'm only changing the outputs size and that triggers the correct error, I consider that to be enough.

📝 instagibbs opened a pull request: "fuzz: have package_rbf always make small txns"
(https://github.com/bitcoin/bitcoin/pull/30300)
hopefully resolves https://github.com/bitcoin/bitcoin/issues/30241

The fuzz target is generating a large amount of
transactions, but the core of the logic is
ConsumeTxMemPoolEntry making the mempool
entries for adding to the mempool. Since
ConsumeTxMemPoolEntry generates its own transaction "vsize", we can improve efficiency of the target
by explicitly creating very small transactionsm
reducing the hashing and mempory burden.

📝 theuni opened a pull request: "depends: bump miniupnpc to 2.2.8"
(https://github.com/bitcoin/bitcoin/pull/30301)
Drops two of our patches that have been merged upstream and adjusts the other to deal with recent changes.

Follow-up from #30283. I can't vouch for the upstream changes here.

💬 instagibbs commented on issue "fuzz: timeout/oom in package_rbf":
(https://github.com/bitcoin/bitcoin/issues/30241#issuecomment-2176231802)
opened https://github.com/bitcoin/bitcoin/pull/30300, I think this should resolve it?