rpc: Fix invalid bech32 address handling #27727

1. 
   maflcko commented at 11:10 am on May 23, 2023: member

   Currently the handling of invalid bech32(m) addresses over RPC has many issues:

   • No error for invalid addresses is reported, leading to internal bugs via CHECK_NONFATAL, see #27723
   • The error messages use “data size” (the meaning of which is unclear to the user, because the witness program data and bech32 section data are related but different) when they mean “program size”

   Fix all issues. Also, use the BIP 173 and BIP 350 test vectors.

2. 
   DrahtBot commented at 11:10 am on May 23, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101, brunoerg

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #27260 (Enhanced error messages for invalid network prefix during address parsing. by russeree)

   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.

3. DrahtBot added the label RPC/REST/ZMQ on May 23, 2023
4. maflcko added the label Needs backport (22.x) on May 23, 2023
5. maflcko added the label Needs backport (23.x) on May 23, 2023
6. maflcko added the label Needs backport (24.x) on May 23, 2023
7. maflcko added the label Needs backport (25.x) on May 23, 2023
8. maflcko added this to the milestone 25.0 on May 23, 2023
9. fanquake removed the label Needs backport (22.x) on May 23, 2023
10. fanquake removed the label Needs backport (23.x) on May 23, 2023
11. fanquake removed the label Needs backport (24.x) on May 23, 2023
12. fanquake added the label Needs backport (22.x) on May 23, 2023
13. fanquake added the label Needs backport (23.x) on May 23, 2023
14. fanquake added the label Needs backport (24.x) on May 23, 2023
15. DrahtBot added the label CI failed on May 23, 2023
16. 
    maflcko commented at 11:22 am on May 23, 2023: member

    (I’d say backport isn’t blocking any release, but if a previous release is done anyway for other reasons, it may be good to backport)
17. maflcko force-pushed on May 23, 2023
18. rpc: Fix invalid bech32 handling eeee55f928
19. maflcko force-pushed on May 23, 2023
20. maflcko renamed this:
    rpc: Fix invalid bech32 handling
    rpc: Fix invalid bech32 address handling
    on May 23, 2023
21. DrahtBot removed the label CI failed on May 23, 2023
22. 
    achow101 commented at 4:16 pm on May 23, 2023: member

    ACK eeee55f9288740747b6e8d806ce8177fd92635cf
23. brunoerg approved
24. 
    brunoerg commented at 9:53 pm on May 23, 2023: contributor

    crACK eeee55f9288740747b6e8d806ce8177fd92635cf
25. in test/functional/rpc_validateaddress.py:177 in eeee55f928

    172+class ValidateAddressMainTest(BitcoinTestFramework):
    173+    def set_test_params(self):
    174+        self.setup_clean_chain = True
    175+        self.chain = ""  # main
    176+        self.num_nodes = 1
    177+        self.extra_args = [["-prune=899"]] * self.num_nodes
    

    ---

    ---

    

    brunoerg commented at 9:55 pm on May 23, 2023:

    feel free to ignore:

    since it’s just 1 node

    0        self.extra_args = [["-prune=899"]]
    

26. 
    russeree commented at 10:27 am on May 24, 2023: contributor

    Very cool! I am not in a position to ack,nack. I have noticed a potential grammatical issue. The term ‘byte’ is correctly used only when the size of the program/data is 1. In all other cases, including 0, 2, 3, 4, …, the correct term would be ‘bytes’.

    For instance, in the error string ‘Invalid Bech32 v0 address program size (16 byte), per BIP141’, ‘byte’ should be replaced with ‘bytes’ as the size is 16."

    An idea I have for a complete solution would be.

    0std::string byteStr = data.size() == 1 ? "byte" : "bytes";
    1strprintf("Invalid Bech32 v0 address program size (%d %s), per BIP141", data.size(), byteStr);
    

27. 
    maflcko commented at 11:23 am on May 24, 2023: member

    @russeree Thx, I think it is fine if you open a follow-up after merge.
28. achow101 merged this on May 24, 2023
29. achow101 closed this on May 24, 2023

    ---

30. maflcko deleted the branch on May 25, 2023
31. fanquake referenced this in commit 796e1145a9 on May 25, 2023
32. fanquake removed the label Needs backport (25.x) on May 25, 2023
33. fanquake referenced this in commit 9d098af5a9 on May 25, 2023
34. fanquake referenced this in commit d23743b0c5 on May 25, 2023
35. fanquake referenced this in commit c2e9214eff on May 25, 2023
36. fanquake removed the label Needs backport (24.x) on May 25, 2023
37. 
    fanquake commented at 2:15 pm on May 25, 2023: member

    Backported to 25.x in #27750 and 24.x in #27755.
38. fanquake referenced this in commit d987707208 on May 25, 2023
39. fanquake removed the label Needs backport (23.x) on May 25, 2023
40. 
    fanquake commented at 2:36 pm on May 25, 2023: member

    23.x done in #27756, using the same change as 24.x.
41. fanquake referenced this in commit 725c3dc2dd on May 25, 2023
42. maflcko removed the label Needs backport (22.x) on May 25, 2023
43. 
    maflcko commented at 2:51 pm on May 25, 2023: member

    i guess 22.x is EOL, so I removed that
44. sidhujag referenced this in commit cae667992b on May 26, 2023
45. achow101 referenced this in commit a537cb0b87 on Jun 1, 2023
46. achow101 referenced this in commit b7ded852bb on Jun 1, 2023
47. bitcoin locked this on May 24, 2024

Contributors
maflcko DrahtBot achow101 brunoerg russeree fanquake

Labels
RPC/REST/ZMQ

Milestone
25.0