💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161777986)
Done as suggested

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161778269)
Done as suggested

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161778333)
Done as suggested

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161787781)
What do you think about: `Multiple messages will each be delimited by a newline (LF) character.`

(`\n` is the escape sequence (for use in a string literal) and `0A` is the OS-specific hex encoded representation)

- https://en.cppreference.com/w/cpp/language/escape
- https://en.wikipedia.org/wiki/Newline
- https://www.ibm.com/docs/en/zos/2.4.0?topic=output-escape-sequences

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161800526)
`WALLET_FLAG_AVOID_REUSE` is defined in `walletutil.h`, not in `wallet.h`, and per `git grep "#include <wallet/wallet.h>" | wc -l` this commit would avoid needlessly adding `WALLET_FLAG_CAVEATS` to nearly fifty compilation units.

I'll add `#include <wallet/walletutil.h>` to `wallet/rpc/wallet.cpp` in this change.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1161800943)
If they're no longer active, we don't know internalness.

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