🤔 hodlinator reviewed a pull request: "build: increase minimum supported Windows to 10.0"
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2423642732)
Reviewed 0440c406eedbc16fa1ce6c77a802b7ea60a79057

Left one concern about comment block.

Runs fine on my Windows 11.

I don't like error messages like "bitcoind.exe is not a valid Win32 application." on Windows 7. Until we introduce a real dependency on Windows 10, I would prefer a custom MessageBox saying "Requires Windows 10" during startup. But maybe we'll introduce such a dependency soon, making it not worth the effort.

Also experimented with making the NSIS installer require Windo
...

💬 hodlinator commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#discussion_r1834305231)
> however it's not possible to set these values accordingly, due to a bug in mingw-w64 when using CRT

I think blaming it on the CERT is incorrect? See [comment](https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2448647985):

> I was mistaken above, linking against UCRT doesn't make the executable build with /GS enabled.

💬 rkrux commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1834336063)
Yeah now that I think about my comment again, I feel creating a constant is not necessary.
Instead, I like your suggestion more specially with that bit hack. If other usages of is-power-of-two is anticipated in the functional tests, then probably a function can be added like so: https://stackoverflow.com/a/600306.

💬 maflcko commented on pull request "scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp}":
(https://github.com/bitcoin/bitcoin/pull/31163#issuecomment-2464666564)
Makes sense to change the remaining ones as well for consistency.

review ACK 4120c7543ee32efed7396d7153411ecbbd588ad3 🛒

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+k
...

💬 maflcko commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2464671613)
skimmed over the diff and a bit of the CI output and it appears plausible.

lgtm ACK c3354ea6dc34a3cadc60346c363478af22a5c0c2

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2464690214)
> > Though I don't completely understand why this comment "ok to not have a config file" (and subsequent flow) is allowed
>
> My guess it is applicable when the daemon is run without a conf because this portion lies in `common/`.

My interpretation is that we shouldn't trigger an error on not finding a configuration file in the specified or default location. If it's there we read it, otherwise just skip.

💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1834363955)
Hi, I'm not sure if I understood you correctly, but I guess you say it because it's possible to call the function with ``hindex = nullptr``.

To solve that I propose this changes:

```c++
double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex* pindex, const CBlockIndex* hindex) {
if (pindex == nullptr || hindex == nullptr)
return 0.0;
...
int64_t end_of_chain_timestamp;

if (hindex->nChainWork > pindex->nChainWork) {
int64_t header_age
...

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1834375774)
I'm relying on this comment from unmerged #16545:
https://github.com/bitcoin/bitcoin/blob/b5ef85497436c3e9e60c760465d8991592efef07/src/common/args.h#L112-L114

One could imagine a user having a config file containing:
```
version=1
<...many lines...>
noversion=1
```
The last occurrence of a bool option takes precedence. Agree it feels very contrived for this specific setting, but I'm trying to stick to the general principle of trying to allow negation if possible.

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1834376280)
Done!

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1834366452)
Thanks! Applied.

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834309312)
Looking at the usage of this endpoint I am wondering if it is even necessary? I.e. instead of getting it first and using it for `chainStateFlushed`, the `chainStateFlushed` implementation could just do that internally.

More generally I have been wondering if it would be a good idea to eventually strongly type these `Data` blobs (and similarly the enums)? For example here, wrapping it in a `BlockLocator` type. I don't think this is relevant as long as libmultiprocess is used to create the c++
...

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834348410)
Nit: Shouldn't this be called `txid`?

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834365825)
Why is the `role` typed as `UInt32` when the other enums are typed as `Int32`?

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834376651)
Should this include the version as well at this point?

💬 TheCharlatan commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834362068)
How does this `write` argument here map to the `action` argument in the c++ interface? They seem to have different types.

👍 BrandonOdiwuor approved a pull request: "test: Add combinerawtransaction test to rpc_createmultisig"
(https://github.com/bitcoin/bitcoin/pull/31249#pullrequestreview-2423781623)
Code Review ACK af4d23178b420f46196fbace2176ce1fe94ed9cd

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2464747471)
@hebasto could you give this another look?

💬 dergoegge commented on pull request "doc: recitfy typos":
(https://github.com/bitcoin/bitcoin/pull/31253#issuecomment-2464751924)
Same as #30259?

💬 maflcko commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1834398930)
> Hi, I'm not sure if I understood you correctly, but I guess you say it because it's possible to call the function with `hindex = nullptr`.

I am asking why it needs to be passed at all. Shouldn't it be the same for all callers?

💬 fanquake commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#discussion_r1834418655)
I've just shortened the comment here.