💬 i-am-yuvi commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#issuecomment-2640833530)
re-ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53

💬 davidgumberg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1945326195)
This branch is unused, maybe delete if retouch

💬 theuni commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1945331641)
I wonder if this should be an `add_custom_command()` with a [`POST_BUILD` event](https://cmake.org/cmake/help/latest/command/add_custom_command.html#build-events) instead? Presumably this version (I haven't checked) creates the symlink even if the link fails, whereas I would expect a `POST_BUILD`command to not have that problem.

👍 theuni approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2599736642)
utACK 433aa99d238740bbdcb07b53b17f639170bd326d

Only change since my last review is the symlink script, which I don't feel strongly about either way. I left a comment about a fixup for it, but it's not a blocker for me.

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945336859)
That doesn't seem likely to be true? Do you mean `>= m_orphans.size()`?

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945338874)
oops, yes

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945342636)
Moved out of the loop

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945343151)
(I added that instead)

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945346739)
So `m_total_orphan_weight`, `PeerOrphanInfo::m_total_weight`?

Are `BytesFromPeer` and `TotalOrphanBytes` ok? Or do we want `WeightFromPeer` and `TotalOrphanWeight`? Or maybe `UsageByPeer` and `TotalOrphanUsage`?

💬 furszy commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-2640881605)
Concept ACK. Will review soon.

🤔 Prabhat1308 reviewed a pull request: "test: test_inv_block, use mocktime instead of waiting"
(https://github.com/bitcoin/bitcoin/pull/31811#pullrequestreview-2599781783)
tACK [2706c5b](https://github.com/bitcoin/bitcoin/pull/31811/commits/2706c5b7c8eee7ffd8c3b23a8012f346165ddb93)

Speedup is observed when running the test with the diffs.

ACK with the use of Mocktime as the test is independent from the use of wall-clock time. This can be seen by the use of `setmocktime()` in other functions in the same file as well .

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945358694)
I like `Usage` :+1:

💬 hebasto commented on pull request "Fix MSVC ccache reporting (Fixes #31771)":
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2640895236)
@vijayabhaskar78

> ### Testing
>
> ```shell
> cmake -B build -G Ninja -DCMAKE_CXX_COMPILER_LAUNCHER=ccache
> ```

All CI job failed. Does this work locally for you?

What are cache hit rates in the following scenarios:
- different build trees
- different git work-trees

?

(for more details, please see https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2331096969 and https://github.com/bitcoin/bitcoin/pull/30861)

💬 davidgumberg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1945362703)
would it make sense to move this check into `assert_is_sqlite()`?

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945367787)
m_total_orphan_usage, m_total_usage, UsageByPeer, TotalOrphanUsage?

🚀 hebasto merged a pull request: "Prepare "Open Transifex translations for v29.0" release step"
(https://github.com/bitcoin/bitcoin/pull/31809)

💬 sipa commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945350493)
Is the `try_emplace` necessary here? Given that we're immediately decrementing `peer_info.m_total_size` after, I think the assumption is that the `m_peer_orphanage_info` object for peer `peer` must already exist here, so a `find` should suffice?

💬 sipa commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945376879)
It looks like there is no accounting for the number of announcers per peer? Is the idea that we use the `m_work_set.size()` as overestimate? If so, can the `SanityCheck` verify that for every `(wtxid, peer)` announcement pair, a `wtxid` entry exists in `peer`'s `m_work_set`?

💬 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-2640929632)
Ok, updated description. It's much simpler now and should be very straightforward to review.

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945393736)
The work set isn't really tied to `m_orphans`, as it can include orphans that have been erased, so I don't think that would make sense.

I'm planning to make `m_orphan_list` a per-peer thing and use the `size()` of that as the announcement count per peer. I will add a loop to `SanityCheck()` then to ensure it is consistent with `m_orphans`.