💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150523650)
No longer the case.
Same for the comment that is above this one.

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150567795)
this also fixes abdef47f206dec114300b2d852f5c297b03179e3 (from #26644)

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150535358)
Not really for this PR, but.. the `used` arg is never false.

`SetAddressUsed` is only called from `SetSpentKeyState` which is only called from `AddToWallet`, which provides `used=true`.

Which.. means that if the address is not reverted to a "not used" state if the tx gets abandoned/discarded.

Could also work on it on a follow-up. Probably after #26836 so changes don't end up conflicting that much.

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150572914)
Could
```c++
BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::::LOAD_OK);
```

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150579720)
Same as above, would be good to not access `m_address_book` directly from outside of the wallet class.
The map [] operator usage on a random place of the sources can easily screw things up.

Could change it to
```c++
BOOST_CHECK(!wallet->IsAddressUsed(PKHash()));
```

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150583313)
```c++
BOOST_CHECK(wallet->IsAddressUsed(PKHash()));
BOOST_CHECK(wallet->IsAddressUsed(ScriptHash()));
```

💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150586523)
```c++
BOOST_CHECK(!wallet->IsAddressUsed(PKHash()));
BOOST_CHECK(!wallet->IsAddressUsed(ScriptHash()));
```

👍 hebasto approved a pull request: "guix: use GCC tool wrappers"
(https://github.com/bitcoin/bitcoin/pull/27345)
ACK 4133c8104f522c403c55d26bd03436a8149ff106

Guix builds:
```
df90584fe5c1c68ec6a547ba04b81b66fae61035d5d4b586bc2c70f19176bc62 guix-build-4133c8104f52/output/aarch64-linux-gnu/SHA256SUMS.part
ba5553f64f73d84850ffe4e2c1712bd803c6e6f13b9f57cbc82eecab8e9e82c1 guix-build-4133c8104f52/output/aarch64-linux-gnu/bitcoin-4133c8104f52-aarch64-linux-gnu-debug.tar.gz
316e07c7baae2ff35e929c25d472ef0b66077b275c2c55f4b24acdd015947e80 guix-build-4133c8104f52/output/aarch64-linux-gnu/bitcoin-4133c8104f
...

🚀 fanquake merged a pull request: "ci: Use TSan new runtime (llvm-16, take 3)"
(https://github.com/bitcoin/bitcoin/pull/27298)

💬 ismaelsadeeq commented on pull request "test: use address_to_scriptpubkey instead of RPC call":
(https://github.com/bitcoin/bitcoin/pull/27349#issuecomment-1486878414)
> ACK [c73a065](https://github.com/bitcoin/bitcoin/commit/c73a0658c2c5c43d4ba2d8dbf2af71abe84b40e8)
>
> Nice to see a standard function for getting the `scriptPubKey` instead of a mix of RPC calls. Removing the RPC call _should_ make the tests run faster, but considering it wasn't used in many places, I doubt it makes a material difference. Left two non-blocking nits.
>
> I also noticed the CI was failing, but when I rebased on master and ran the failing functional test locally, everything
...

💬 josibake commented on pull request "wallet: add `seeds` argument to `importdescriptors`":
(https://github.com/bitcoin/bitcoin/pull/27351#issuecomment-1486913782)
Concept ACK

Regarding the approach, my first thought was why not extend `sethdseed` to support BIP93, but I'm guessing this doesn't work out of the box per your findings in https://github.com/bitcoin/bitcoin/pull/27337 ?

💬 josibake commented on pull request "test: use address_to_scriptpubkey instead of RPC call":
(https://github.com/bitcoin/bitcoin/pull/27349#issuecomment-1486923482)
ACK https://github.com/bitcoin/bitcoin/pull/27349/commits/01e0a1679fb1deeae4daf674730de2e1a270139e

thanks for taking the suggestions! CI failure seems unrelated from what I can tell :shrug:

💬 josibake commented on pull request "test: use address_to_scriptpubkey instead of RPC call":
(https://github.com/bitcoin/bitcoin/pull/27349#discussion_r1150643887)
nit: should be a space here

```suggestion
ms_unlock_details = {"scriptPubKey": address_to_scriptpubkey(self.ms_address).hex(),
```

💬 ismaelsadeeq commented on pull request "test: use address_to_scriptpubkey instead of RPC call":
(https://github.com/bitcoin/bitcoin/pull/27349#discussion_r1150647051)
Thanks

💬 MarcoFalke commented on issue "test: `wallet_importdescriptors.py --descriptors` failure":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1486934274)
Is this still an issue with a recent version of Bitcoin Core? If yes, what are the steps to reproduce?

💬 RandyMcMillan commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1486945327)
Concept ACK

💬 josibake commented on pull request "test: wallet_create_tx.py fix race":
(https://github.com/bitcoin/bitcoin/pull/27318#issuecomment-1486945822)
ACK https://github.com/bitcoin/bitcoin/pull/27318/commits/8aab5157c55249c9023ae4e9654f5d42aaa4f314

💬 apoelstra commented on pull request "wallet: add `seeds` argument to `importdescriptors`":
(https://github.com/bitcoin/bitcoin/pull/27351#issuecomment-1486947079)
@josibake Correct. Let me give a quick overview of alternate approaches, which I realize I forgot to do:

* Extend the existing `sethdseed` RPC. This RPC is legacy-only and not supported by descriptor wallets, because it depends on the legacy "bag of keys" model where you could stick keys into the wallet and it'd "just work", often in surprising ways.
* Add a new descriptor-wallet-based `importseed` RPC or something. This doesn't work for more-or-less the same reason; the descriptor wallet as
...

💬 josibake commented on issue "wallet_create_tx.py "Not solvable pre-selected input" exception":
(https://github.com/bitcoin/bitcoin/issues/27316#issuecomment-1486956388)
seen here: https://cirrus-ci.com/task/5086775773757440

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#issuecomment-1486957262)
@furszy, thanks for the thorough review. I will start implementing your suggestions, but do you think you could help me with your comments about encapsulation like https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150541159?

The comments are just a little vague from my perspective so it'd be great if you could provide specific code suggestions and I will apply or implement them. For background, I've been a little detached from the address encapsulation attempt, and personally like m_
...