👍 furszy approved a pull request: "wallet: reuse change dest when re-creating TX with avoidpartialspends"
(https://github.com/bitcoin/bitcoin/pull/27053)

💬 furszy commented on pull request "wallet: reuse change dest when re-creating TX with avoidpartialspends":
(https://github.com/bitcoin/bitcoin/pull/27053#discussion_r1103244668)
nit: this shadows the other `change_pos` variable. Can overwrite it instead of create a new one.

💬 TheCharlatan commented on pull request "build: Add CMake-based build system (1 of N)":
(https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1426369789)
ACK f9e1e7264f925690db74abd3efe7f1a3e10f2542

I was about to post what @vasild just suggested and second his approach.

👍 theStack approved a pull request: "wallet: group outputs only once, decouple it from Coin Selection"
(https://github.com/bitcoin/bitcoin/pull/25806)

📝 john-moffett opened a pull request: "Wallet: Zero out wallet master key upon locking so it doesn't persist in memory"
(https://github.com/bitcoin/bitcoin/pull/27080)
When an encrypted wallet is locked (for instance via the RPC `walletlock`), the documentation indicates that the key is removed from memory:

https://github.com/bitcoin/bitcoin/blob/b92d609fb25637ccda000e182da854d4b762eee9/src/wallet/rpc/encrypt.cpp#L157-L158

However, the vector (a `std::vector<unsigned char, secure_allocator<unsigned char>>`) is merely _cleared_. As it is a member variable, it also stays in scope as long as the wallet is loaded, preventing the secure allocator from dealloc
...

💬 rserranon commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1426394741)
> ```c++
> if (IsCliOnlyCommand(executableCommand)) { // --> here we check if string starts with "-".
> // Execute cli-only commands
> if (!ExecuteCliCommand(executableCommand, result, etc)) { // --> This function will dispatch cli-only commands ("-generate" should be placed into its own standalone function).
> // emit any error here or inside the function
> return;
> }
> } else {
> // Execute RPC commands
> if (!RPCConsole::RPCExecuteCommandLine(m_n
...

💬 furszy commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#discussion_r1103295515)
I'm not really against this line but it is a bit of a hack to trigger the column amount re-paint.

To make the intent clearer, could decouple the `dataChanged` signal into its own method, so it can be called from `updateDisplayUnit` internally and here too.

💬 Xekyo commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1103292025)
```suggestion
return max_tx_weight_exceeded ? ErrorMaxWeightExceeded() : util::Error();
```

💬 Xekyo commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1103290900)
Nit: [weight != size](https://bitcoin.stackexchange.com/a/84006/5406)

```suggestion
bool max_tx_weight_exceeded = false;
```

💬 Xekyo commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1103291613)
```suggestion
max_tx_weight_exceeded = true; // at least one selection attempt exceeded the max weight
```

💬 Xekyo commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1103268851)
Nit: whitespace

💬 Xekyo commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#discussion_r1103289270)
```suggestion
CAmount target = 25.33L * COIN;
int max_weight = 10000; // WU
const auto& res = SelectCoinsSRD(target, cs_params, m_node.chain.get(), m_args, max_weight, [&](CWallet& wallet) {
CoinsResult available_coins;
for (int j = 0; j < 60; ++j) { // 60 UTXO --> 19,8 BTC total --> 60 × 272 WU = 16320 WU
add_coin(available_coins, wallet, CAmount(0.33 * COIN), CFeeRate(0), 144, false, 0, true);
}
for
...

👍 Xekyo approved a pull request: "wallet: coin selection, don't return results that exceed the max allowed weight"
(https://github.com/bitcoin/bitcoin/pull/26720)

📝 sipa opened a pull request: "Modernize rpcauth.py"
(https://github.com/bitcoin/bitcoin/pull/27081)
Use Python3 constructions, and f-strings.

💬 mzumsande commented on pull request "test: Fix intermittent sync issue in wallet_pruning":
(https://github.com/bitcoin/bitcoin/pull/27066#issuecomment-1426452302)
https://cirrus-ci.com/task/5441089956478976 :cry:

💬 ariard commented on pull request "consensus: Remove mainnet checkpoints":
(https://github.com/bitcoin/bitcoin/pull/25725#discussion_r1103347369)
Yeah, no strong opinion here. Doesn't seem the most complex commit to reverse if needed. At the same time good to cleanup the code (or add a historical note to explain what where checkpoints and why unused code is still there i don't know).

💬 dergoegge commented on pull request "mempool: Increase mempool default size":
(https://github.com/bitcoin/bitcoin/pull/27079#issuecomment-1426479599)
Concept NACK🪄

The rational that is provided here is unconvincing.

💬 achow101 commented on pull request "wallet: reuse change dest when re-creating TX with avoidpartialspends":
(https://github.com/bitcoin/bitcoin/pull/27053#discussion_r1103462065)
I think this is actually more than a nit, it can have consequences on the change position of the change output for the aps solution.

If the user specified a change position but we didn't use it, now `change_pos` would be `-1` , so the aps run would put its change in a random position, if it needs one. This could put the change in an unexpected position. So we shouldn't overwrite nor shadow `change_pos`.

Good catch!

💬 achow101 commented on pull request "wallet: rpc to add automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/25907#issuecomment-1426565205)
Per the discussion during the IRC meeting on 2023-01-27, I've removed the ability to rotate the active hd key. `sethdseed` can be used to set one on a blank wallet, but if there is already on set, then it cannot be changed.

💬 ishaanam commented on pull request "wallet: be able to specify a wallet name and passphrase to migratewallet":
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1099038619)
In ee620e999a8ce269a90eee15b25988428e7620fd " tests: Tests for migrating wallets by name, and providing passphrase "
It would be useful to also test the behavior of the wallet encryption post-migration (eg. that `migratewallet` re-locks the wallet). A test for when the passphrase is provided but is incorrect could also be added here. A test for when the wallet name is provided but is incorrect could also be added.