tests: Remove OldSetKeyFromPassphrase/OldEncrypt/OldDecrypt #11024

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:OldDecrypt-cleanup changing 1 files +11 −122
  1. practicalswift commented at 5:09 pm on August 10, 2017: contributor

    Reduces the number of non-free:d allocs with four (Δ in use at exit = -928 bytes).

    With this patch applied:

     0$ valgrind --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite --run_test=wallet_crypto
 1…
 2==20243== HEAP SUMMARY:
 3==20243==     in use at exit: 72,704 bytes in 1 blocks
 4==20243==   total heap usage: 53,138 allocs, 53,137 frees, 49,600,420 bytes allocated
 5==20243==
 6==20243== 72,704 bytes in 1 blocks are still reachable in loss record 1 of 1
 7==20243==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
 8==20243==    by 0x6AA5EFF: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
 9==20243==    by 0x40106B9: call_init.part.0 (dl-init.c:72)
10==20243==    by 0x40107CA: call_init (dl-init.c:30)
11==20243==    by 0x40107CA: _dl_init (dl-init.c:120)
12==20243==    by 0x4000C69: ??? (in /lib/x86_64-linux-gnu/ld-2.23.so)
13==20243==    by 0x2: ???
14==20243==    by 0xFFF0006A2: ???
15==20243==    by 0xFFF0006B8: ???
16==20243==    by 0xFFF0006CF: ???
17==20243==
18==20243== LEAK SUMMARY:
19==20243==    definitely lost: 0 bytes in 0 blocks
20==20243==    indirectly lost: 0 bytes in 0 blocks
21==20243==      possibly lost: 0 bytes in 0 blocks
22==20243==    still reachable: 72,704 bytes in 1 blocks
23==20243==         suppressed: 0 bytes in 0 blocks

    Without this patch applied:

     0$ valgrind --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite --run_test=wallet_crypto
 1…
 2==19023== HEAP SUMMARY:
 3==19023==     in use at exit: 73,632 bytes in 5 blocks
 4==19023==   total heap usage: 52,718 allocs, 52,713 frees, 49,502,962 bytes allocated
 5==19023==
 6==19023== 24 bytes in 1 blocks are still reachable in loss record 1 of 5
 7==19023==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
 8==19023==    by 0x642DE77: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
 9==19023==    by 0x64E5665: lh_insert (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
10==19023==    by 0x64E7BB3: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
11==19023==    by 0x64E87AD: ERR_get_state (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
12==19023==    by 0x64E883D: ERR_put_error (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
13==19023==    by 0x64EAAE4: EVP_DecryptFinal_ex (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
14==19023==    by 0x3AD150: wallet_crypto::OldDecrypt(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, secure_allocator<unsigned char> >&, unsigned char const*, unsigned char const*) (crypto_tests.cpp:81)
15==19023==    by 0x3AF892: wallet_crypto::TestCrypter::TestDecrypt(CCrypter const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&) (crypto_tests.cpp:137)
16==19023==    by 0x3AD5E9: wallet_crypto::decrypt::test_method() (crypto_tests.cpp:223)
17==19023==    by 0x3ADC11: wallet_crypto::decrypt_invoker() (crypto_tests.cpp:216)
18==19023==    by 0x182596: invoke<void (*)()> (callback.hpp:56)
19==19023==    by 0x182596: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
20==19023==
21==19023== 128 bytes in 1 blocks are still reachable in loss record 2 of 5
22==19023==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
23==19023==    by 0x642DE77: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
24==19023==    by 0x64E5331: lh_new (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
25==19023==    by 0x64E7862: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
26==19023==    by 0x64E7B7F: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
27==19023==    by 0x64E87AD: ERR_get_state (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
28==19023==    by 0x64E883D: ERR_put_error (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
29==19023==    by 0x64EAAE4: EVP_DecryptFinal_ex (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
30==19023==    by 0x3AD150: wallet_crypto::OldDecrypt(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, secure_allocator<unsigned char> >&, unsigned char const*, unsigned char const*) (crypto_tests.cpp:81)
31==19023==    by 0x3AF892: wallet_crypto::TestCrypter::TestDecrypt(CCrypter const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&) (crypto_tests.cpp:137)
32==19023==    by 0x3AD5E9: wallet_crypto::decrypt::test_method() (crypto_tests.cpp:223)
33==19023==    by 0x3ADC11: wallet_crypto::decrypt_invoker() (crypto_tests.cpp:216)
34==19023==
35==19023== 176 bytes in 1 blocks are still reachable in loss record 3 of 5
36==19023==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
37==19023==    by 0x642DE77: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
38==19023==    by 0x64E530F: lh_new (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
39==19023==    by 0x64E7862: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
40==19023==    by 0x64E7B7F: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
41==19023==    by 0x64E87AD: ERR_get_state (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
42==19023==    by 0x64E883D: ERR_put_error (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
43==19023==    by 0x64EAAE4: EVP_DecryptFinal_ex (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
44==19023==    by 0x3AD150: wallet_crypto::OldDecrypt(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, secure_allocator<unsigned char> >&, unsigned char const*, unsigned char const*) (crypto_tests.cpp:81)
45==19023==    by 0x3AF892: wallet_crypto::TestCrypter::TestDecrypt(CCrypter const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&) (crypto_tests.cpp:137)
46==19023==    by 0x3AD5E9: wallet_crypto::decrypt::test_method() (crypto_tests.cpp:223)
47==19023==    by 0x3ADC11: wallet_crypto::decrypt_invoker() (crypto_tests.cpp:216)
48==19023==
49==19023== 600 bytes in 1 blocks are still reachable in loss record 4 of 5
50==19023==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
51==19023==    by 0x642DE77: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
52==19023==    by 0x64E8745: ERR_get_state (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
53==19023==    by 0x64E883D: ERR_put_error (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
54==19023==    by 0x64EAAE4: EVP_DecryptFinal_ex (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
55==19023==    by 0x3AD150: wallet_crypto::OldDecrypt(std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, secure_allocator<unsigned char> >&, unsigned char const*, unsigned char const*) (crypto_tests.cpp:81)
56==19023==    by 0x3AF892: wallet_crypto::TestCrypter::TestDecrypt(CCrypter const&, std::vector<unsigned char, std::allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> > const&) (crypto_tests.cpp:137)
57==19023==    by 0x3AD5E9: wallet_crypto::decrypt::test_method() (crypto_tests.cpp:223)
58==19023==    by 0x3ADC11: wallet_crypto::decrypt_invoker() (crypto_tests.cpp:216)
59==19023==    by 0x182596: invoke<void (*)()> (callback.hpp:56)
60==19023==    by 0x182596: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
61==19023==    by 0x596CCB0: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
62==19023==    by 0x594C995: boost::execution_monitor::catch_signals(boost::unit_test::callback0<int> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
63==19023==
64==19023== 72,704 bytes in 1 blocks are still reachable in loss record 5 of 5
65==19023==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
66==19023==    by 0x6AA5EFF: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
67==19023==    by 0x40106B9: call_init.part.0 (dl-init.c:72)
68==19023==    by 0x40107CA: call_init (dl-init.c:30)
69==19023==    by 0x40107CA: _dl_init (dl-init.c:120)
70==19023==    by 0x4000C69: ??? (in /lib/x86_64-linux-gnu/ld-2.23.so)
71==19023==    by 0x2: ???
72==19023==    by 0xFFF0006A2: ???
73==19023==    by 0xFFF0006B8: ???
74==19023==    by 0xFFF0006CF: ???
75==19023==
76==19023== LEAK SUMMARY:
77==19023==    definitely lost: 0 bytes in 0 blocks
78==19023==    indirectly lost: 0 bytes in 0 blocks
79==19023==      possibly lost: 0 bytes in 0 blocks
80==19023==    still reachable: 73,632 bytes in 5 blocks
81==19023==         suppressed: 0 bytes in 0 blocks
82==19023==
83==19023== For counts of detected and suppressed errors, rerun with: -v
84==19023== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
  2. practicalswift commented at 5:16 pm on August 10, 2017: contributor
    Note to reviewers: Let me know if this cleanup is better placed somewhere else (i.e. outside of OldDecrypt(…) to avoid doing it more than once). One alternative would be to put it in ~BasicTestingSetup(), but then it’ll run for unrelated tests as well.
  3. laanwj added the label Refactoring on Aug 10, 2017
  4. in src/wallet/test/crypto_tests.cpp:85 in b3afb6242e outdated 
    80@@ -80,8 +81,8 @@ bool OldDecrypt(const std::vector<unsigned char>& vchCiphertext, CKeyingMaterial
81     if (fOk) fOk = EVP_DecryptUpdate(ctx, &vchPlaintext[0], &nPLen, &vchCiphertext[0], nLen) != 0;
82     if (fOk) fOk = EVP_DecryptFinal_ex(ctx, (&vchPlaintext[0]) + nPLen, &nFLen) != 0;
83     EVP_CIPHER_CTX_cleanup(ctx);
84-
85     EVP_CIPHER_CTX_free(ctx);
86+    ERR_remove_thread_state(nullptr);
    laanwj commented at 6:49 pm on August 10, 2017:
    I’m not sure I understand this. OpenSSL API is confusing, where is the symmetric “create” for this remove/free? Surprised that this needs ERR_* just to deallocate a structure that comes out of the blue. Is this related to cleanup in ~CInit() in util.cpp?
    practicalswift commented at 9:02 pm on August 10, 2017:

    Yes, the OpenSSL API is full of surprises! :-)

    This is my understanding:

    OldDecrypt(…) (in crypto_tests.cpp) calls OpenSSL’s EVP_DecryptFinal_ex(…).

    If EVP_DecryptFinal_ex(…) encounters any errors then EVPerr(…) is called.

    EVPerr(…) is defined as:

    0#define EVPerr(f,r)  ERR_PUT_error(ERR_LIB_EVP,(f),(r),OPENSSL_FILE,OPENSSL_LINE)

    ERR_PUT_error(…) stores stores the errors in a thread local error queue for which the allocations are made using OPENSSL_zalloc(…) in ERR_get_state(…).

    practicalswift commented at 9:47 pm on August 10, 2017:
    ERR_remove_thread_state(nullptr) must be run for each thread as I’ve understood it, so I don’t think putting it in ~CInit() would be the right thing to do?
  5. gmaxwell commented at 10:23 pm on August 10, 2017: contributor
    Alternatively we could drop this test. I think it’s served its purpose now (our implementation is fine even across many systems) and hopefully OpenSSL will not be required to build without the GUI in 0.16.
  6. gmaxwell commented at 11:00 pm on August 10, 2017: contributor
    @sipa @theuni
  7. sipa commented at 11:02 pm on August 10, 2017: member
    I’d rather just get rid of OpenSSL… we’re very close to the point that it doesn’t have much added benefit over our internal RNG.
  8. practicalswift commented at 8:08 am on August 11, 2017: contributor

    @gmaxwell I’ve now added a commit that removes Old{SetKeyFromPassphrase,Encrypt,Decrypt}. Is that in accordance with your suggestion? :-)

    Let me know if it looks good and I’ll squash it into one commit.

  9. gmaxwell commented at 4:15 pm on August 11, 2017: contributor
    Looks good to me.
  10. MarcoFalke renamed this:
    tests: Free the OpenSSL error queue as part of the wallet_crypto/OldDecrypt test cleanup
    tests: Remove OldSetKeyFromPassphrase/OldEncrypt/OldDecrypt
    on Aug 11, 2017
  11. MarcoFalke added this to the milestone 0.16.0 on Aug 11, 2017
  12. MarcoFalke added the label Tests on Aug 11, 2017
  13. practicalswift force-pushed on Aug 11, 2017
  14. practicalswift commented at 5:21 pm on August 11, 2017: contributor
    Squashed into one commit. Please review :-)
  15. tests: Remove OldSetKeyFromPassphrase/OldEncrypt/OldDecrypt a897d0e37a
  16. practicalswift force-pushed on Aug 14, 2017
  17. practicalswift commented at 3:52 pm on August 14, 2017: contributor
    Rebased!
  18. theuni commented at 10:11 pm on August 16, 2017: member

    I have no issue with removing the old comparisons, as I think it’s safe to say by now that our implementation lines up well enough with the old openssl one.

    However, I don’t think it’d be a terrible idea to snapshot our current implementation and compare against that. That way future changes are checked against something.

    I suppose that’s really not necessary until those future changes exist, though.

    utACK.

  19. laanwj merged this on Aug 22, 2017
  20. laanwj closed this on Aug 22, 2017
  21. laanwj referenced this in commit 3e55f13bfc on Aug 22, 2017
  22. MarcoFalke commented at 2:25 pm on August 23, 2017: member
    post-merge utACK a897d0e37a02d29907c3a3f0f6536a26a155751d
  23. PastaPastaPasta referenced this in commit 38300a7a4b on Sep 19, 2019
  24. PastaPastaPasta referenced this in commit c2d1138472 on Sep 19, 2019
  25. PastaPastaPasta referenced this in commit cdca51c9cf on Feb 2, 2020
  26. UdjinM6 referenced this in commit 581626f23b on Feb 4, 2020
  27. zkbot referenced this in commit 8777d2884e on Sep 29, 2020
  28. zkbot referenced this in commit b5fa52b701 on Oct 1, 2020
  29. Fuzzbawls referenced this in commit 71ed429370 on Feb 14, 2021
  30. ckti referenced this in commit 185f3ae9c5 on Mar 28, 2021
  31. practicalswift deleted the branch on Apr 10, 2021
  32. gades referenced this in commit ce6eff4cc0 on Apr 21, 2022
  33. DrahtBot locked this on Aug 18, 2022


practicalswift laanwj gmaxwell sipa theuni MarcoFalke

Labels
Refactoring Tests

Milestone
0.16.0

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: 2024-07-01 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me