💬 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).
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1867271307)
> ... explicitly list the required (or removed) caps ...
Done, `NET_RAW` is required to run `tcpdump`: https://www.tcpdump.org/manpages/pcap.3pcap.html
Plus, the ASAN job requires that `tcpdump -w` runs and creates the file, otherwise it will be red.
I think that resolves the concerns from this thread, so I am closing it. Feel free to comment / reopen if there is more to this.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1867271307)
> ... explicitly list the required (or removed) caps ...
Done, `NET_RAW` is required to run `tcpdump`: https://www.tcpdump.org/manpages/pcap.3pcap.html
Plus, the ASAN job requires that `tcpdump -w` runs and creates the file, otherwise it will be red.
I think that resolves the concerns from this thread, so I am closing it. Feel free to comment / reopen if there is more to this.
💬 ajtowns commented on pull request "p2p: Increase tx relay rate":
(https://github.com/bitcoin/bitcoin/pull/28592#issuecomment-2513884424)
Rebased for fresh CI
(https://github.com/bitcoin/bitcoin/pull/28592#issuecomment-2513884424)
Rebased for fresh CI
📝 maflcko opened a pull request: "test: Avoid logging error when logging error"
(https://github.com/bitcoin/bitcoin/pull/31408)
Currently a logging error in the form of `--- Logging error ---` happens when an error is logged in the `_on_data` helper.
Fix it by properly logging the error.
Also, treat pylint errors as errors, to avoid this problem in the future.
Can be tested by running `p2p_addrv2_relay.py` with the following example diff:
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 523e1bd068..0f1eb29d13 100755
--- a/test/functional/test_framewor
...
(https://github.com/bitcoin/bitcoin/pull/31408)
Currently a logging error in the form of `--- Logging error ---` happens when an error is logged in the `_on_data` helper.
Fix it by properly logging the error.
Also, treat pylint errors as errors, to avoid this problem in the future.
Can be tested by running `p2p_addrv2_relay.py` with the following example diff:
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 523e1bd068..0f1eb29d13 100755
--- a/test/functional/test_framewor
...
👍 i-am-yuvi approved a pull request: "test: fix `test_invalid_tx_in_compactblock` in `p2p_compactblocks`"
(https://github.com/bitcoin/bitcoin/pull/31406#pullrequestreview-2475052056)
Tested ACK 9e278f7f13395219ab056ba8c2810d3ce5c41d65
- The test works correctly
- For those who didn't understand why its better to use `!=` instead of `is not` here, since we are converting the tip hex to int and `is not` checks for object identity (if two references point to different objects in memory) which might not accurately capture intended comparison whereas `!=` checks the numerical values directly.
(https://github.com/bitcoin/bitcoin/pull/31406#pullrequestreview-2475052056)
Tested ACK 9e278f7f13395219ab056ba8c2810d3ce5c41d65
- The test works correctly
- For those who didn't understand why its better to use `!=` instead of `is not` here, since we are converting the tip hex to int and `is not` checks for object identity (if two references point to different objects in memory) which might not accurately capture intended comparison whereas `!=` checks the numerical values directly.
💬 willcl-ark commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2513978792)
Awesome!
> 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...
This is surprising, and potentially annoying. It reads to me from [the docs](https://developer.apple.com/documentation/security/resolving-common-notarization-issues#Ensure-a-valid-code-signature) that one could only notarize a codesigned binary. I hope, that the notarisation authentication must match the codesignature in some way?
...
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2513978792)
Awesome!
> 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...
This is surprising, and potentially annoying. It reads to me from [the docs](https://developer.apple.com/documentation/security/resolving-common-notarization-issues#Ensure-a-valid-code-signature) that one could only notarize a codesigned binary. I hope, that the notarisation authentication must match the codesignature in some way?
...