💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834128269)
Wanted to make clear it wasn't something internal to `BitcoinTestFramework/TestNode` deciding to set the RPC timeout. Replaced that with instead appending ", expect one warning" to the log message.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834128269)
Wanted to make clear it wasn't something internal to `BitcoinTestFramework/TestNode` deciding to set the RPC timeout. Replaced that with instead appending ", expect one warning" to the log message.
💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834132074)
Goot point, changed to
`Suppress these in favor of a later outcome (success, FailedToStartError, or timeout).`
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834132074)
Goot point, changed to
`Suppress these in favor of a later outcome (success, FailedToStartError, or timeout).`
💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834132851)
Made comment less definitive.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834132851)
Made comment less definitive.
💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834133495)
Added:
`Suppress similarly to the above JSONRPCException errors:`
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834133495)
Added:
`Suppress similarly to the above JSONRPCException errors:`
💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834134904)
Agree, made comment less definitive, but still think it's good to include the common cause:
`Suppress if cookie file is missing and no rpcuser or rpcpassword; bitcoind may be starting`
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834134904)
Agree, made comment less definitive, but still think it's good to include the common cause:
`Suppress if cookie file is missing and no rpcuser or rpcpassword; bitcoind may be starting`
💬 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.
(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`.
(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. :)
(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
...
(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.