💬 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
...
(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?
(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
(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/
...
(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.')
```
(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",
```
(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
...
(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.
(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.
(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
...
(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
(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.
(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
...
(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.
(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!
(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.
(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++
...
(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`?
(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`?
(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?
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r1834376651)
Should this include the version as well at this point?