💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2464405109)
Thanks for the reviews! Was trying to keep comments largely intact, but agree with @maflcko that they were somewhat incorrect, so altered them. Also renamed `ignored_errors` (introduced by this PR) to `suppressed_errors` since it felt more fitting and also matches the updated comments.

💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1834151211)
I don't think this is correct. The size is identical for both, so how can it be invalid for one, but not the other? They are invalid due to the embedded `=` and `\0`.

💬 rkrux commented on pull request "doc: recitfy typos":
(https://github.com/bitcoin/bitcoin/pull/31253#issuecomment-2464434603)
> If your change contains a merge commit, the above workflow may not work and you will need to remove the merge commit first. See the next section for details on how to rebase.

The merge commit would need to be removed: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes

> doc: recitfy typos

Also, there's a typo in the PR title. :)

💬 maflcko commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1834183665)
f04529fc40f2f7c8e4e63d892cdcbe6c91dfbf96: Any reason to make this a member method, when it doesn't use any member fields? I'd say it would be fine to either make this a global, standalone, util function. Otherwise, if you want to make it a member, it would be good to also add it to the other classes, like test_framework. Then, always use it from the outer-most scope.

For example, it is a bit confusing to call `self.nodes[0].p2ps[0].ensure_for(...)`, when `self.ensure_for(...)` is identical, o
...

💬 maflcko commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1834202940)
Is it really useful to map one exception type to another for a test-only dev-only debug-only feature? I'd say it seems fine to just use the python built-in exceptions, which should be self-explanatory. I understand that they won't print the method name, but it could make sense to log that either way as the first thing in the for loop?

💬 maflcko commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2464504665)
I think fixing stability would be nice, but not a blocker

👍 rkrux approved a pull request: "contrib: skip missing binaries in gen-manpages"
(https://github.com/bitcoin/bitcoin/pull/30986#pullrequestreview-2423578275)
Coming here from https://github.com/bitcoin/bitcoin/pull/31255, putting my two cents in.

I had to generate man pages while reviewing a PR and found that the script threw error even in the default case due to the new build process that moved the binaries in `/build`, so a strong ACK for 0194a638d31fef7e8193c50843c11d2c2ee212a3 because IMO the script should work in the default case w/o needing to set the `BUILDDDIR`.

As I read from @laanwj's [comment](https://github.com/bitcoin/bitcoin/pull/
...

💬 rkrux commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1834300124)
```
Set BUILDDIR to specify the correct build path.
```

Is the intent to make the user explicitly set BUILDDIR? Not aware of the usual use cases but as a user, my preference is to just run the script to generate the man pages w/o needing to set an env var. I'd be ok with this error as well.

```
print(f'No binaries found in {builddir}. Please specify the correct build path.')
```

💬 rkrux commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1834261233)
nit: for verbosity
```
help="skip generation for binaries that are not found in the build path",
```

🤔 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++
...