💬 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

💬 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.

💬 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)));
}
```

💬 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.

💬 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.

💬 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.

💬 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.

💬 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

💬 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

💬 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`.

💬 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

💬 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

💬 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_r1435436670)
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_r1435436690)
Indeed, fixed.

💬 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_r1435436779)
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_r1435436813)
Moved to the right commit.

💬 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_r1435437218)
This function was intended to only return a `CExtPubKey` if all active descriptors shared the same xpub, so if they differed, it would return `std::nullopt`. The comment was stating what it was attempting to do, not what it was expecting.

In any case, having the caller determine what to do if there is more than one xpub in the active descriptors is probably better, so I've changed it to do that and this comment is no longer relevant.

💬 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_r1435437249)
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_r1435439477)
Done

⚠️ derekm opened an issue: "Spam Filter Code in Tx Relay & Mempool Acceptance Ignores OFAC SDN Sanctions"
(https://github.com/bitcoin/bitcoin/issues/29137)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

As an American Bitcoin miner,
New transactions' inputs and outputs are not checked for OFAC SDN Sanctions against XBT,
Therefore I am exposed to the risk of imprisonment should I mine a block containing txns for specially designated nationals and blocked persons.

### Expected behaviour

As an American Bitcoin miner,
I should be able to configure my node to check incoming transactions f
...

💬 derekm commented on issue "Spam Filter Code in Tx Relay & Mempool Acceptance Ignores OFAC SDN Sanctions":
(https://github.com/bitcoin/bitcoin/issues/29137#issuecomment-1868196209)
What XBT sanctions all American citizens must implement can be found here: https://www.treasury.gov/ofac/downloads/sanctions/1.0/sdn_advanced.xml