🤔 tdb3 reviewed a pull request: "test: Validate oversized transactions or without inputs"
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-2125235394)
Thanks.
Most of the updates look great. Left a question.

💬 pythcoiner commented on issue "docs: Wrong/outdated docs for `tr(KEY)` in doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/issues/30279#issuecomment-2175909656)
> There it would also be great to expand the docs and link to https://bitcoin.sipa.be/miniscript/ (afaik there is no BIP for that).

There is re [recent PR](https://github.com/bitcoin/bips/pull/1610) in BIP repo.

💬 sipa commented on issue "docs: Wrong/outdated docs for `tr(KEY)` in doc/descriptors.md":
(https://github.com/bitcoin/bitcoin/issues/30279#issuecomment-2175913472)
I agree with @benma. I think we should make the descriptors.md file more explicit, but also link to the relevant BIPs now.

👋 fanquake's pull request is ready for review: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876)

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