💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1626758407)
I've made `m_batch` public so that we can just pass the `WalletBatch` but still use the underlying batch object. However I'm not sure if that's something we want to do in the long term.
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1626758407)
I've made `m_batch` public so that we can just pass the `WalletBatch` but still use the underlying batch object. However I'm not sure if that's something we want to do in the long term.
💬 achow101 commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1626758476)
Done
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r1626758476)
Done
💬 achow101 commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2148598856)
CI failure suggests there's a silent merge conflict.
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2148598856)
CI failure suggests there's a silent merge conflict.
💬 achow101 commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#issuecomment-2148603063)
ACK fa3169b0732d7eb4b9166e7ecc6b7cfb669a9b54
(https://github.com/bitcoin/bitcoin/pull/29997#issuecomment-2148603063)
ACK fa3169b0732d7eb4b9166e7ecc6b7cfb669a9b54
🚀 achow101 merged a pull request: "rpc: Remove index-based Arg accessor"
(https://github.com/bitcoin/bitcoin/pull/29997)
(https://github.com/bitcoin/bitcoin/pull/29997)
💬 achow101 commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2148617880)
ACK 7f3f6c6dc80247e6dfb0d406dc53bc8198f029fd
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2148617880)
ACK 7f3f6c6dc80247e6dfb0d406dc53bc8198f029fd
💬 achow101 commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2148626504)
ACK d7290d662f494503f28e087dd728b492c0bb2c5f
I get that any coverage of these functions is better than no coverage, but it seems less useful to me when we aren't checking the output. There may not be a crash, but each function could be doing something unexpected, and we should be checking to make sure that they aren't. I'm going to merge this for now though just so there is coverage, but a followup which verifies the behavior would be welcomed.
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2148626504)
ACK d7290d662f494503f28e087dd728b492c0bb2c5f
I get that any coverage of these functions is better than no coverage, but it seems less useful to me when we aren't checking the output. There may not be a crash, but each function could be doing something unexpected, and we should be checking to make sure that they aren't. I'm going to merge this for now though just so there is coverage, but a followup which verifies the behavior would be welcomed.
🚀 achow101 merged a pull request: "refactor: Model the bech32 charlimit as an Enum"
(https://github.com/bitcoin/bitcoin/pull/30047)
(https://github.com/bitcoin/bitcoin/pull/30047)
🤔 achow101 reviewed a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2097634633)
ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2097634633)
ACK 2451a217dd2c21b6d2f2b2699ceddd0bf9073019
💬 achow101 commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1626782997)
In b5a328943362cfac6e90fd4e1b167c357d53b7d4 "test: refactor, multiple cleanups in rpc_createmultisig.py"
Strictly speaking, this should also check for whether we have legacy wallets since the test is specifically for whether `addmultisigaddress` (a legacy wallet only rpc) matches `createmultisig`.
```suggestion
if wallet_multi is not None and not self.options.descriptors:
```
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1626782997)
In b5a328943362cfac6e90fd4e1b167c357d53b7d4 "test: refactor, multiple cleanups in rpc_createmultisig.py"
Strictly speaking, this should also check for whether we have legacy wallets since the test is specifically for whether `addmultisigaddress` (a legacy wallet only rpc) matches `createmultisig`.
```suggestion
if wallet_multi is not None and not self.options.descriptors:
```
💬 achow101 commented on pull request "rpc, wallet: fix incorrect segwit redeem script size limit":
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1626792838)
In 9be6065cc03f2408f290a332b203eef9c9cebf24 "test: coverage for 16-20 segwit multisig scripts"
nit: s/deduce/deduct
(https://github.com/bitcoin/bitcoin/pull/28307#discussion_r1626792838)
In 9be6065cc03f2408f290a332b203eef9c9cebf24 "test: coverage for 16-20 segwit multisig scripts"
nit: s/deduce/deduct
💬 achow101 commented on issue "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets":
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2148691438)
`addmultisigaddress` might be a little bit more complicated since users would expect it to behave the same way and be able to have the specific private keys from addresses. Given that we do not want to allow exposing child private keys, retaining that behavior would allow extracting specific child private keys in a rather roundabout way (`addmultisigaddress` which puts creates a descriptor with a child privkey, then `listdescriptors true` which outputs the descriptor with that privkey).
Also
...
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2148691438)
`addmultisigaddress` might be a little bit more complicated since users would expect it to behave the same way and be able to have the specific private keys from addresses. Given that we do not want to allow exposing child private keys, retaining that behavior would allow extracting specific child private keys in a rather roundabout way (`addmultisigaddress` which puts creates a descriptor with a child privkey, then `listdescriptors true` which outputs the descriptor with that privkey).
Also
...
🚀 achow101 merged a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074)
(https://github.com/bitcoin/bitcoin/pull/28074)
💬 achow101 commented on pull request "functional test: ensure confirmed utxo being sourced for 2nd chain":
(https://github.com/bitcoin/bitcoin/pull/29998#issuecomment-2148693400)
ACK 07aba8dd215b23b06853b1a9fe04ac8b08f62932
(https://github.com/bitcoin/bitcoin/pull/29998#issuecomment-2148693400)
ACK 07aba8dd215b23b06853b1a9fe04ac8b08f62932
💬 achow101 commented on pull request "refactor: remove unused `CKey::Negate` method":
(https://github.com/bitcoin/bitcoin/pull/30218#issuecomment-2148694127)
ACK 8801e319d51209fe3a3b06e2aab5f96ceead290d
(https://github.com/bitcoin/bitcoin/pull/30218#issuecomment-2148694127)
ACK 8801e319d51209fe3a3b06e2aab5f96ceead290d
🚀 achow101 merged a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307)
(https://github.com/bitcoin/bitcoin/pull/28307)
🚀 achow101 merged a pull request: "functional test: ensure confirmed utxo being sourced for 2nd chain"
(https://github.com/bitcoin/bitcoin/pull/29998)
(https://github.com/bitcoin/bitcoin/pull/29998)
🚀 achow101 merged a pull request: "refactor: remove unused `CKey::Negate` method"
(https://github.com/bitcoin/bitcoin/pull/30218)
(https://github.com/bitcoin/bitcoin/pull/30218)
💬 Kordestan1993 commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2148721299)
address wallet BTC
1ATUEQQ1PTJQgqiWg2MpRWJHAipnpkQr6U
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2148721299)
address wallet BTC
1ATUEQQ1PTJQgqiWg2MpRWJHAipnpkQr6U
🤔 achow101 reviewed a pull request: "Several randomness improvements"
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2097721638)
Reviewed up to 64bcc828cde51a5da551b49a31b9fe44e0a255c3
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2097721638)
Reviewed up to 64bcc828cde51a5da551b49a31b9fe44e0a255c3