Add tests and error handling to DecodeExtPubKey/DecodeExtKey. Add [[nodiscard]]. #13971

pull practicalswift wants to merge 4 commits into bitcoin:master from practicalswift:DecodeExtKey changing 6 files +86 −15
  1. practicalswift commented at 9:11 PM on August 14, 2018: contributor

    Add error handling to DecodeExtPubKey/DecodeExtKey and make sure return value is checked (via [[nodiscard]]).

    Prior to this commit:

    $ cat > test.cpp
…
int main(void) {
  CExtPubKey key = DecodeExtPubKey("foo");
  std::cout << key.nChild << "\n";
}
^D
$ g++ -o test test.cpp
$ ./test
109452364
$ ./test
-1789494196
$ ./test
-1568076724
$ ./test
1483189324
$ ./test
-1168606132
  2. in src/script/descriptor.cpp:470 in 5beab3bf01 outdated 
     468 | -    CExtPubKey extpubkey = DecodeExtPubKey(str);
 469 | -    if (!extkey.key.IsValid() && !extpubkey.pubkey.IsValid()) return nullptr;
 470 | +    CExtKey extkey;
 471 | +    bool validExtKey = DecodeExtKey(str, extkey);
 472 | +    CExtPubKey extpubkey;
 473 | +    bool validExtPubKey = DecodeExtPubKey(str, extpubkey);
    domob1812 commented at 6:09 AM on August 15, 2018:

    Nit: This and validExtKey above can be const.

  3. in src/script/descriptor.cpp:471 in 5beab3bf01 outdated 
     469 | -    if (!extkey.key.IsValid() && !extpubkey.pubkey.IsValid()) return nullptr;
 470 | +    CExtKey extkey;
 471 | +    bool validExtKey = DecodeExtKey(str, extkey);
 472 | +    CExtPubKey extpubkey;
 473 | +    bool validExtPubKey = DecodeExtPubKey(str, extpubkey);
 474 | +    if (!(validExtKey && extkey.key.IsValid()) && !(validExtPubKey && extpubkey.pubkey.IsValid())) return nullptr;
    domob1812 commented at 6:10 AM on August 15, 2018:

    Why do you check IsValid here again? That's already contained in the return value you added. So either you should make it explicit in DecodeExtKey's contract that the returned key is valid if true is returned and remove the check here (preferred for me), or you should not check the key for validity in DecodeExtKey and have the callers check it - but not both.

  4. domob1812 approved
  5. domob1812 commented at 6:11 AM on August 15, 2018: contributor

    utACK 6ade9e8e463ff1843a025ffb7d79fe76870c6342 with a few nits

  6. Add error checking to DecodeExtPubKey/DecodeExtKey 8b2e52c307
  7. practicalswift force-pushed on Aug 15, 2018
  8. practicalswift renamed this:
    Add error handling to DecodeExtPubKey/DecodeExtKey and make sure return value is checked (via [[nodiscard]])
    Add error handling to DecodeExtPubKey/DecodeExtKey. Add [[nodiscard]]. Add tests.
    on Aug 15, 2018
  9. practicalswift renamed this:
    Add error handling to DecodeExtPubKey/DecodeExtKey. Add [[nodiscard]]. Add tests.
    Add error handling to DecodeExtPubKey/DecodeExtKey. Add tests. Add [[nodiscard]].
    on Aug 15, 2018
  10. practicalswift force-pushed on Aug 15, 2018
  11. practicalswift commented at 1:46 PM on August 15, 2018: contributor

    @domob1812 Thanks for reviewing. Feedback addressed. Please re-review :-)

  12. practicalswift renamed this:
    Add error handling to DecodeExtPubKey/DecodeExtKey. Add tests. Add [[nodiscard]].
    Add testa and error handling to DecodeExtPubKey/DecodeExtKey. Add [[nodiscard]].
    on Aug 15, 2018
  13. practicalswift renamed this:
    Add testa and error handling to DecodeExtPubKey/DecodeExtKey. Add [[nodiscard]].
    Add tests and error handling to DecodeExtPubKey/DecodeExtKey. Add [[nodiscard]].
    on Aug 15, 2018
  14. domob1812 commented at 4:31 PM on August 18, 2018: contributor

    Thanks for the fixes, looks good to me - utACK 067409221dde450ee9ba5dd7a545d9640ed7608b.

  15. in src/test/bip32_tests.cpp:156 in 067409221d outdated 
     153 | +        BOOST_CHECK(!DecodeExtPubKey(derive.pub + "\a\n", extpubkeyTmp));
 154 | +        BOOST_CHECK(DecodeExtPubKey(derive.pub + "\r", extpubkeyTmp));
 155 | +        BOOST_CHECK(!DecodeExtPubKey(derive.pub + "\r\a", extpubkeyTmp));
 156 | +
 157 | +        // Test expected similarities/differences in return values between DecodeExtKey and DecodeExtPubKey
 158 | +        BOOST_CHECK(DecodeExtKey(derive.prv, extkeyTmp) != DecodeExtPubKey(derive.prv, extpubkeyTmp));
    ryanofsky commented at 5:28 PM on August 21, 2018:

    I don't understand the style used in this part of the test. It seems like the test would be both more useful and more readable with:

    BOOST_CHECK(DecodeExtKey(derive.prv, extkeyTmp) != DecodeExtPubKey(derive.prv, extpubkeyTmp));

    replaced by something like:

    BOOST_CHECK_EQUAL(DecodeExtKey(derive.prv, extkeyTmp, true);
BOOST_CHECK_EQUAL(DecodeExtPubKey(derive.prv, extpubkeyTmp), false);
  16. ryanofsky commented at 5:30 PM on August 21, 2018: member

    utACK 067409221dde450ee9ba5dd7a545d9640ed7608b

  17. practicalswift force-pushed on Aug 21, 2018
  18. practicalswift commented at 9:42 PM on August 21, 2018: contributor

    @ryanofsky Thanks for reviewing. Good point! Feedback now addressed. Please re-review! :-)

  19. practicalswift commented at 7:58 AM on August 23, 2018: contributor

    AppVeyor build failure seems unrelated? :-)

  20. ryanofsky commented at 4:33 PM on August 24, 2018: member

    AppVeyor build failure seems unrelated? :-)

    It looks related to lack of visual c++ support for __attribute__. You can probably avoid it by checking for _MSC_VER. It's probably fine to do this in another PR, though it might be nice to get out of the way now.

    Relevant logs:

    c:\projects\bitcoin\src\key_io.h(21): error C2065: 'warn_unused_result': undeclared identifier [C:\projects\bitcoin\build_msvc\libbitcoin_wallet\libbitcoin_wallet.vcxproj]
c:\projects\bitcoin\src\key_io.h(21): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [C:\projects\bitcoin\build_msvc\libbitcoin_wallet\libbitcoin_wallet.vcxproj]
c:\projects\bitcoin\src\key_io.h(21): error C2448: '__attribute__': function-style initializer appears to be a function definition [C:\projects\bitcoin\build_msvc\libbitcoin_wallet\libbitcoin_wallet.vcxproj]
c:\projects\bitcoin\src\key_io.h(24): error C2065: 'warn_unused_result': undeclared identifier [C:\projects\bitcoin\build_msvc\libbitcoin_wallet\libbitcoin_wallet.vcxproj]
c:\projects\bitcoin\src\key_io.h(24): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [C:\projects\bitcoin\build_msvc\libbitcoin_wallet\libbitcoin_wallet.vcxproj]
c:\projects\bitcoin\src\key_io.h(24): error C2374: '__attribute__': redefinition; multiple initialization [C:\projects\bitcoin\build_msvc\libbitcoin_wallet\libbitcoin_wallet.vcxproj]
  c:\projects\bitcoin\src\key_io.h(21): note: see declaration of '__attribute__'
c:\projects\bitcoin\src\key_io.h(24): error C2448: '__attribute__': function-style initializer appears to be a function definition [C:\projects\bitcoin\build_msvc\libbitcoin_wallet\libbitcoin_wallet.vcxproj]
  21. ryanofsky commented at 4:34 PM on August 24, 2018: member

    utACK a7db4f76e3700f17f3d627eee73b338f86b9c254. Only change since previous review was suggested test cleanup (Thanks!)

  22. Add #define NODISCARD a736ecd5c4
  23. Add NODISCARD to DecodeExtPubKey/DecodeExtKey b246e43b26
  24. Add tests for DecodeExtKey and DecodeExtPubKey 273a622e05
  25. practicalswift force-pushed on Aug 26, 2018
  26. DrahtBot commented at 9:00 PM on August 26, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->Reviewers, this pull request conflicts with the following ones:

    • #14224 (Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations by practicalswift)
    • #14194 (Annotate unused parameters with [[maybe_unused]] by practicalswift)

    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.

  27. practicalswift commented at 4:12 PM on August 27, 2018: contributor

    @ryanofsky Updated to work with Visual C++ as well. Please re-review :-) @domob1812 Please re-review :-)

  28. in src/attributes.h:14 in 273a622e05 
       9 | +#if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard)
  10 | +#  define NODISCARD [[nodiscard]]
  11 | +#elif defined(_MSC_VER) && _MSC_VER >= 1700
  12 | +#  define NODISCARD _Check_return_
  13 | +#else
  14 | +#  define NODISCARD __attribute__((warn_unused_result))
    luke-jr commented at 8:01 AM on August 28, 2018:

    This isn't guaranteed to be valid AFAIK?

    practicalswift commented at 7:43 AM on August 29, 2018:

    Do you have any suggestion on a more proper way to do it?

    FWIW this is how Chromium does it :-)

    #undef WARN_UNUSED_RESULT
#if defined(COMPILER_GCC) || defined(__clang__)
#define WARN_UNUSED_RESULT __attribute__((warn_unused_result))
#else
#define WARN_UNUSED_RESULT
#endif
  29. in src/key_io.cpp:162 in 273a622e05 
     158 | @@ -159,17 +159,17 @@ std::string EncodeSecret(const CKey& key)
 159 |      return ret;
 160 |  }
 161 |  
 162 | -CExtPubKey DecodeExtPubKey(const std::string& str)
 163 | +bool DecodeExtPubKey(const std::string& str, CExtPubKey& extpubkey)
    luke-jr commented at 8:03 AM on August 28, 2018:

    I think the current API is better. Just check .IsValid() in the caller...

  30. ryanofsky approved
  31. ryanofsky commented at 8:56 PM on September 14, 2018: member

    utACK 273a622e058c8eafaac56393e0a3bf7d8868ab4f. Only change since previous review is adding NODISCARD for visual c

  32. practicalswift closed this on Oct 19, 2018
  33. practicalswift deleted the branch on Apr 10, 2021
  34. DrahtBot locked this on Aug 18, 2022
Contributors
practicalswiftdomob1812ryanofskyDrahtBotluke-jr
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-16 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me