💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161805097)
So they aren't repeatedly written when the same key shows up in multiple descriptors.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161806010)
The encrypted secrets are all in the same format for both legacy wallet keys, descriptor keys, and now hd ckeys. I suppose this could be documented somewhere, although it's not immediately obvious where.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161810575)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161810707)
Changed

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161810822)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161810914)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161811025)
Done

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161811758)
Either way someone will ask why it isn't done the other way, i.e. https://github.com/bitcoin/bitcoin/pull/17578#pullrequestreview-338232718, so I went with the smallest diff.

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1161841784)
Done as suggested

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1161841837)
Done as suggested

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1161841922)
Removed

💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1161841985)
Done as suggested

💬 theuni commented on pull request "contrib: followups to #27358 (verify-binaries)":
(https://github.com/bitcoin/bitcoin/pull/27440#discussion_r1161851409)
Yeah, I think this is the crux of the thing. Agree that we need a domain argument for this script to be generic enough to be useful. So I propose that we (as yet another follow-up):
- Document the current assumed remote dir structure and call it the standard*.
- Hook up a domain option that's intended to work with above well-documented standard.

This would allow for a crude federated model of pulling that suits the multisig model well, so I think it's a good way forward for us.

* Obvious
...

👍 ryanofsky approved a pull request: "wallet: Replace use of purpose strings with an enum"
(https://github.com/bitcoin/bitcoin/pull/27217#pullrequestreview-1377763599)
Code review ACK 98821a7aca88ac9401786e848899866b71446460. Just suggested changes since last review, the main one being changing `translateTransactionType` to use a switch statement.

This had 3 reviews before, so it would be nice to have reacks and hopefully merge this soon.

👍 theuni approved a pull request: "contrib: followups to #27358 (verify-binaries)"
(https://github.com/bitcoin/bitcoin/pull/27440#pullrequestreview-1377799833)
ACK ad841608d4edf6151b60e483793f60ba9f03cdbf. Thanks for doing these.

Anybody interested in taking https://github.com/bitcoin/bitcoin/pull/27440#discussion_r1161268754 ? :)

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161892675)
Done.

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161898967)
It would only be broken if the word "warnings" is backported into the loadwallet help in a previous release, but to address your request replaced this line with `if int(node_master.getnetworkinfo()["version"]) >= 249900:`

🤔 Xekyo reviewed a pull request: "wallet: group outputs only once, decouple it from Coin Selection"
(https://github.com/bitcoin/bitcoin/pull/25806#pullrequestreview-1377824610)
Post merge ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44

💬 Xekyo commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1161893162)
```suggestion
if (!positive_only || (coin.GetEffectiveValue() > 0)) {
```

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161912974)
Done.