doc: getaddressinfo[isscript] is optional #30457

1. 
   maflcko commented at 5:10 pm on July 16, 2024: member

   isscript is unknown for unknown witness versions, so it should be marked optional in the docs

   Fixes https://github.com/bitcoin/bitcoin/issues/30456

2. 
   DrahtBot commented at 5:10 pm on July 16, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	stickies-v, tdb3

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Docs on Jul 16, 2024
4. in test/functional/rpc_invalid_address_message.py:114 in fa63ecc720 outdated

   110@@ -109,6 +111,7 @@ def test_getaddressinfo(self):
   111         assert_raises_rpc_error(-5, "Invalid or unsupported Segwit (Bech32) or Base58 encoding.", node.getaddressinfo, BECH32_INVALID_PREFIX)
   112         assert_raises_rpc_error(-5, "Invalid or unsupported Base58-encoded address.", node.getaddressinfo, BASE58_INVALID_PREFIX)
   113         assert_raises_rpc_error(-5, "Invalid or unsupported Segwit (Bech32) or Base58 encoding.", node.getaddressinfo, INVALID_ADDRESS)
   114+        "isscript" not in node.getaddressinfo(BECH32_VALID_UNKNOWN_WITNESS)
   

   ---

   ---

   

   stickies-v commented at 6:05 pm on July 16, 2024:

   This is a no-op, I think you meant to assert?

   0        assert "isscript" not in node.getaddressinfo(BECH32_VALID_UNKNOWN_WITNESS)
   

   ---

   

   maflcko commented at 4:52 am on July 17, 2024:

   thanks, fixed
5. in src/wallet/rpc/addresses.cpp:537 in fa63ecc720 outdated

   533@@ -534,7 +534,7 @@ RPCHelpMan getaddressinfo()
   534                         {RPCResult::Type::BOOL, "solvable", "If we know how to spend coins sent to this address, ignoring the possible lack of private keys."},
   535                         {RPCResult::Type::STR, "desc", /*optional=*/true, "A descriptor for spending coins sent to this address (only when solvable)."},
   536                         {RPCResult::Type::STR, "parent_desc", /*optional=*/true, "The descriptor used to derive this address if this is a descriptor wallet"},
   537-                        {RPCResult::Type::BOOL, "isscript", "If the key is a script."},
   538+                        {RPCResult::Type::BOOL, "isscript", /*optional=*/true, "If the key is a script."},
   

   ---

   ---

   

   stickies-v commented at 6:16 pm on July 16, 2024:

   unrelated: that is quite the field description 😳 Time to start thinking about deprecating isscript and iswitness, perhaps?

   ---

   

   instagibbs commented at 2:09 pm on July 17, 2024:

   yeah I was having a real hard time understanding what a lot of these fields actually mean…

   ---

   

   maflcko commented at 2:15 pm on July 17, 2024:

   I think it means “some kind of pay to script hash”, that is SH or WSH.

   I guess it would be clearer to just return an enum as string, rather than a collection of boolean fields.

6. 
   stickies-v commented at 6:17 pm on July 16, 2024: contributor

   code LGTM fa63ecc72045ffa771448941e1fe066f7421f640 but I think the test needs to be fixed
7. in test/functional/rpc_invalid_address_message.py:15 in fa63ecc720 outdated

   11@@ -12,6 +12,7 @@
   12 )
   13 
   14 BECH32_VALID = 'bcrt1qtmp74ayg7p24uslctssvjm06q5phz4yrxucgnv'
   15+BECH32_VALID_UNKNOWN_WITNESS = 'bcrt1p424qxxyd0r'
   

   ---

   ---

   

   tdb3 commented at 0:59 am on July 17, 2024:

   Maybe it’s worth adding a line for segwit v0 as well? (e.g. witness program not 20 or 32 bytes long)?

   also

   nit: might increase clarity

   0- BECH32_VALID_UNKNOWN_WITNESS = 'bcrt1p424qxxyd0r'
   1+ BECH32_VALID_UNKNOWN_WITNESS_PROG = 'bcrt1p424qxxyd0r'
   

   ---

   

   maflcko commented at 4:53 am on July 17, 2024:

   Left the nit for now.
8. 
   tdb3 commented at 1:06 am on July 17, 2024: contributor

   Approach ACK Thanks for increasing clarity in RPC. Looks like there’s a missing assert in test_getaddressinfo(). Left a nit.
9. doc: getaddressinfo[isscript] is optional fa6390df20
10. maflcko force-pushed on Jul 17, 2024
11. DrahtBot added the label CI failed on Jul 17, 2024
12. DrahtBot removed the label CI failed on Jul 17, 2024
13. stickies-v approved
14. 
    stickies-v commented at 10:19 am on July 17, 2024: contributor

    ACK fa6390df205513319f28e35e3e17c40ecaa7d731
15. DrahtBot requested review from tdb3 on Jul 17, 2024
16. tdb3 approved
17. 
    tdb3 commented at 11:31 am on July 17, 2024: contributor

    ACK fa6390df205513319f28e35e3e17c40ecaa7d731
18. fanquake merged this on Jul 17, 2024
19. fanquake closed this on Jul 17, 2024

    ---

20. maflcko deleted the branch on Jul 17, 2024

Contributors
maflcko DrahtBot stickies-v instagibbs tdb3

Labels
Docs