📝 brunoerg opened a pull request: "test: fix `test_invalid_tx_in_compactblock` in `p2p_compactblocks`"
(https://github.com/bitcoin/bitcoin/pull/31406)
The `test_invalid_tx_in_compactblock` function tests that we don't get disconnected if we relay a compact block with valid header, but invalid transactions. After sending the block with invalid transactions, this test checks 2 things: the tip in the receiver node did not advance and the sender does not get disconnected. However, even if the block contains only valid transactions, the tip would not advance because the receiver does not have the necessary transactions to reconstruct the valid and
...
(https://github.com/bitcoin/bitcoin/pull/31406)
The `test_invalid_tx_in_compactblock` function tests that we don't get disconnected if we relay a compact block with valid header, but invalid transactions. After sending the block with invalid transactions, this test checks 2 things: the tip in the receiver node did not advance and the sender does not get disconnected. However, even if the block contains only valid transactions, the tip would not advance because the receiver does not have the necessary transactions to reconstruct the valid and
...
🤔 furszy reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2474045739)
Code review ACK 7b267c034fdc2
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2474045739)
Code review ACK 7b267c034fdc2
👍 ryanofsky approved a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2473436614)
Code review ACK c972e691d937f49d7d3f44d2b19b4035c6e67aaf. Thanks for breaking the commits up make making things easy to understand.
Changes since last review were simplifying the -norpccookiefile and -noconf fixes by breaking them up and changing some implementation details. There are also new combine_logs and feature_config args test improvements.
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2473436614)
Code review ACK c972e691d937f49d7d3f44d2b19b4035c6e67aaf. Thanks for breaking the commits up make making things easy to understand.
Changes since last review were simplifying the -norpccookiefile and -noconf fixes by breaking them up and changing some implementation details. There are also new combine_logs and feature_config args test improvements.
💬 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
...