👍 instagibbs approved a pull request: "TxOrphanage: account for size of orphans and count announcements"
(https://github.com/bitcoin/bitcoin/pull/31810#pullrequestreview-2598861794)
ACK 76af0b4d993c3a5a4a9c37f40d85b9996b38e7a0

Agree with https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944888846

and up for litigating the naming of size vs weight.

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944832866)
can we at least internally call it weight, even if externally we don't?

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944908395)
extra period?

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944903683)
nit: can assert number of announcers must be non-0 while we're here.

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944911351)
can use wtxid here

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944911842)
can use `wtxid` here

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1944927518)
while we're here could also assert `m_total_announcements >= m_total_orphan_size`

💬 Sjors commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944933102)
We should probably try to keep the output lines of `bitcoin-cli -netinfo help` below 80 characters. It doesn't wrap in a nice way.

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944940106)
Don't disagree but that would require reformatting most of the help.

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944942184)
For here kept it shorter than the longest line ("If that argument is passed...")

👍 stickies-v approved a pull request: "Prepare "Open Transifex translations for v29.0" release step"
(https://github.com/bitcoin/bitcoin/pull/31809#pullrequestreview-2599059064)
ACK 2f27c910869e301b7e7669e81a0878da64e49957

I'm not sure how necessary 864386a7444fb5cf16613956ce8bf335f51b67d5 is (has this lead to an issue before?), but the rationale makes sense, and it seems harmless and a minimal change.

I reviewed 2f27c910869e301b7e7669e81a0878da64e49957 without `depends` and with `-DWITH_MULTIPROCESS=OFF` because the sources lists of `bitcoin-qt` and `bitcoin-gui` are currently identical. Diff is the same.

💬 theuni commented on pull request "build: set build type and per-build-type flags as early as possible":
(https://github.com/bitcoin/bitcoin/pull/31711#discussion_r1944952072)
Not critically, no. I just figured it should for consistency. Is there a downside to this?

💬 theuni commented on pull request "build: set build type and per-build-type flags as early as possible":
(https://github.com/bitcoin/bitcoin/pull/31711#issuecomment-2640196265)

> The `CMAKE_BUILD_TYPE` cache variable lost its help string:
>
Ugh, presumably because it's enabling c++ that sets this?

I guess that's a downside, and there may be other side-affects as well. I suppose it'd be safer to do this after enabling c++, which means letting that check be done with the wrong build type. Which realistically shouldn't ever be a problem. Agreed?

💬 hebasto commented on pull request "build: set build type and per-build-type flags as early as possible":
(https://github.com/bitcoin/bitcoin/pull/31711#issuecomment-2640206091)
> > The `CMAKE_BUILD_TYPE` cache variable lost its help string:
>
> Ugh, presumably because it's enabling c++ that sets this?
>
> I guess that's a downside, and there may be other side-effects as well. I suppose it'd be safer to do this after enabling c++, which means letting that check be done with the wrong build type. Which realistically shouldn't ever be a problem. Agreed?

Given https://github.com/bitcoin/bitcoin/pull/31711#discussion_r1944952072, yes, I agree.

💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1944976427)
Descriptor wallets never show `iswatchonly` for individual addresses. They are only `ismine` or not. That's that whole point. The watchonly distinction lives at the wallet level, not that address level.

💬 sr-gi commented on pull request "func test: Expand tx download preference tests":
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2640221382)
This seems to have heavily increased the run time of the rest (from ~30s to ~90s on my local setup) :/

💬 sr-gi commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#issuecomment-2640225426)
Rebased to fix merge conflicts.

Thanks for the review @tdb3 @danielabrozzoni @i-am-yuvi

💬 mzumsande commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640229392)
> Running with -reindex-chainstate will leave you stuck in an infinite loop revalidating an invalid tip?

I wouldn't expect that. `ConnectTip()` will call `InvalidBlockFound()` which should mark the block index as failed, and we won't revalidate / stay at the last valid block - same scenario as if it failed the script checks.

I can see the infinite loop happening only if a previously admitted block fails with `BLOCK_MUTATED` during connection, but mutation rules shouldn't be affected by a
...

💬 maflcko commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944986256)
"i.e." is latin and stands for "id est" ("that is"). I think what you want is to given an example? That'd be "e.g.", which stands for "exempli gratia" ("for example"). See also

```
src/net_processing.cpp:/** Whether this peer can only serve limited recent blocks (e.g. because
src/net_processing.cpp- * it prunes old blocks) */
```

However, I think it is fine to not list all examples here, because the list can never be known to be complete anyway. Just adding the reference to the BIP se
...

💬 instagibbs commented on pull request "func test: Expand tx download preference tests":
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2640247828)
@sr-gi I'll take a look