doc: Add developer documentation to isminetype #17350

pull HAOYUatHZ wants to merge 1 commits into bitcoin:master from HAOYUatHZ:doc_isminetype changing 1 files +21 −1
  1. HAOYUatHZ commented at 2:57 AM on November 2, 2019: contributor

    Closes: https://github.com/bitcoin/bitcoin/issues/17217

  2. DrahtBot added the label Docs on Nov 2, 2019
  3. DrahtBot added the label Wallet on Nov 2, 2019
  4. in src/wallet/ismine.h:20 in d4fae034dd outdated 
      16 | @@ -17,13 +17,13 @@ class CScript;
  17 |  /** IsMine() return codes */
  18 |  enum isminetype : unsigned int
  19 |  {
  20 | -    ISMINE_NO         = 0,
  21 | -    ISMINE_WATCH_ONLY = 1 << 0,
  22 | -    ISMINE_SPENDABLE  = 1 << 1,
  23 | -    ISMINE_USED       = 1 << 2,
  24 | +    ISMINE_NO         = 0, // the scriptPubKey is not contained in the wallet
    laanwj commented at 8:53 AM on November 2, 2019:

    Please use //<! here so that doxygen picks up the descriptions

    HAOYUatHZ commented at 9:15 AM on November 3, 2019:

    @laanwj @fanquake fixed, can u take a look again? thx~

  5. fanquake added the label Waiting for author on Nov 2, 2019
  6. HAOYUatHZ requested review from laanwj on Nov 3, 2019
  7. in src/wallet/ismine.h:20 in 08b8e81e9d outdated 
      22 | -    ISMINE_SPENDABLE  = 1 << 1,
  23 | -    ISMINE_USED       = 1 << 2,
  24 | -    ISMINE_ALL        = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
  25 | -    ISMINE_ALL_USED   = ISMINE_ALL | ISMINE_USED,
  26 | -    ISMINE_ENUM_ELEMENTS,
  27 | +    ISMINE_NO         = 0, //!< the scriptPubKey is not contained in the wallet
    promag commented at 9:23 PM on November 3, 2019:

    nit, could keep comments aligned, like https://github.com/bitcoin/bitcoin/blob/463eab5e1418a592036e7bf9bf46f66fe6462435/src/chain.h#L40-L51

    HAOYUatHZ commented at 10:59 PM on November 3, 2019:

    nit, could keep comments aligned, like

    https://github.com/bitcoin/bitcoin/blob/463eab5e1418a592036e7bf9bf46f66fe6462435/src/chain.h#L40-L51

    done. thx for pointing it out

  8. promag commented at 9:25 PM on November 3, 2019: member

    Concept ACK.

  9. HAOYUatHZ requested review from promag on Nov 3, 2019
  10. in src/wallet/ismine.h:24 in cff4e50ab6 outdated 
      26 | -    ISMINE_ENUM_ELEMENTS,
  27 | +    ISMINE_NO         = 0,                                    //!< the scriptPubKey is not contained in the wallet
  28 | +    ISMINE_WATCH_ONLY = 1 << 0,                               //!< the scriptPubKey has been imported into the wallet
  29 | +    ISMINE_SPENDABLE  = 1 << 1,                               //!< the scriptPubKey is corresponding to an address owned by the wallet user (can spend with the private key)
  30 | +    ISMINE_USED       = 1 << 2,                               //!< the scriptPubKey is corresponding to an reused address owned by the wallet user (can spend with the private key)
  31 | +    ISMINE_ALL        = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE, //!< ISMINE_WATCH_ONLY or ISMINE_SPENDABLE
    laanwj commented at 12:40 PM on November 4, 2019:

    I don't think these two particular comments add anything to what is in the code. What about: "All ISMINE flags except for USED" "All ISMINE flags including USED"

  11. in src/wallet/ismine.h:23 in cff4e50ab6 outdated 
      25 | -    ISMINE_ALL_USED   = ISMINE_ALL | ISMINE_USED,
  26 | -    ISMINE_ENUM_ELEMENTS,
  27 | +    ISMINE_NO         = 0,                                    //!< the scriptPubKey is not contained in the wallet
  28 | +    ISMINE_WATCH_ONLY = 1 << 0,                               //!< the scriptPubKey has been imported into the wallet
  29 | +    ISMINE_SPENDABLE  = 1 << 1,                               //!< the scriptPubKey is corresponding to an address owned by the wallet user (can spend with the private key)
  30 | +    ISMINE_USED       = 1 << 2,                               //!< the scriptPubKey is corresponding to an reused address owned by the wallet user (can spend with the private key)
    laanwj commented at 12:41 PM on November 4, 2019:

    used, not re-used, after all this is part of the avoid-reuse logic :smile:

    laanwj commented at 12:42 PM on November 4, 2019:

    also "(can spend with the private key)" doesn't belong here, USED doesn't imply anything about spendability in the future

  12. HAOYUatHZ referenced this in commit efbda5f9b4 on Nov 4, 2019
  13. HAOYUatHZ requested review from laanwj on Nov 4, 2019
  14. HAOYUatHZ commented at 12:11 AM on November 7, 2019: contributor

    hi @laanwj and @promag

    can u take a look again? let me know if there's any mistake, thx

  15. in src/wallet/ismine.h:23 in efbda5f9b4 outdated 
      25 | -    ISMINE_ALL_USED   = ISMINE_ALL | ISMINE_USED,
  26 | -    ISMINE_ENUM_ELEMENTS,
  27 | +    ISMINE_NO         = 0,                                    //!< the scriptPubKey is not contained in the wallet
  28 | +    ISMINE_WATCH_ONLY = 1 << 0,                               //!< the scriptPubKey has been imported into the wallet
  29 | +    ISMINE_SPENDABLE  = 1 << 1,                               //!< the scriptPubKey is corresponding to an address owned by the wallet user (can spend with the private key)
  30 | +    ISMINE_USED       = 1 << 2,                               //!< the scriptPubKey is corresponding to an used address owned by the wallet user
    jonatack commented at 10:00 AM on November 7, 2019:

    Suggestion for lines 22 and 23: s/is corresponding/corresponds/

    jonatack commented at 10:11 AM on November 7, 2019:

    s/an used/a used/

    Reason: "used" is pronounced as if it began with the consonant "y"

    jonatack commented at 10:59 AM on November 7, 2019:

    (See discussion here #16047 (review))

    HAOYUatHZ commented at 7:50 AM on November 13, 2019:

    Suggestion for lines 22 and 23: s/is corresponding/corresponds/

    fixed

    HAOYUatHZ commented at 7:50 AM on November 13, 2019:

    s/an used/a used/

    Reason: "used" is pronounced as if it began with the consonant "y"

    fixed

  16. in src/wallet/ismine.h:20 in efbda5f9b4 outdated 
      22 | -    ISMINE_SPENDABLE  = 1 << 1,
  23 | -    ISMINE_USED       = 1 << 2,
  24 | -    ISMINE_ALL        = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
  25 | -    ISMINE_ALL_USED   = ISMINE_ALL | ISMINE_USED,
  26 | -    ISMINE_ENUM_ELEMENTS,
  27 | +    ISMINE_NO         = 0,                                    //!< the scriptPubKey is not contained in the wallet
    jonatack commented at 10:04 AM on November 7, 2019:

    Suggestion: remove "contained"

    HAOYUatHZ commented at 7:50 AM on November 13, 2019:

    Suggestion: remove "contained"

    fixed

  17. jonatack commented at 10:12 AM on November 7, 2019: member

    Concept ACK. A few nits below. It might be good to squash the commits.

  18. HAOYUatHZ referenced this in commit c9b6527177 on Nov 13, 2019
  19. HAOYUatHZ force-pushed on Nov 13, 2019
  20. laanwj removed the label Waiting for author on Nov 18, 2019
  21. HAOYUatHZ commented at 2:54 PM on December 2, 2019: contributor

    Hi @laanwj this PR has been open for a while, does it look good to you? :D

  22. fanquake requested review from MarcoFalke on Dec 2, 2019
  23. laanwj commented at 2:20 PM on December 3, 2019: member

    yes it looks ok to me, but someone who is closer with the wallet code should make sure it's correct

  24. fanquake requested review from achow101 on Dec 3, 2019
  25. achow101 commented at 10:36 PM on December 3, 2019: member

    Concept meh.

    I like having more documentation, but soon, the definition of some types is both going to change and have two meanings, depending on the ScriptPubKeyMan in use.

    For example, ISMINE_SPENDABLE currently means that the wallet has enough private keys to produce a valid scriptSig/scriptWitness. But once we get descriptor wallets and other future ScriptPubKeyMan implementations, ISMINE_SPENDABLE just becomes that the scriptPubKey matches what we are looking for regardless of whether we are able to spend the outputs or not. However because LegacyScriptPubKeyMan will still be around, the first definition is also still correct. And in that second case, ISMINE_WATCHONLY becomes meaningless and is not used at all.

    I don't think that there's really a concise way to document that inline. So we would need to have a block comment which documents the meanings under the LegacyScriptPubKeyMan model and the "future ScriptPubKeyMans" model.

  26. HAOYUatHZ commented at 11:55 PM on December 3, 2019: contributor

    meh

    I see. So should I wait for future implementations? Will adding "future ScriptPubKeyMans" doc be confusing ATM?

  27. HAOYUatHZ commented at 5:59 AM on December 5, 2019: contributor

    Concept meh.

    I like having more documentation, but soon, the definition of some types is both going to change and have two meanings, depending on the ScriptPubKeyMan in use.

    For example, ISMINE_SPENDABLE currently means that the wallet has enough private keys to produce a valid scriptSig/scriptWitness. But once we get descriptor wallets and other future ScriptPubKeyMan implementations, ISMINE_SPENDABLE just becomes that the scriptPubKey matches what we are looking for regardless of whether we are able to spend the outputs or not. However because LegacyScriptPubKeyMan will still be around, the first definition is also still correct. And in that second case, ISMINE_WATCHONLY becomes meaningless and is not used at all.

    I don't think that there's really a concise way to document that inline. So we would need to have a block comment which documents the meanings under the LegacyScriptPubKeyMan model and the "future ScriptPubKeyMans" model.

    I see. So should I wait for future implementations? Will adding "future ScriptPubKeyMans" doc be confusing ATM?

  28. laanwj commented at 8:19 PM on January 20, 2020: member

    I like having more documentation, but soon, the definition of some types is both going to change and have two meanings, depending on the ScriptPubKeyMan in use.

    If you disagree with the concept of documenting isminetype, please post in #17217. It's probably better to close that in that case so that people don't waste time trying to document it in a PR that gets stuck.

  29. meshcollider commented at 2:29 AM on April 16, 2020: contributor

    I'm concept ACK on this tbh, I see what Andrew means but the documentation is still useful, and I think we can keep the two alternative descriptions concise enough that it isn't bad to have it inline.

  30. in src/wallet/ismine.h:21 in c9b6527177 outdated 
      23 | -    ISMINE_USED       = 1 << 2,
  24 | -    ISMINE_ALL        = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
  25 | -    ISMINE_ALL_USED   = ISMINE_ALL | ISMINE_USED,
  26 | -    ISMINE_ENUM_ELEMENTS,
  27 | +    ISMINE_NO         = 0,                                    //!< the scriptPubKey is not in the wallet
  28 | +    ISMINE_WATCH_ONLY = 1 << 0,                               //!< the scriptPubKey has been imported into the wallet
    achow101 commented at 5:45 PM on June 19, 2020:

    This should note that this is only used for legacy wallets.

  31. in src/wallet/ismine.h:22 in c9b6527177 outdated 
      24 | -    ISMINE_ALL        = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
  25 | -    ISMINE_ALL_USED   = ISMINE_ALL | ISMINE_USED,
  26 | -    ISMINE_ENUM_ELEMENTS,
  27 | +    ISMINE_NO         = 0,                                    //!< the scriptPubKey is not in the wallet
  28 | +    ISMINE_WATCH_ONLY = 1 << 0,                               //!< the scriptPubKey has been imported into the wallet
  29 | +    ISMINE_SPENDABLE  = 1 << 1,                               //!< the scriptPubKey corresponds to an address owned by the wallet user (can spend with the private key)
    achow101 commented at 5:47 PM on June 19, 2020:

    This should note that needing a private key is only for legacy wallets.

    It would be good to mention somewhere that DescriptorScriptPubKeyMan (and future ScriptPubKeyMan implementations) will only use ISMINE_NO and ISMINE_SPENDABLE, essentially treating this as a boolean (except for ISMINE_USED which is kind of orthogonal to the other isminetypes).

  32. achow101 commented at 5:48 PM on June 19, 2020: member

    Since descriptor wallets has been merged, I think we can update this to account for the semantic changes from there.

  33. HAOYUatHZ commented at 7:48 AM on June 21, 2020: contributor

    Since descriptor wallets has been merged, I think we can update this to account for the semantic changes from there.

    No problem! I will read related PRs and work on that!

  34. meshcollider added the label Waiting for author on Jul 11, 2020
  35. S3RK commented at 2:05 AM on October 26, 2020: member

    @HAOYUatHZ do you feel like updating this PR? Otherwise I'll pick it up

  36. HAOYUatHZ commented at 2:27 AM on October 26, 2020: contributor

    @HAOYUatHZ do you feel like updating this PR? Otherwise I'll pick it up @S3RK sorry I forgot about it. I will update it this week

  37. HAOYUatHZ referenced this in commit 2cf5866337 on Nov 1, 2020
  38. HAOYUatHZ force-pushed on Nov 1, 2020
  39. HAOYUatHZ referenced this in commit f54cf2808b on Nov 1, 2020
  40. HAOYUatHZ force-pushed on Nov 1, 2020
  41. HAOYUatHZ commented at 6:25 AM on November 1, 2020: contributor

    Maybe take a look again? @achow101 @S3RK

  42. in src/wallet/ismine.h:27 in f54cf2808b outdated 
      24 | + * 
  25 | + * For LegacyScriptPubKeyMan,
  26 | + * ISMINE_NO: the scriptPubKey is not in the wallet;
  27 | + * ISMINE_WATCH_ONLY: the scriptPubKey has been imported into the wallet;
  28 | + * ISMINE_SPENDABLE: the scriptPubKey corresponds to an address owned by the wallet user (can spend with the private key);
  29 | + * ISMINE_USED: the scriptPubKey corresponds to a used address owned by the wallet user;
    achow101 commented at 4:10 PM on November 1, 2020:

    ISMINE_USED is used by all ScriptPubKeyMan types, not just legacy.

    HAOYUatHZ commented at 5:11 AM on November 2, 2020:

    Thx! Anything else I need to address?

  43. HAOYUatHZ force-pushed on Nov 2, 2020
  44. HAOYUatHZ force-pushed on Nov 2, 2020
  45. HAOYUatHZ referenced this in commit 935e2a8343 on Nov 2, 2020
  46. HAOYUatHZ referenced this in commit 1bb2f52b7e on Nov 2, 2020
  47. HAOYUatHZ force-pushed on Nov 2, 2020
  48. achow101 commented at 4:23 PM on November 2, 2020: member

    ACK 1bb2f52b7ece307fbf4c87c5da6633ca8574143a

    The commit message should be cleaned up to not contain the entire history of changes. Also we don't @ mention people in commits as they will receive a notification every time someone pushes that particular commit to a repo.

  49. HAOYUatHZ force-pushed on Nov 3, 2020
  50. HAOYUatHZ commented at 12:28 AM on November 3, 2020: contributor

    ACK 1bb2f52

    The commit message should be cleaned up to not contain the entire history of changes. Also we don't @ mention people in commits as they will receive a notification every time someone pushes that particular commit to a repo.

    I see. Thanks!

  51. in src/wallet/ismine.h:34 in 2400c3aadc outdated 
      29 | + * ISMINE_ALL_USED: all ISMINE flags including USED;
  30 | + * ISMINE_ENUM_ELEMENTS: the number of isminetype enum elements.
  31 | + * 
  32 | + * For DescriptorScriptPubKeyMan and future ScriptPubKeyMan,
  33 | + * ISMINE_NO: the scriptPubKey is not in the wallet;
  34 | + * ISMINE_SPENDABLE: the scriptPubKey matches a scriptPubKey in the wallet.
    ryanofsky commented at 1:33 AM on November 11, 2020:

    In commit "doc: Add developer documentation to isminetype" (2400c3aadc2e3c87f5a1eda862bbb45213e9898f)

    No need to change and this should all be deduplicated later anyway, but from #17350 (comment) it sounded like maybe this should explicitly say ISMINE_SPENDABLE for descriptor wallets doesn't mean spendable

  52. in src/wallet/ismine.h:20 in 2400c3aadc outdated 
      13 | @@ -14,7 +14,26 @@
  14 |  class CWallet;
  15 |  class CScript;
  16 |  
  17 | -/** IsMine() return codes */
  18 | +/**
  19 | + * IsMine() return codes, which depend on ScriptPubKeyMan implementation.
  20 | + * Not every ScriptPubKeyMan covers all types, please refer to
  21 | + * doc/release-notes.md#ismine-semantics for better understanding.
    ryanofsky commented at 1:34 AM on November 11, 2020:

    In commit "doc: Add developer documentation to isminetype" (2400c3aadc2e3c87f5a1eda862bbb45213e9898f)

    doc/release-notes.md#ismine-semantics doesn't seem to exist

  53. ryanofsky approved
  54. ryanofsky commented at 1:37 AM on November 11, 2020: member

    Code review ACK 2400c3aadc2e3c87f5a1eda862bbb45213e9898f. Seems all right

  55. in src/wallet/ismine.h:21 in 2400c3aadc outdated 
      13 | @@ -14,7 +14,26 @@
  14 |  class CWallet;
  15 |  class CScript;
  16 |  
  17 | -/** IsMine() return codes */
  18 | +/**
  19 | + * IsMine() return codes, which depend on ScriptPubKeyMan implementation.
  20 | + * Not every ScriptPubKeyMan covers all types, please refer to
  21 | + * doc/release-notes.md#ismine-semantics for better understanding.
  22 | + *
    adamjonas commented at 1:40 PM on December 10, 2020:

    Looks like the trailing whitespace on this line, line 30, and line 35 are tripping the whitespace linter.

  56. meshcollider removed the label Waiting for author on Jan 19, 2021
  57. meshcollider added the label Waiting for author on Jan 19, 2021
  58. meshcollider commented at 12:23 AM on January 19, 2021: contributor

    @HAOYUatHZ please fix the whitespace issue so this can be merged :)

  59. HAOYUatHZ commented at 12:59 AM on January 19, 2021: contributor

    @HAOYUatHZ please fix the whitespace issue so this can be merged :)

    sorry for being late. fix!

  60. HAOYUatHZ force-pushed on Jan 19, 2021
  61. doc: Add developer documentation to isminetype 40f05647ee
  62. HAOYUatHZ force-pushed on Jan 19, 2021
  63. meshcollider removed the label Waiting for author on Jan 25, 2021
  64. meshcollider commented at 11:44 PM on January 25, 2021: contributor

    utACK 40f05647ee298f8419df795942248d9ded3beb43

  65. meshcollider merged this on Jan 26, 2021
  66. meshcollider closed this on Jan 26, 2021
  67. HAOYUatHZ deleted the branch on Jan 26, 2021
  68. sidhujag referenced this in commit 67f2bfb0f0 on Jan 26, 2021
  69. PastaPastaPasta referenced this in commit aac5f63e6d on Apr 3, 2022
  70. PastaPastaPasta referenced this in commit 36e5e6c85c on Apr 4, 2022
  71. DrahtBot locked this on Aug 16, 2022
Contributors
HAOYUatHZlaanwjpromagjonatackachow101meshcolliderS3RKryanofskyMarcoFalkeadamjonas


laanwjpromagMarcoFalke

Labels
DocsWallet
Linked (view graph)
#17217 doc: Add developer documentation to isminetype
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 09:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me