💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866689589)
In commit "args: Catch directories in place of config files" (01b174493ec210c1c627083dfa9e9b86035c7eef)
If macos is setting the eof bit when using ifstream to read from a directory paths as described https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2506175947, treating directories like empty files, maybe we really do need these is_directory calls, since there might be no other way on macos to distinguish directories from empty files, though I suspect it probably is indicating some e
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866689589)
In commit "args: Catch directories in place of config files" (01b174493ec210c1c627083dfa9e9b86035c7eef)
If macos is setting the eof bit when using ifstream to read from a directory paths as described https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2506175947, treating directories like empty files, maybe we really do need these is_directory calls, since there might be no other way on macos to distinguish directories from empty files, though I suspect it probably is indicating some e
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866653856)
In commit "args: Properly support -noconf" (2ac863bdf974d7b4b4e8314876d02270400b6240)
I'm still not sure why this is a useful warning. There would seem to be little reason to specify `-noconf` unless you are trying to ignore a config file, so why should you be warned if a file you were trying to ignore exists? If anything, you might expect a warning in the opposite case, when a file you specified an option to ignore does *not* exist. Both warnings seem excessive to me and and I think the app
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866653856)
In commit "args: Properly support -noconf" (2ac863bdf974d7b4b4e8314876d02270400b6240)
I'm still not sure why this is a useful warning. There would seem to be little reason to specify `-noconf` unless you are trying to ignore a config file, so why should you be warned if a file you were trying to ignore exists? If anything, you might expect a warning in the opposite case, when a file you specified an option to ignore does *not* exist. Both warnings seem excessive to me and and I think the app
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866406451)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866191667
> optional of optional sounds like a `std::variant` - but I'm fine with doing these in a separate PR, of course
Yes could use something like `std::variant<Disabled, not_empty<fs::path>>` if we want a separate type to represent the separate condition. ([`Disabled`](https://github.com/ryanofsky/bitcoin/blob/4e33ed4eb054e230436b68c681d978d0e7bea0a1/src/common/setting.h#L17-L18) could be the type from #31260)). `not_empty
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866406451)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866191667
> optional of optional sounds like a `std::variant` - but I'm fine with doing these in a separate PR, of course
Yes could use something like `std::variant<Disabled, not_empty<fs::path>>` if we want a separate type to represent the separate condition. ([`Disabled`](https://github.com/ryanofsky/bitcoin/blob/4e33ed4eb054e230436b68c681d978d0e7bea0a1/src/common/setting.h#L17-L18) could be the type from #31260)). `not_empty
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866384411)
In commit "test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning" (eb2d96bb66f109e610ee44cf3fed5c6701e3b8e3)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863337310
> My untrained eyes would prefer seeing this in a different PR, I can't meaningfully review these yet :/
Don't disagree this commit could be a separate PR. A separate PR might attract feedback from reviewers more familiar with how the test framework is designed to be us
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866384411)
In commit "test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning" (eb2d96bb66f109e610ee44cf3fed5c6701e3b8e3)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863337310
> My untrained eyes would prefer seeing this in a different PR, I can't meaningfully review these yet :/
Don't disagree this commit could be a separate PR. A separate PR might attract feedback from reviewers more familiar with how the test framework is designed to be us
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866307162)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863342759
> nit Q: I'm not sure I understand the logging quoting standards here, when do we use `\"` and when `'` and when just `=%s`?
I think these are just quoted to convey that 'owner' 'group' and 'all' are literal strings, not placeholders for permission values.
It would be nice if there was a standard way paths and arguments were quoted in log messages, but I don't think there is a standard right now.
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866307162)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863342759
> nit Q: I'm not sure I understand the logging quoting standards here, when do we use `\"` and when `'` and when just `=%s`?
I think these are just quoted to convey that 'owner' 'group' and 'all' are literal strings, not placeholders for permission values.
It would be nice if there was a standard way paths and arguments were quoted in log messages, but I don't think there is a standard right now.
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866369774)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863359735
> Do I understand correctly that we're basically treating an empty path as an `std::nullopt` here (and commenting it everywhere since we admit it's hard to understand)? I understand that we can't fix everything in the same PR, but would like us to consider if it still makes sense to build on previous hacks.
Just using std::optional here would not be an improvement IMO. An `nullopt` setting is conventionally an unspeci
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866369774)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863359735
> Do I understand correctly that we're basically treating an empty path as an `std::nullopt` here (and commenting it everywhere since we admit it's hard to understand)? I understand that we can't fix everything in the same PR, but would like us to consider if it still makes sense to build on previous hacks.
Just using std::optional here would not be an improvement IMO. An `nullopt` setting is conventionally an unspeci
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866325181)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863372988
> what if it's empty?
If it's empty it means the user specified `-norpccookiefile`, and some other authentication mechanism must be used. There's another log statement `LogInfo("RPC authentication cookie file generation is disabled.");` in `GenerateAuthCookie` that could probably be moved here to make this clearer.
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866325181)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863372988
> what if it's empty?
If it's empty it means the user specified `-norpccookiefile`, and some other authentication mechanism must be used. There's another log statement `LogInfo("RPC authentication cookie file generation is disabled.");` in `GenerateAuthCookie` that could probably be moved here to make this clearer.
📝 achow101 opened a pull request: "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries"
(https://github.com/bitcoin/bitcoin/pull/31407)
I have updated signapple to notarize MacOS app bundles without adding any additional dependencies. Further, it can also sign and apply detached signatures to standalone binaries.
As such, we can use signapple to perform the notarization and stapling steps so that MacOS will run the app bundle after it is installed. `detached-sig-create.sh` is updated to have a notarization step and to download the ticket which will be included in the detached signatures. The workflow is largely unchanged for
...
(https://github.com/bitcoin/bitcoin/pull/31407)
I have updated signapple to notarize MacOS app bundles without adding any additional dependencies. Further, it can also sign and apply detached signatures to standalone binaries.
As such, we can use signapple to perform the notarization and stapling steps so that MacOS will run the app bundle after it is installed. `detached-sig-create.sh` is updated to have a notarization step and to download the ticket which will be included in the detached signatures. The workflow is largely unchanged for
...
💬 Julian128 commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1866731481)
this is definitely the wrong file, I couldn't even find this change
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1866731481)
this is definitely the wrong file, I couldn't even find this change
💬 achow101 commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2513143909)
signapple has been updated to do notarization and stapling, and I've opened #31407 to add that to the guix build process.
Also, it turns out anyone enrolled in the Apple Developer Program can submit any app to Apple for notarization even if they weren't the code signer...
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2513143909)
signapple has been updated to do notarization and stapling, and I've opened #31407 to add that to the guix build process.
Also, it turns out anyone enrolled in the Apple Developer Program can submit any app to Apple for notarization even if they weren't the code signer...
🤔 mzumsande reviewed a pull request: "refactor: Move GuessVerificationProgress into ChainstateManager"
(https://github.com/bitcoin/bitcoin/pull/31393#pullrequestreview-2473893875)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31393#pullrequestreview-2473893875)
Concept ACK
💬 mzumsande commented on pull request "refactor: Move GuessVerificationProgress into ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/31393#discussion_r1866597615)
can remove `params`, it's no longer used
(https://github.com/bitcoin/bitcoin/pull/31393#discussion_r1866597615)
can remove `params`, it's no longer used
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2513198369)
It's unclear to me whether the standalone binaries need to be notarized too. This is currently not implemented, but should not be that much more complicated to do.
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2513198369)
It's unclear to me whether the standalone binaries need to be notarized too. This is currently not implemented, but should not be that much more complicated to do.
👋 mzumsande's pull request is ready for review: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405)
(https://github.com/bitcoin/bitcoin/pull/31405)
💬 edilmedeiros commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2513389140)
> It's unclear to me whether the standalone binaries need to be notarized too. This is currently not implemented, but should not be that much more complicated to do.
Are they being codesigned already?
I was getting the v28 binaries from bitcoincore.org instantly killed in Sonoma 14.6.1 when trying to run them in the terminal today. Took me a while to understand what was happening because the processes are killed without a security message or anything like the "nice" gatekeeper popup. Codes
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2513389140)
> It's unclear to me whether the standalone binaries need to be notarized too. This is currently not implemented, but should not be that much more complicated to do.
Are they being codesigned already?
I was getting the v28 binaries from bitcoincore.org instantly killed in Sonoma 14.6.1 when trying to run them in the terminal today. Took me a while to understand what was happening because the processes are killed without a security message or anything like the "nice" gatekeeper popup. Codes
...
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2513391341)
> Are they being codesigned already?
This PR codesigns them.
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2513391341)
> Are they being codesigned already?
This PR codesigns them.
👍 maflcko approved a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2474834990)
Changes since my previous review:
* Doc/test fixups
* debug message fixup
* rebase
* Add back missing test coverage for par=1
re-ACK 492e1f09943fcb6145c21d470299305a19e17d8b 🍈
<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}"
RUTRmVTMe
...
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2474834990)
Changes since my previous review:
* Doc/test fixups
* debug message fixup
* rebase
* Add back missing test coverage for par=1
re-ACK 492e1f09943fcb6145c21d470299305a19e17d8b 🍈
<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}"
RUTRmVTMe
...
💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1867208630)
nano nit in 492e1f0994 (only if you retouch): Could remove the unused `f`?
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1867208630)
nano nit in 492e1f0994 (only if you retouch): Could remove the unused `f`?
💬 Sjors commented on pull request "Miner: never create a template which exploits the timewarp bug":
(https://github.com/bitcoin/bitcoin/pull/31376#discussion_r1867245362)
So why not drop `- MAX_TIMEWARP` and just make sure that we never propose a block with an earlier timestamp?
Or go beyond that and never allow time to go backwards on _any_ block?
(https://github.com/bitcoin/bitcoin/pull/31376#discussion_r1867245362)
So why not drop `- MAX_TIMEWARP` and just make sure that we never propose a block with an earlier timestamp?
Or go beyond that and never allow time to go backwards on _any_ block?
💬 Sjors commented on pull request "Miner: never create a template which exploits the timewarp bug":
(https://github.com/bitcoin/bitcoin/pull/31376#issuecomment-2513848116)
> But this PR does not make any block invalid. This is therefore not a concern with this PR
No, but it's adding complexity that might not match the actual soft fork. See my inline comment for a more generic approach (but I'm not sure if it's safe).
(https://github.com/bitcoin/bitcoin/pull/31376#issuecomment-2513848116)
> But this PR does not make any block invalid. This is therefore not a concern with this PR
No, but it's adding complexity that might not match the actual soft fork. See my inline comment for a more generic approach (but I'm not sure if it's safe).