💬 fanquake commented on issue "macOS App Notarization":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1866001929)
> Not if the notarization is stapled to the binary, no. That should prevent phoning home. I have not used Wireshark to verify this, but that's what the documentation claims.
Can you link to this documentation you're talking about? Or is that the developer thread above?
Reading https://developer.apple.com/documentation/security/notarizing_macos_software_before_distribution/customizing_the_notarization_workflow#3087720, it's not clear that no phone home happens if there is a ticket present,
...
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1866001929)
> Not if the notarization is stapled to the binary, no. That should prevent phoning home. I have not used Wireshark to verify this, but that's what the documentation claims.
Can you link to this documentation you're talking about? Or is that the developer thread above?
Reading https://developer.apple.com/documentation/security/notarizing_macos_software_before_distribution/customizing_the_notarization_workflow#3087720, it's not clear that no phone home happens if there is a ticket present,
...
💬 gruve-p commented on issue "macOS App Notarization":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1866038316)
Might be relevant:
- https://blog.jacopo.io/en/post/apple-ocsp/
- https://lapcatsoftware.com/articles/ocsp.html
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1866038316)
Might be relevant:
- https://blog.jacopo.io/en/post/apple-ocsp/
- https://lapcatsoftware.com/articles/ocsp.html
✅ glozow closed a pull request: "test: fix typo"
(https://github.com/bitcoin/bitcoin/pull/29121)
(https://github.com/bitcoin/bitcoin/pull/29121)
💬 glozow commented on pull request "test: fix typo":
(https://github.com/bitcoin/bitcoin/pull/29121#issuecomment-1866076264)
Thanks for your interest in Bitcoin Core! Feel free to read CONTRIBUTING.md for advice on contributing https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy
(https://github.com/bitcoin/bitcoin/pull/29121#issuecomment-1866076264)
Thanks for your interest in Bitcoin Core! Feel free to read CONTRIBUTING.md for advice on contributing https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy
✅ glozow closed a pull request: "Fix spelling errors"
(https://github.com/bitcoin/bitcoin/pull/29107)
(https://github.com/bitcoin/bitcoin/pull/29107)
💬 glozow commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1866082831)
I'm going to close this in an effort focus review attention on more important PRs, sorry (see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy). I think it makes more sense to fix typos as we work on those areas of code.
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1866082831)
I'm going to close this in an effort focus review attention on more important PRs, sorry (see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy). I think it makes more sense to fix typos as we work on those areas of code.
💬 fanquake commented on pull request "Use hardened runtime on macOS release builds.":
(https://github.com/bitcoin/bitcoin/pull/29127#issuecomment-1866088324)
> The release maintainer, or any authorized Apple Developer, will need to run xcrun notarytool
How does someone do this on Linux, without macOS hardware/Xcode?
(https://github.com/bitcoin/bitcoin/pull/29127#issuecomment-1866088324)
> The release maintainer, or any authorized Apple Developer, will need to run xcrun notarytool
How does someone do this on Linux, without macOS hardware/Xcode?
💬 glozow commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1866099123)
Agree with the intent of avoiding legal problems, but NACK on adding this text. Unless we have some kind of legal guidance saying this text would protect us beyond what our existing license-related docs say, I don't see any reason to discourage specific tools in the contributing guidelines. I agree with the above that trying to speculate/innovate can do more harm than good.
I think we should close this for now and reconsider if/when a a lawyer advises us to do something like this.
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1866099123)
Agree with the intent of avoiding legal problems, but NACK on adding this text. Unless we have some kind of legal guidance saying this text would protect us beyond what our existing license-related docs say, I don't see any reason to discourage specific tools in the contributing guidelines. I agree with the above that trying to speculate/innovate can do more harm than good.
I think we should close this for now and reconsider if/when a a lawyer advises us to do something like this.
💬 hebasto commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1866125765)
> Since it is always part of the internal library, the `HAVE_CONSENSUS_LIB` define used by the tests is essentially
> meaningless, since the module is always available.
It seems worth noting that we do not use the `HAVE_CONSENSUS_LIB` macro in `/src/test/fuzz/script_bitcoin_consensus.cpp` in the master branch.
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1866125765)
> Since it is always part of the internal library, the `HAVE_CONSENSUS_LIB` define used by the tests is essentially
> meaningless, since the module is always available.
It seems worth noting that we do not use the `HAVE_CONSENSUS_LIB` macro in `/src/test/fuzz/script_bitcoin_consensus.cpp` in the master branch.
💬 kristapsk commented on pull request "Fix spelling errors":
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1866131660)
IMHO it would been simpler to just merge such trivial changes, but ok.
(https://github.com/bitcoin/bitcoin/pull/29107#issuecomment-1866131660)
IMHO it would been simpler to just merge such trivial changes, but ok.
💬 hebasto commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1866158665)
Concept ACK on moving the `script/bitcoinconsensus.cpp` out from the internal `libbitcoin_consensus.a` library.
@theuni
> I think another way of looking at this is that `script/bitcoinconsensus.cpp` is moving out into its own convenience lib, it just happens to be a single file. We could imagine if the api were written in multiple translation units, we'd actually want to build that convenience lib.
That is very similar to what I suggested in https://github.com/bitcoin/bitcoin/pull/2499
...
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1866158665)
Concept ACK on moving the `script/bitcoinconsensus.cpp` out from the internal `libbitcoin_consensus.a` library.
@theuni
> I think another way of looking at this is that `script/bitcoinconsensus.cpp` is moving out into its own convenience lib, it just happens to be a single file. We could imagine if the api were written in multiple translation units, we'd actually want to build that convenience lib.
That is very similar to what I suggested in https://github.com/bitcoin/bitcoin/pull/2499
...
💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1866160684)
> Yes, that's what the compiler error says, but that function doesn't seem to be in that namespace. Its other callers don't qualify it, and if I do qualify it, the build fails for me.
Seems that the function was moved to the `util` namespace on #28075.
Try cleaning up your project and rebasing it on master. (git clean -fdx && git pull --rebase origin master && make).
Will need to do some modifications here. `LockDirectory()` does not return a boolean anymore.
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1866160684)
> Yes, that's what the compiler error says, but that function doesn't seem to be in that namespace. Its other callers don't qualify it, and if I do qualify it, the build fails for me.
Seems that the function was moved to the `util` namespace on #28075.
Try cleaning up your project and rebasing it on master. (git clean -fdx && git pull --rebase origin master && make).
Will need to do some modifications here. `LockDirectory()` does not return a boolean anymore.
📝 brunoerg opened a pull request: "wallet, rpc: add BIP44 `account` in `createwallet` "
(https://github.com/bitcoin/bitcoin/pull/29129)
This PR adds an `account` parameter in `createwallet` RPC. It's the
BIP44 account that will be used in `SetupDescriptorScriptPubKeyMans`
to fetch the descriptors from the external signer.
(https://github.com/bitcoin/bitcoin/pull/29129)
This PR adds an `account` parameter in `createwallet` RPC. It's the
BIP44 account that will be used in `SetupDescriptorScriptPubKeyMans`
to fetch the descriptors from the external signer.
👋 fanquake's pull request is ready for review: "build: switch to using LLVM 17.x for macOS builds"
(https://github.com/bitcoin/bitcoin/pull/28880)
(https://github.com/bitcoin/bitcoin/pull/28880)
💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1866292269)
Guix build (x86_64 & aarch64):
```bash
65981453fcb83338ed76435dc8fed06e9903441731b461e4f2c8a04ad102043c guix-build-b335710782c2/output/aarch64-linux-gnu/SHA256SUMS.part
3127d18bc55a7a207eb7e1252feaf24f767d5983e5d064323671139659c4f3d7 guix-build-b335710782c2/output/aarch64-linux-gnu/bitcoin-b335710782c2-aarch64-linux-gnu-debug.tar.gz
0a67737c010e742249f07dd3ca3ae289df02edf2dedd8d6a5a07b291921bab47 guix-build-b335710782c2/output/aarch64-linux-gnu/bitcoin-b335710782c2-aarch64-linux-gnu.tar.g
...
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1866292269)
Guix build (x86_64 & aarch64):
```bash
65981453fcb83338ed76435dc8fed06e9903441731b461e4f2c8a04ad102043c guix-build-b335710782c2/output/aarch64-linux-gnu/SHA256SUMS.part
3127d18bc55a7a207eb7e1252feaf24f767d5983e5d064323671139659c4f3d7 guix-build-b335710782c2/output/aarch64-linux-gnu/bitcoin-b335710782c2-aarch64-linux-gnu-debug.tar.gz
0a67737c010e742249f07dd3ca3ae289df02edf2dedd8d6a5a07b291921bab47 guix-build-b335710782c2/output/aarch64-linux-gnu/bitcoin-b335710782c2-aarch64-linux-gnu.tar.g
...
💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1866295932)
This is now reproducible, and reviewable.
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1866295932)
This is now reproducible, and reviewable.
👍 BrandonOdiwuor approved a pull request: "wallettool: Always be able to dump a wallet's database"
(https://github.com/bitcoin/bitcoin/pull/29117#pullrequestreview-1793028431)
Code Review ACK d83bea42d1f0ffb0899a6de3556c489543468995
(https://github.com/bitcoin/bitcoin/pull/29117#pullrequestreview-1793028431)
Code Review ACK d83bea42d1f0ffb0899a6de3556c489543468995
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432797027)
Rule 5 is not accurately checked here.
IMO its better to remove it completely and depend on `ApplyV3Rules` for an accurate child size limit check.
The method might even be removed completely in the future cc #28345
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432797027)
Rule 5 is not accurately checked here.
IMO its better to remove it completely and depend on `ApplyV3Rules` for an accurate child size limit check.
The method might even be removed completely in the future cc #28345
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432787931)
iwyu
```suggestion
#include <set>
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432787931)
iwyu
```suggestion
#include <set>
```
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432801208)
We can just return early if all the transactions are not v3
```suggestion
const auto any_v3 = std::any_of(package.begin(), package.end(), [](const auto& tx){ return tx->nVersion == 3;});
if (!any_v3) { return v3_ancestor_sets;}
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432801208)
We can just return early if all the transactions are not v3
```suggestion
const auto any_v3 = std::any_of(package.begin(), package.end(), [](const auto& tx){ return tx->nVersion == 3;});
if (!any_v3) { return v3_ancestor_sets;}
```