💬 theuni commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290493781)
Ok, I think I see. I can force a similar error on Linux by adding "-Wl,-no-undefined" to the link-line.

I'm guessing ld64 is opinionated about undefined symbols by default. And in this case we're relying on them.

Could you try messing with undefined symbol behavior and seeing what happens?


`/opt/homebrew/opt/ccache/libexec/c++ -O3 -DNDEBUG -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk -bundle -Wl,-headerpad_max
...

💬 stevenroose commented on pull request "wallet: Allow users to create a wallet that encrypts all database records":
(https://github.com/bitcoin/bitcoin/pull/28142#issuecomment-1673672150)
Concept ACK :) (Concept Request, in fact)

💬 stevenroose commented on pull request "wallet: Allow users to create a wallet that encrypts all database records":
(https://github.com/bitcoin/bitcoin/pull/28142#issuecomment-1673671724)
> Concept NACK, I think. Wallet encryption is not going to help you if someone steals your wallet, only if someone gains very temporary access to your keyboard/mouse only. I don't think this full-encryption you're proposing has a real use case.
>
> (What might make sense is to support fully encrypting backups.)

I requested this because I have a real use case.. I run a full node that is on an unencrypted external drive. Bitcoin data is public data, it shouldn't be encrypted. I want to use t
...

💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1290509079)
I don't think we need to prepend the zero here.

👍 SambhavsCreation approved a pull request: "test: verify spend from 999-of-999 taproot multisig wallet"
(https://github.com/bitcoin/bitcoin/pull/28212#pullrequestreview-1572527277)
I think it looks good. I think the concerns about testing with different keys are not of critical importance right now. Something could be done about it in the future.

💬 jonatack commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290575196)
```bash
jon|master:~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ rm -rf build

jon|master:~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ cmake -S . -B build -DLLVM_DIR=$(llvm-config --cmakedir) -DCMAKE_BUILD_TYPE=Release -Wl,-undefined,dynamic_lookup
-- The C compiler identification is AppleClang 14.0.3.14030022
-- The CXX compiler identification is AppleClang 14.0.3.14030022
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /o
...

💬 jonatack commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290588401)
(The above works with master and 2. above, but not with 1.)

💬 theuni commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290589149)
Yep, looks like that worked! And it seems to be correct too, turns out [upstream LLVM does the same thing](https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/HandleLLVMOptions.cmake#L178).

This should be the actual fix, could you please confirm that building works with it?

```diff
diff --git a/contrib/devtools/bitcoin-tidy/CMakeLists.txt b/contrib/devtools/bitcoin-tidy/CMakeLists.txt
index 9ed18696d4..24216f2fb0 100644
--- a/contrib/devtools/bitcoin-tidy/CMakeLists.txt
+
...

💬 theuni commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#issuecomment-1673822972)
Nit: stale description.

@jonatack's issue is valid but unrelated. I'll PR a fix for that separately once we have it nailed down.

ACK d82bb90a5b9dc1fd48b10514bdcd8f425aced256.

💬 jonatack commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290624677)
```bash
jon|master =:~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ rm -rf build

jon|master =:~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ cmake -S . -B build -DLLVM_DIR=$(llvm-config --cmakedir) -DCMAKE_BUILD_TYPE=Release
-- The C compiler identification is AppleClang 14.0.3.14030022
-- The CXX compiler identification is AppleClang 14.0.3.14030022
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /opt/homebrew/opt/ccache/lib
...

🤔 furszy reviewed a pull request: "wallet: Allow users to create a wallet that encrypts all database records"
(https://github.com/bitcoin/bitcoin/pull/28142#pullrequestreview-1572652006)
> I requested this because I have a real use case.. I run a full node that is on an unencrypted external drive. Bitcoin data is public data, it shouldn't be encrypted. I want to use the wallet for a watch-only wallet though. Unfortunately the wallet files live alongside the public blockchain data so I would have to store the wallet on an unencrypted drive (that I occasionally want to borrow out to friends for them to copy the blockchain in order not to have to download it). That would reveal my
...

💬 jonatack commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#issuecomment-1673842977)
ACK

The updated steps appear to be working in https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290624677 apart from the separate issue.

💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#issuecomment-1673851694)
Thanks, @brunoerg for your reviews! In the recent push, I've addressed most of your reviews.

- `clang-format` to fix the lint's issue.
- Changed `PickValueFromArray` to the `PickValue` approach.
- Fixed one grammar mistake.

Your [comment](https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1286425941) is really interesting, but I'm not sure whether the `non-determinism` in this case might pose a significant issue or not.
So, I'll wait for more people to give their thoughts on this
...

🤔 jarolrod reviewed a pull request: "ci: Run "macOS native x86_64" job on GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/28187#pullrequestreview-1572659956)
ACK 9658d0dc17c270138523c41a982425e276b24271

As long as permissions are looked over and activation of Github Workflows and Actions is safe here: https://docs.github.com/en/rest/actions/permissions?apiVersion=2022-11-28

💬 jarolrod commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1290633471)
nit: if there is any opinion on the pr/commit title not being sufficient, you could make it something like the following (and add a body):

```
ci: Run "macOS native" job on Github Actions, revert back to x86_64

Updates the macOS native ci job to run on Github Actions which will
allow us to stay on a free plan for the job. Additionally, temporarily
revert back to running on a x86_64 architecture until Github Actions
has a arm64 (Apple Silicon) runner; which is preferable to no macOS na
...

💬 theuni commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290640874)
Heh, we're getting there. Thanks for your patience!

This should get you one step further:

```patch
diff --git a/contrib/devtools/bitcoin-tidy/CMakeLists.txt b/contrib/devtools/bitcoin-tidy/CMakeLists.txt
index 9ed18696d4..ea74b98620 100644
--- a/contrib/devtools/bitcoin-tidy/CMakeLists.txt
+++ b/contrib/devtools/bitcoin-tidy/CMakeLists.txt
@@ -33,7 +33,7 @@ else()
target_compile_options(bitcoin-tidy PRIVATE -Wextra)
endif()

-set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "--loa
...

🤔 jarolrod reviewed a pull request: "doc: Use GitHub's "Alert" markdown syntax"
(https://github.com/bitcoin/bitcoin/pull/28243#pullrequestreview-1572738308)
this would mess up the rendering in every markdown renderer except on Github, example:

<img width="634" alt="Screenshot 2023-08-10 at 4 07 03 PM" src="https://github.com/bitcoin/bitcoin/assets/23396902/a5e18b4d-9cfb-4a64-b24b-b8300aec4ff0">

💬 theuni commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290693717)
Assuming this is close to working, I've got a branch ready to PR here: https://github.com/theuni/bitcoin/tree/bitcoin-tidy-macos

💬 Jones098 commented on issue "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit"":
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1673945821)
Thanks for helping

💬 jonatack commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290704000)
Happy to be learning by doing 😄

Using your branch, with the updated doc here:

```bash
jon|master =:~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ cmake -S . -B build
-DLLVM_DIR=$(llvm-config --cmakedir) -DCMAKE_BUILD_TYPE=Release
-- The C compiler identification is AppleClang 14.0.3.14030022
-- The CXX compiler identification is AppleClang 14.0.3.14030022
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /opt/homebrew/opt/c
...