💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952406392)
Maybe elaborate the message that we could be here if no tip was connected within the timeout: "No tip within timeout or shutting down".

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952403604)
Why is comment saying "Timeout: ..." when we could be here if the tip changed (no timeout)? Is the comment wrong?

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952410800)
nit: I think it is redundant to check that `maybe_tip` has value with `CHECK_NONFATAL()` just after `if (!maybe_tip)`. That's like:

```
if (A) {
throw ...
}
assert(!A);
```

`tip = maybe_tip->hash;` seems fine as well.

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952422440)
The message of the first commit has a line that is 211 chars. Would be nice to use the [50/72 rule](https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html) in commit messages.

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952424790)
In commit message of ad3af401c19b8d05ce69011a359db3090b7018e1 `rpc: handle shutdown during long poll and wait methods`:

```diff
- getblocktemplate, waitfornewblock
+ getblocktemplate, waitfornewblock
```

💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2653380149)
> Left over mentions of Wine stem from [eaafa23](https://github.com/bitcoin/bitcoin/commit/eaafa23cbd83b7bda4b28779138c62446bbdea2a) and [21704f6](https://github.com/bitcoin/bitcoin/commit/21704f6334d2a4bd140c6e3260c4bfa3f3157bad). The former seems like it could be more of a mingw issue,

eaafa23 can probably be removed, or the comment should be adjusted, because time isn't used anymore in GetTime since 0000a63689036dc4368d04c0648a55fdf507932f and instead chrono::system_clock is used.

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2653388069)
> > Left over mentions of Wine stem from [eaafa23](https://github.com/bitcoin/bitcoin/commit/eaafa23cbd83b7bda4b28779138c62446bbdea2a) and [21704f6](https://github.com/bitcoin/bitcoin/commit/21704f6334d2a4bd140c6e3260c4bfa3f3157bad). The former seems like it could be more of a mingw issue,
>
> [eaafa23](https://github.com/bitcoin/bitcoin/commit/eaafa23cbd83b7bda4b28779138c62446bbdea2a) can probably be removed, or the comment should be adjusted, because time isn't used anymore in GetTime since
...

📝 maflcko opened a pull request: "test: Remove stale gettime test"
(https://github.com/bitcoin/bitcoin/pull/31846)
(currently typing)

💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2653417798)
Removed the stale test in https://github.com/bitcoin/bitcoin/pull/31846

💬 pinheadmz commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2653432795)
@Sjors does this help? https://github.com/pinheadmz/bitcoin-detached-sigs/tree/achow101-macos-notarization-096525e92cc2

💬 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};
```