💬 maaku commented on pull request "Use hardened runtime on macOS release builds.":
(https://github.com/bitcoin/bitcoin/pull/29127#issuecomment-1868124202)
It's a step that only needs to be done once, on an Apple device. I'm not aware of any open source alternatives.
(https://github.com/bitcoin/bitcoin/pull/29127#issuecomment-1868124202)
It's a step that only needs to be done once, on an Apple device. I'm not aware of any open source alternatives.
💬 maaku commented on issue "macOS App Notarization":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1868125416)
> If so, how can it distinguish notarized+not_stapled vs not_notarized applications?
You've hit the nail on the head: notarized+not_stapled !notarized apps are bit-for-bit identical. It phones home for all applications being run, except those with stapled notary (maybe, this is disputed above).
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1868125416)
> If so, how can it distinguish notarized+not_stapled vs not_notarized applications?
You've hit the nail on the head: notarized+not_stapled !notarized apps are bit-for-bit identical. It phones home for all applications being run, except those with stapled notary (maybe, this is disputed above).
💬 Farnoosh85 commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1868125550)
> https://cirrus-ci.com/task/5898005006516224?logs=ci#L2515:
>
> ```bash
>
> _20231218_134952/wallet_basic_259/node0/regtest/wallets/default_wallet/wallet.dat] SQLite Statement: ROLLBACK TRANSACTION
>
> node0 2023-12-18T13:53:29.193915Z [scheduler] [wallet/sqlite.cpp:400] [Close] SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted
>
> node0 2023-12-18T13:53:29.194038Z [scheduler] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_bas
...
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1868125550)
> https://cirrus-ci.com/task/5898005006516224?logs=ci#L2515:
>
> ```bash
>
> _20231218_134952/wallet_basic_259/node0/regtest/wallets/default_wallet/wallet.dat] SQLite Statement: ROLLBACK TRANSACTION
>
> node0 2023-12-18T13:53:29.193915Z [scheduler] [wallet/sqlite.cpp:400] [Close] SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted
>
> node0 2023-12-18T13:53:29.194038Z [scheduler] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_bas
...
📝 achow101 opened a pull request: "wallet: `addhdkey` RPC to add just keys to wallets via new `void(KEY)` descriptor"
(https://github.com/bitcoin/bitcoin/pull/29136)
It is sometimes useful for the wallet to have keys that it can sign with but are not (initially) involved in any scripts, e.g. for setting up a multisig. Ryanofsky [suggested](https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867721948) A `void(KEY)` descriptor which allows for a key to be specified, but produces no scripts. These can be imported into the wallet, and subsequently retrieved with `gethdkeys`. Additionally, `listdescriptors` will output these descriptors so that they can
...
(https://github.com/bitcoin/bitcoin/pull/29136)
It is sometimes useful for the wallet to have keys that it can sign with but are not (initially) involved in any scripts, e.g. for setting up a multisig. Ryanofsky [suggested](https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1867721948) A `void(KEY)` descriptor which allows for a key to be specified, but produces no scripts. These can be imported into the wallet, and subsequently retrieved with `gethdkeys`. Additionally, `listdescriptors` will output these descriptors so that they can
...
🤔 ryanofsky reviewed a pull request: "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1795206875)
Concept ACK 08b9b414d9f4b4b65bf5ec0ff875889997631a79. I think I'm maybe halfway through reviewing, but here are my comments so far.
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1795206875)
Concept ACK 08b9b414d9f4b4b65bf5ec0ff875889997631a79. I think I'm maybe halfway through reviewing, but here are my comments so far.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435355508)
In commit "desc spkm: Add functions to retrieve specific private keys" (484711baa994d482110edaf510377181b8b2b465)
It looks like this function would be a little more flexible and efficient if took a CKeyID parameter instead of a CPubKey
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435355508)
In commit "desc spkm: Add functions to retrieve specific private keys" (484711baa994d482110edaf510377181b8b2b465)
It looks like this function would be a little more flexible and efficient if took a CKeyID parameter instead of a CPubKey
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435366492)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
Would suggest making these name-only parameters to avoid different bool options being confused with each other, and to leave room to add new positional parameters in the future. You could do this with:
```diff
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -274,8 +274,9 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "logging", 1, "exclude" },
{ "disconnectnode", 1, "nodeid"
...
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435366492)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
Would suggest making these name-only parameters to avoid different bool options being confused with each other, and to leave room to add new positional parameters in the future. You could do this with:
```diff
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -274,8 +274,9 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "logging", 1, "exclude" },
{ "disconnectnode", 1, "nodeid"
...
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435368965)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
Note: This is better than what I suggested. I was originally thinking this default should be true, to make it more convenient to get active hd key and ignore other keys. But defaulting to false is actually better, because it's still easy to get the active hd key with an option, and it's probably more confusing to see missing keys than extra keys. Also this default makes `gethdkeys` output more consistent wit
...
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435368965)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
Note: This is better than what I suggested. I was originally thinking this default should be true, to make it more convenient to get active hd key and ignore other keys. But defaulting to false is actually better, because it's still easy to get the active hd key with an option, and it's probably more confusing to see missing keys than extra keys. Also this default makes `gethdkeys` output more consistent wit
...
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435372329)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
This is just general style feedback, but I think where possible it's better to use `auto&` or `auto*` instead of bare `auto` because with bare `auto` in c++, you don't know if a potentially expensive object copy will happen without manually checking the type. With `auto&` or `auto*` you know there won't be an big copy without having to think about it. Up to you though if you prefer less verbosity instead
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435372329)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
This is just general style feedback, but I think where possible it's better to use `auto&` or `auto*` instead of bare `auto` because with bare `auto` in c++, you don't know if a potentially expensive object copy will happen without manually checking the type. With `auto&` or `auto*` you know there won't be an big copy without having to think about it. Up to you though if you prefer less verbosity instead
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435369940)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
Could use `Assert(desc_spkm)` to avoid undefined behavior if it is null.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435369940)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
Could use `Assert(desc_spkm)` to avoid undefined behavior if it is null.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435375318)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
It seems like theoretically there could be a race condition if the wallet became locked during this loop. Also the code ignores other potential errors. Would maybe suggest:
```c++
if (priv && desc_spkm->HasPrivKey(xpub.pubkey)) {
wallet_xprvs[xpub] = CExtKey(xpub, *CHECK_NONFATAL(desc_spkm->GetKey(xpub.pubkey)));
}
```
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435375318)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
It seems like theoretically there could be a race condition if the wallet became locked during this loop. Also the code ignores other potential errors. Would maybe suggest:
```c++
if (priv && desc_spkm->HasPrivKey(xpub.pubkey)) {
wallet_xprvs[xpub] = CExtKey(xpub, *CHECK_NONFATAL(desc_spkm->GetKey(xpub.pubkey)));
}
```
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435376241)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
Not important but I think I would suggest switching to `auto&` in the loops above and using `std::move(desc)` here to avoid copying strings when not necessary.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435376241)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
Not important but I think I would suggest switching to `auto&` in the loops above and using `std::move(desc)` here to avoid copying strings when not necessary.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435381067)
In commit "wallet: Refactor function for single DescSPKM setup" (280d26b19e9222afa902ad09cd7b42c2eb3e0044)
Seems like the &batch argument option is replaced with nullptr here. This seems like a bug, but if it is intended behavior should definitely have a comment explaining the nullptr.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435381067)
In commit "wallet: Refactor function for single DescSPKM setup" (280d26b19e9222afa902ad09cd7b42c2eb3e0044)
Seems like the &batch argument option is replaced with nullptr here. This seems like a bug, but if it is intended behavior should definitely have a comment explaining the nullptr.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435377746)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
Could use std::move here and on line 906 as well to avoid copying univalue objects. I will stop commenting about copies, though. I don't think they are important, I just figure it's very easy to avoid copying and we don't need 3 copies of each descriptor string so why not avoid it.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435377746)
In commit "wallet, rpc: Add gethdkeys RPC" (5deafad2f61f626b50b6c173558ac7236cceff13)
Could use std::move here and on line 906 as well to avoid copying univalue objects. I will stop commenting about copies, though. I don't think they are important, I just figure it's very easy to avoid copying and we don't need 3 copies of each descriptor string so why not avoid it.
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435423974)
`cs_wallet` is being held earlier, and that will prevent the wallet from locked.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435423974)
`cs_wallet` is being held earlier, and that will prevent the wallet from locked.
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435436394)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435436394)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435436422)
Done as suggested
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435436422)
Done as suggested
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435436471)
Added a `CHECK_NONFATAL`.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435436471)
Added a `CHECK_NONFATAL`.
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435436629)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435436629)
Done
💬 achow101 commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435436652)
Done
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1435436652)
Done