Serialization improvements: final step #19032

pull sipa wants to merge 4 commits into bitcoin:master from sipa:202005_noncastserial_final changing 8 files +87 −164
  1. sipa commented at 6:16 PM on May 20, 2020: member

    This is the final step 🥳 of the serialization improvements extracted from #10785.

    It converts the LimitedString wrapper to a new-style formatter, and updates the wallet and Qt code to use the new serialization framework. Finally all remaining old primitives are removed.

  2. Convert Qt to new serialization 65c589e45e
  3. sipa requested review from ryanofsky on May 20, 2020
  4. in src/wallet/scriptpubkeyman.h:141 in 3fc67497b8 outdated
     164 | -            READWRITE(fInternal);
     165 | -            READWRITE(m_pre_split);
     166 | +        try {
     167 | +            s >> m_pre_split;
     168 | +        } catch (std::ios_base::failure&) {
     169 | +            /* flag as external address if we can't read the internal boolean
    


    MarcoFalke commented at 6:49 PM on May 20, 2020:

    in commit 3fc67497b85b1f465beb5c3d52303a7828c8cacc:

    Why change the documentation?

                /* flag as external address if we can't read the m_pre_split boolean
    
  5. in src/wallet/scriptpubkeyman.h:142 in 3fc67497b8 outdated
     165 | -            READWRITE(m_pre_split);
     166 | +        try {
     167 | +            s >> m_pre_split;
     168 | +        } catch (std::ios_base::failure&) {
     169 | +            /* flag as external address if we can't read the internal boolean
     170 | +               (this will be the case for any wallet before the HD chain split version) */
    


    MarcoFalke commented at 6:49 PM on May 20, 2020:
                   (this will be the case for any wallet that upgrades to HD chain split)*/
    

    Same

  6. in src/wallet/wallet.h:250 in 3fc67497b8 outdated
     246 | @@ -247,15 +247,13 @@ struct COutputEntry
     247 |  class CMerkleTx
     248 |  {
     249 |  public:
     250 | -    template<typename Stream>
     251 | -    void Unserialize(Stream& s)
     252 | +    SERIALIZE_METHODS(CMerkleTx, obj)
    


    MarcoFalke commented at 6:51 PM on May 20, 2020:

    in commit 3fc67497b85b1f465beb5c3d52303a7828c8cacc:

    Looks like the change in this file isn't needed? I don't mind either way, just noting.


    sipa commented at 11:09 PM on May 21, 2020:

    You're right. I think this code was originally written before the Serialize method was removed here, and through rebases now reintroduces it. I'm happy to remove it, but perhaps it's desirable for consistency reasons.


    MarcoFalke commented at 11:38 PM on May 21, 2020:

    The serialization code has been removed in 05b56d1c937b7667ad51400d2f9fb674af72953f, just for reference.


    sipa commented at 7:00 AM on May 24, 2020:

    Ok, removed here. No need to effectively revert that change.

  7. MarcoFalke commented at 7:04 PM on May 20, 2020: member

    ACK f57088302fa3f2bf1d84c7d90abfaeaeb2a25684 code looks correct, but documentation got corrupted for some reason 🚖

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    ACK f57088302fa3f2bf1d84c7d90abfaeaeb2a25684 code looks correct, but documentation got corrupted for some reason 🚖
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUg98gwAoENgW+nEfnRVpU+BYSwAmkWsGXmnb3mhgzZ5iaW6aADpwJ6Ja9na+N6C
    xXR/Ehg7uz3Eorj7NzJSBUWBgH/Tw0lCI6v0mXHXUnbcsNS0Xaj1UGBMtI2qFYyX
    PKuM9/T8SsbxpnE4QUPXoEytiS0u1sdRkGqJ7/N3TLkhg3a0ZcSeF+GEH2SV2xxB
    L3njvthwIv4YAn5iCzGNyRV1/DVL4bUdk7JTjiaZUJj1GEr7BI/j+tryWahkuB54
    yIGL8rQojO8vWOwVUj7AQuR7fEgpoD8LAtdsD8Oxq581+sIH3ahyl8ZeqxYDk2X/
    4aej8GK+RQ1Tqb3Gc9l9/y7/XL42DoCmGMSuW6IFD17XQDTv2UV95+DlR9p4NV1w
    hHX0cq0JsXaJNBQzR2CMAx+fIKs5BevHtEODENnOd0Co52XZYVmwZfUkpCOthDKN
    94hAA2tQ/gP3Fo+60ub3ZAdZ2CrkSKXmL/R0Ro6ykRsLeRrN70ZpzA5yokp7vaQG
    jMLT8uSs
    =kX23
    -----END PGP SIGNATURE-----
    

    Timestamp of file with hash 3225b42876eec8e47ecaa04d904d9008dc762f25068d2120bfd47040bd83f701 -

    </details>

  8. sipa force-pushed on May 20, 2020
  9. MarcoFalke commented at 7:15 PM on May 20, 2020: member

    The fuzz tests are complaining:

    test/fuzz/string.cpp:111:15: error: no template named 'LimitedString'
    
            const LimitedString<10> limited_string = LIMITED_STRING(random_string_1, 10);
    
  10. DrahtBot added the label GUI on May 20, 2020
  11. DrahtBot added the label Tests on May 20, 2020
  12. DrahtBot added the label Wallet on May 20, 2020
  13. sipa force-pushed on May 20, 2020
  14. sipa force-pushed on May 20, 2020
  15. MarcoFalke removed the label GUI on May 20, 2020
  16. MarcoFalke removed the label Tests on May 20, 2020
  17. MarcoFalke removed the label Wallet on May 20, 2020
  18. MarcoFalke added the label Refactoring on May 20, 2020
  19. practicalswift commented at 7:14 AM on May 21, 2020: contributor

    Concept ACK

    The new serialization code is clearer and thus much easier to reason about.

  20. sipa force-pushed on May 24, 2020
  21. MarcoFalke commented at 12:42 PM on May 24, 2020: member

    re-ACK 81caf8a869 only changes since previous review 🏺

    • Change author email address in one commit
    • Restore uncorrupted documentation for m_pre_split
    • Remove one overly eager hunk in CMerkleTx to minimize the diff
    • Make the fuzz tests compile again

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    re-ACK 81caf8a869 only changes since previous review 🏺
    
    * Change author email address in one commit
    * Restore uncorrupted documentation for m_pre_split
    * Remove one overly eager hunk in CMerkleTx to minimize the diff
    * Make the fuzz tests compile again
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUiyqQv/WKvoztimli1L/+qEb2zHJmcLmfgAQoCzC3GOh9Hi2Px9fK4Zsp691OsA
    2T937ZSmwdWHpbVCzW+mDAQnGX9qN6h6ysISSsu3jUOWtBwCkHHUmAiZIItrDrsQ
    Al44fOPEDW84S1RMm1cL71Sn+rt4qWQCAJ028cx37Wpp2zhGCBjBvZx+IY1BKJ4o
    8L1TdkhoRSpDz6KFS5vpFGzePxwBl3LVFP/Q5Fx8Ijz0qFXn7Ewg8NtMXtuW8D0I
    V32JuWT5qo4qFO0/lBdNrb8icN7tpfkH/VVZ9nKMp3RXiTdsGvJeC4iwLiEzOPJU
    opQNWpkpJ79we9lS9Hw35BLMgDz9jRpcckImYceM46HrDrDG7kyKV+iCDsn4Edyj
    U5JY835uzcKDXxWXyW6GijCX6why4qKwFUNdmB3HIpa269IaOy9cjaM4ZpGzzK1R
    BEVlwsY9eo8YyOssR3gscZoBHMsJrTqId5oU6q9PodfewntD0O4h579bD1181iZ+
    Tkr4vdOA
    =IsD3
    -----END PGP SIGNATURE-----
    

    Timestamp of file with hash f7b948dcd680ca3ad0d8ba39d18536d69a5d93ae4b678d5c0fb27d54153de077 -

    </details>

  22. in src/wallet/scriptpubkeyman.h:141 in 81caf8a869 outdated
     164 | -            READWRITE(fInternal);
     165 | -            READWRITE(m_pre_split);
     166 | +        try {
     167 | +            s >> m_pre_split;
     168 | +        } catch (std::ios_base::failure&) {
     169 | +            /* flag as external address if we can't read the m_pre_split boolean
    


    MarcoFalke commented at 12:45 PM on May 24, 2020:
                /* flag as postsplit address if we can't read the m_pre_split boolean
    

    I think this is still wrong


    jonatack commented at 2:16 PM on May 24, 2020:

    309a653 s/external/postsplit/?

    -            /* flag as external address if we can't read the m_pre_split boolean
    +            /* flag as postsplit address if we can't read the m_pre_split boolean
    

    sipa commented at 5:38 PM on May 24, 2020:

    Fixed.


    sipa commented at 5:38 PM on May 24, 2020:

    Fixed.

  23. jonatack commented at 2:24 PM on May 24, 2020: member

    ACK 81caf8a869 modulo doc nit below. Review, build/tests, ran bitcoind/bitcoin-qt, fuzz build/ran string fuzzer.

    $ src/test/fuzz/string ../qa-assets/fuzz_seed_corpus/
    INFO: Seed: 1884355399
    INFO: Loaded 1 modules   (499191 inline 8-bit counters): 499191 [0x562be9805588, 0x562be987f37f), 
    INFO: Loaded 1 PC tables (499191 PCs): 499191 [0x562be987f380,0x562bea01d2f0), 
    INFO:    52918 files found in ../qa-assets/fuzz_seed_corpus/
    INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
    INFO: seed corpus: files: 52918 min: 1b max: 3984182b total: 495166672b rss: 163Mb
    [#4096](/bitcoin-bitcoin/4096/)	pulse  cov: 5223 ft: 12262 corp: 216/949b exec/s: 2048 rss: 255Mb
    [#8192](/bitcoin-bitcoin/8192/)	pulse  cov: 5391 ft: 13542 corp: 299/1855b exec/s: 2048 rss: 342Mb
    
    .../...
    
    [#376078](/bitcoin-bitcoin/376078/)	REDUCE cov: 5594 ft: 17542 corp: 576/19Kb exec/s: 1047 rss: 1123Mb L: 31/412 MS: 1 EraseBytes-
    [#382183](/bitcoin-bitcoin/382183/)	REDUCE cov: 5594 ft: 17542 corp: 576/19Kb exec/s: 1047 rss: 1123Mb L: 7/412 MS: 1 EraseBytes-
    
  24. sipa force-pushed on May 24, 2020
  25. sipa force-pushed on May 24, 2020
  26. sipa force-pushed on May 24, 2020
  27. Convert wallet to new serialization ef17c03e07
  28. Convert LimitedString to formatter 92beff15d3
  29. Remove old serialization primitives 71f016c6eb
  30. sipa force-pushed on May 24, 2020
  31. in src/wallet/scriptpubkeyman.h:126 in 71f016c6eb
     124 | -    template <typename Stream, typename Operation>
     125 | -    inline void SerializationOp(Stream& s, Operation ser_action) {
     126 | +    template<typename Stream>
     127 | +    void Unserialize(Stream& s)
     128 | +    {
     129 |          int nVersion = s.GetVersion();
    


    jonatack commented at 9:26 AM on May 25, 2020:

    ef17c03 is this new change from 309a653 intentional (needed for deserializing)?

    -        int nVersion;
    +        int nVersion = s.GetVersion();
    

    sipa commented at 3:28 PM on May 25, 2020:

    I doubt it's needed, as I don't think this class is ever deserialized with SER_GETHASH. However, if it were ever invoked that way, the old code was UB (nVersion would be used without being initialized).


    jonatack commented at 3:52 PM on May 25, 2020:

    Thank you. Good catch and reminder.

  32. jonatack commented at 10:05 AM on May 25, 2020: member

    ACK 71f016c6eb42e1ac2 reviewed diff, builds/tests/re-fuzzed.

    <details><summary>changes since 81caf8a: avoiding use before initialization of <code>nVersion</code> in <code>ScriptPubKeyMan::Unserialize()</code> and documentation fixup</summary><p>

    
    diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
    --- a/src/wallet/scriptpubkeyman.h
    +++ b/src/wallet/scriptpubkeyman.h
    
    @@ -123,7 +123,7 @@ public:
         void Unserialize(Stream& s)
         {
    -        int nVersion;
    +        int nVersion = s.GetVersion();
             if (!(s.GetType() & SER_GETHASH)) {
                 s >> nVersion;
             }
    
    @@ -138,8 +138,8 @@ public:
             } catch (std::ios_base::failure&) {
    -            /* flag as external address if we can't read the m_pre_split boolean
    -               (this will be the case for any wallet that upgrades to HD chain split)*/
    +            /* flag as postsplit address if we can't read the m_pre_split boolean
    +               (this will be the case for any wallet that upgrades to HD chain split) */
                 m_pre_split = false;
    

    </p></details>

  33. laanwj commented at 1:44 PM on May 26, 2020: member

    Code review ACK 71f016c6eb42e1ac2c905e04ba4d20c2009e533f

  34. laanwj merged this on May 26, 2020
  35. laanwj closed this on May 26, 2020

  36. sidhujag referenced this in commit 22576c8382 on May 27, 2020
  37. MarcoFalke commented at 2:29 PM on June 8, 2020: member

    re-ACK 71f016c 🤴

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    re-ACK 71f016c 🤴
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUiCcgv9FZxj/YquESruZvS2FcLiLs55nDP0DClnbFBcYts6JsuqkeMwNIiBY1Oa
    A8Qx6zv8qN1PdFVXsOhzoA86abTQMGxzJkBHRqC73eDtB9n9DsSZygz+bWP+aLKN
    cC7r5YRwdBmnIv/47+rELLK8DD31geVNyISZUvsPk/JfJAwJhf2uSi1MEAI2qjds
    FV890WMLshTbj86n2cMCLWlN6LXL9Okkiw4SKs/qo9OzLvCXQwJzycL4grs9lHTj
    M3MzZJ6W66jp1BZe9/ob3LeHGwd4qL+5gvaO3FM1y7LoxNCZAnZk7kC83nCv4TOC
    MLX2aruo2I1KpumZWoICSkqqp1MXMG6qPnfu7RpntXu/0ky6bB40eyg8q6HaWQPT
    jIO7F8DkEoow0jC9XNQmG4DJYNq3FhAkDAeKCxnnUsR9zSLEa2HyZ3N/VHp3THsK
    hBNzdzfAQ0sAN88Gz1WFL/ky1fy4v2ynBz5hixctLGvvU7EkmYdC4QJnRs6yErvu
    1X0GS7hd
    =cvua
    -----END PGP SIGNATURE-----
    

    Timestamp of file with hash 3646ddaf3ced2728b4c9185e8341178d02575e344a8f4292869f74ec2b3f5a97 -

    </details>

  38. Fabcien referenced this in commit 6b64f26d14 on Feb 22, 2021
  39. deadalnix referenced this in commit 7f8be21094 on Feb 22, 2021
  40. deadalnix referenced this in commit 632bf80813 on Feb 22, 2021
  41. deadalnix referenced this in commit c044e9dcf1 on Feb 22, 2021
  42. UdjinM6 referenced this in commit 9029448233 on May 28, 2021
  43. furszy referenced this in commit 5c93f159bc on Jul 5, 2021
  44. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  45. DrahtBot locked this on Feb 15, 2022

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-19 09:14 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me