IsMine isn't used outside of the wallet except for the tests. It also doesn't make sense to be outside of the wallet. This PR moves IsMine into the wallet module and for it to take a CWallet instead of CKeyStore. The test that used IsMine is also moved to the wallet tests.
This is first prerequisites for the wallet structure changes.
Concept ACK. I think you'd need to update the linter after the rename to CWallet?
Added the circular dependency to the linter. It should be removed in the future once ScriptPubKeyManager is introduced.
Approach ACK, I'm ok with adding the circular dependency temporarily to be removed in a future PR as discussed with you at coredev
Concept ACK
ACK 3d77d8f48cc494fe5f272c175663b554e8aa6e3a (checked move-only and CWallet rename)
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 3d77d8f48cc494fe5f272c175663b554e8aa6e3a (checked move-only and CWallet rename)
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUikRQwAs9jDnCyIA93DlExBgTpbLV4DtTLKwfchRLURdMwt9/RFGjJAF9SDwwj1
gQC7eSmJYIrBff4Lx6BcYu/7vbqrSsxVmmHMQZJGZ8iv2hIjUg+YBwV6xgItRLbJ
hab8JXQ6a9QREK1beMlnvuWviIFX2EwEgk/dihByTIl7BLGL40x9t0XHuDWwdYz2
4VNsAYI6CO4oo0MztvpMKWPJutNeZpHJz6dBCj+ZB+Ra46yTZP6+HBLVHesZuomE
jjBWLfIHbv0/U8AkdhJCb+Y7uAKR9NNn6j2fGhuNqno/PhrFV4/oT9wa5XU7yLOP
Ze+0CYJa/j3oemkmWrYwWQRZyUcfdgK3h0nEMhWN36TpUalcbdR8+lynmxi5p82m
4YvlhiN9B2Hby4gXnVqnuNFqpi3HUfsexM0bxEJaoqg/Vn7A8lcnaf47e8WC5RYl
k9AeC/VV8qC6g6hYO+l4nLPs+nhkJtcdYFfwX+JxLs/wn/RJMFmvEMzdj6YloKx0
BLikxgFs
=j1at
-----END PGP SIGNATURE-----
Timestamp of file with hash 8f465f280a7c3d1fd7c5da64fad29ace14155377e17cdb1af8661e6bfe1a967a -
</details>
23 | @@ -25,7 +24,10 @@ class CCoinControl; 24 | class CFeeRate; 25 | class CKey; 26 | class CWallet; 27 | +enum isminetype : unsigned int; 28 | enum class FeeReason; 29 | +typedef uint8_t isminefilter;
nit: using?
Yeah, but should change it in the original header file as well.
Concept ACK maybe keep CKeystore to avoid circular dependency?
maybe keep
CKeystoreto avoid circular dependency?
In this whole wallet restructure, I don't think there's any way around a circular dependency here temporarily. IMO it makes more sense to do the change here rather than in another PR.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
This will be merged next week, unless there are objections.
ACK 3d77d8f48cc494fe5f272c175663b554e8aa6e3a.
Concept and superficial code review ACK 3d77d8f48cc494fe5f272c175663b554e8aa6e3a. I haven't verified move-only-ness.
Circular dependencies are expected while things are being moved around.
Concept ACK
Rebased
re-ACK e61de6306f (only change is rebase with git auto-merge)
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK e61de6306f (only change is rebase with git auto-merge)
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhq9QwAycC7mPLbHS+867UvtogkH0L4oG05HtDBKb0kCQ2e/CeLV2PaJB0s1cgo
hsZKIKtGErRyY1AnFq30VdkVweUD8UkICSb9q2zqPWYOXmG87hM4kOoySXlUCp/i
VaDPpd/nG1evVqkrfe3Q1WeQzgELJaf5zNghka1rcuJH/SMGVKTLY8zR/9KvG7n2
CQxyX6QbU6bddDqO3AfnE2+hDs5OpfPI3DXouz+pd17qLzH0wpksI+6CcnzI9w2J
x8jlQwZO8HB0FV0FB2N0rS3kqgfON9At3P++sGfdxxbfjZgyfxKtjKs54RHwSix5
UQ/P4c25iRqNlG3DI7+icZ1zp/JRNlZ5v20a/pIG/r/ZD6jBKn/MvY2aPZve9X8b
zfwvYMivFY7D8bHLWZ+3AqrDwZN/jDTuRhXAuXPzPxtWD6sTimAb95Yt/jqI67Pr
9ri/xaDHugu2Ay7wJM36MqSd9kV6cppXQXaeRKbaPHtO2QAAQApo/hlx4Kr6YJ1y
c0iF595K
=Cy/u
-----END PGP SIGNATURE-----
Timestamp of file with hash 422247e430c86078dea26fdbc2e84832a7979645f33ca2b8bd2dd37469561631 -
</details>
posthumous ACK e61de6306fd89fe9aae90253062e7b1b20343f8a, agree it belongs with the wallet and that this is a better grouping than 'script', which is not a very useful axis to split (consensus/policy/wallet is better).
27 | @@ -28,8 +28,8 @@ enum isminetype 28 | /** used for bitflags of isminetype */ 29 | typedef uint8_t isminefilter; 30 | 31 | -isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey); 32 | -isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest); 33 | +isminetype IsMine(const CWallet& wallet, const CScript& scriptPubKey);
It's a shame that the declaration and definition have different argument names (wallet here in the decl and keystore in the def)
32 | + CScript scriptPubKey; 33 | + isminetype result; 34 | + 35 | + // P2PK compressed 36 | + { 37 | + CWallet keystore(chain.get(), WalletLocation(), WalletDatabase::CreateDummy());
I'd prefer all these variables are named wallet instead of keystore, since that's what they are!