💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1952480020)
Ok, I decided to spend time reviewing #31785 instead of trying to trigger this assert. Now, should merging #31785 be a blocker for this PR? If not, then I should try to trigger this assert and confirm that it is not triggered (if this is to be merged without #31785).

💬 laanwj commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2653477563)
Rebased for merge of #31022.

💬 laanwj commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2653485246)
> eaafa23 can probably be removed, or the comment should be adjusted, because time isn't used anymore in GetTime since 0000a63 and instead chrono::system_clock is used.

Oh wow that's an ancient issue, agree we can get rid of that test.

👍 laanwj approved a pull request: "test: Remove stale gettime test"
(https://github.com/bitcoin/bitcoin/pull/31846#pullrequestreview-2611675848)
ACK fa3a4eafa11ee03fccea1c2a16df5bf2ab164159
This test has ceased to be relevant long ago.

💬 Eunovo commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952506706)
I understand that the use of `reserve` here is not critical, but, I generally use `reserve()` whenever I know the size of the vector beforehand in line with best practices

🤔 l0rinc requested changes to a pull request: "Add -pruneduringinit option to temporarily use another prune target during IBD"
(https://github.com/bitcoin/bitcoin/pull/31845#pullrequestreview-2611477974)
Before merging I would like to run an IBD, a reindex and an assumeUTXO on the branch.

My main concern currently (or rather the part I don't yet fully understand) is that if ["during IBD there shouldn't be reorgs in the first place"](https://bitcoincore.reviews/20827#l-122) (given a reliable block-header-first sync), why is this even configurable, why not always skip storage of blocks and undos completely during IBD (or at least reduce it to 6 block or something trivial) when prune is enabled
...

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1952422473)
Does `unused` mean that the optional is never empty because we've just checked `IsArgSet`? Given that it's an optional, we could include it directly in the condition instead:
```suggestion
if (auto prune_during_init{args.GetIntArg("-pruneduringinit")}) {
const auto pdi_parsed{ParsePruneOption(*prune_during_init, "-pruneduringinit")};
```

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1952384039)
I know the extra space was there before, but it might be time to fix it now that we're touching it:
```suggestion
return util::Error{strprintf(_("%s configured below the minimum of %d MiB. Please use a higher number."), opt_name, MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024)};
```

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1952394413)
is this compatible with `-fastprune`? If so, we should cover the new feature with tests, separating IBD pruning from up-to-date pruning (maybe in `feature_index_prune.py`)

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1952382570)
In many places in the codebase we use bit shifts for Kib/MiB/GiB, consider using them here as well:
```suggestion
const uint64_t nPruneTarget{uint64_t(nPruneArg) << 20};
```

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1952399152)
can we clarify what disabling this feature means exactly? Can we rather add a default value for minimal blocks kept during IBD instead of disabling it by default?

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1952433232)
[`MIN_DISK_SPACE_FOR_BLOCK_FILES`](https://github.com/bitcoin/bitcoin/blob/master/src/validation.h#L74) docs also mention block and undo data - should we add orphan blocks as well in the description here?

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1952396257)
how many block do we absolutely need (for reorgs, I guess) during IBD?

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1952409701)
Could you please rebase the build to make sure the latest tests are passing?
It's ~7 months old and the build system is complaining when switching branches.

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1952440159)
To obviate that `prune_parsed` has a very narrow scope here we could encapsulate it into an if/else:
```suggestion
if (auto prune_parsed{ParsePruneOption(nPruneArg, "Prune")}) {
opts.prune_target = *prune_parsed;
} else {
return util::Error{ErrorString(prune_parsed)};
}
```

💬 hebasto commented on pull request "depends: Use `CC_FOR_BUILD` for `config.guess `":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2653582453)
Rebased to resolve the conflict with the merged bitcoin/bitcoin#30584.

💬 hebasto commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-2653604048)
Rebased due to the conflict with the merged bitcoin/bitcoin#31818.

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1952580350)
Ok! that was my suggestion offline too. Bounding announcements means we bound the workset :+1:

📝 s373nZ opened a pull request: "[WIP] build: name install targets as components"
(https://github.com/bitcoin/bitcoin/pull/31847)
Adds component names to the CMake install targets to allow for component-based installs. Such installs are helpful when a user only wants to install `bitcoind` or `bitcoin-cli` executables specifically, for example.

At present, the component names use Pascal case to follow the existing convention for the only pre-existing named component `Kernel`, however, there is a convincing case to be made for using kebab-case to more intuitively match the names of the executable binaries.

Component ba
...

💬 l0rinc commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2653664441)
Given that this seemed like a change that would affect IBD, I ran a few reindex-chainstates and a few full IBDs to check for regressions.
The reindexes are usually very stable so I only ran 2 iterations of before and after, comparing a full reindex-chainstate until 880k against master.

<details>
<summary>reindex details</summary>

```bash
c59cbba518 update comment on MinimumChainWork check
ece9ac177d qa: sanity check ConnectBlock now checks all rules

COMMITS="c59cbba518fc8835b9b26040
...