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

💬 l0rinc commented on pull request "test: Remove stale gettime test":
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2653744003)
ACK fa3a4eafa11ee03fccea1c2a16df5bf2ab164159

Your explanations make sense to me here.

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952673009)
It's good general practice and it documents the code as well, e.g:
```
std::vector<CKey> keys{coinbaseKey};
keys.reserve(num_taproot + num_nontaproot + 1);
````

💬 fanquake commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2653761199)
> Looks like everything matches:

I've run a build, but don't yet see everything matching:
```bash
find guix-build-096525e92cc2/output -wholename "*codesigned*" -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
207621cdc43868870f4136e9e6784a2a3e9ba89ec1edc6fa92b315cfa3c4432c guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/SHA256SUMS.part
111016205f0a2ac732feb934acb3e8a36d5251f119d8fa9215790310ba46c31d guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/bi
...

💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2653785671)
@pinheadmz yes that did the trick.

When running code sign I do get lots of warnings:

```
WARNING: Part of the file was not parsed: 4332 bytes
```

I get the same hashes as @fanquake (built on Ubuntu VM running on an M4 MacBook Pro in Qemu / UTM).