doc: getaddressinfo[isscript] is optional #30457

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2407-doc changing 2 files +6 −3

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
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.
DrahtBot added the label Docs on Jul 16, 2024

Type	Reviewers
ACK	stickies-v, tdb3

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?

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

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

thanks, fixed

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.

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

code LGTM fa63ecc72045ffa771448941e1fe066f7421f640 but I think the test needs to be fixed
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 12: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

- BECH32_VALID_UNKNOWN_WITNESS = 'bcrt1p424qxxyd0r' + BECH32_VALID_UNKNOWN_WITNESS_PROG = 'bcrt1p424qxxyd0r'
maflcko commented at 4:53 AM on July 17, 2024:

Left the nit for now.
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.
doc: getaddressinfo[isscript] is optional fa6390df20
maflcko force-pushed on Jul 17, 2024
DrahtBot added the label CI failed on Jul 17, 2024
DrahtBot removed the label CI failed on Jul 17, 2024
stickies-v approved
stickies-v commented at 10:19 AM on July 17, 2024: contributor

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

ACK fa6390df205513319f28e35e3e17c40ecaa7d731
fanquake merged this on Jul 17, 2024
fanquake closed this on Jul 17, 2024
maflcko deleted the branch on Jul 17, 2024
luke-jr referenced this in commit e9896f8150 on Aug 1, 2024
bitcoin locked this on Jul 18, 2025

Contributors

Labels

Linked (view graph)

#30456 getaddressinfo: complains missing `isscript` when called on unknown witness version

doc: getaddressinfo[isscript] is optional #30457

Code Coverage

Reviews