Serialization improvements: final step #19032

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.

Convert Qt to new serialization 65c589e45e

sipa requested review from ryanofsky on May 20, 2020

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

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

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.

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:

Timestamp of file with hash 3225b42876eec8e47ecaa04d904d9008dc762f25068d2120bfd47040bd83f701 -

</details>

sipa force-pushed on May 20, 2020

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);

DrahtBot added the label GUI on May 20, 2020

DrahtBot added the label Tests on May 20, 2020

DrahtBot added the label Wallet on May 20, 2020

sipa force-pushed on May 20, 2020

MarcoFalke removed the label GUI on May 20, 2020

MarcoFalke removed the label Tests on May 20, 2020

MarcoFalke removed the label Wallet on May 20, 2020

MarcoFalke added the label Refactoring on May 20, 2020

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.

sipa force-pushed on May 24, 2020

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:

Timestamp of file with hash f7b948dcd680ca3ad0d8ba39d18536d69a5d93ae4b678d5c0fb27d54153de077 -

</details>

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.

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-

sipa force-pushed on May 24, 2020

Convert wallet to new serialization ef17c03e07

Convert LimitedString to formatter 92beff15d3

Remove old serialization primitives 71f016c6eb

sipa force-pushed on May 24, 2020

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.

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>

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

Code review ACK 71f016c6eb42e1ac2c905e04ba4d20c2009e533f

laanwj merged this on May 26, 2020

laanwj closed this on May 26, 2020

sidhujag referenced this in commit 22576c8382 on May 27, 2020

MarcoFalke commented at 2:29 PM on June 8, 2020: member

re-ACK 71f016c 🤴

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

Signature:

Timestamp of file with hash 3646ddaf3ced2728b4c9185e8341178d02575e344a8f4292869f74ec2b3f5a97 -

</details>

Fabcien referenced this in commit 6b64f26d14 on Feb 22, 2021

deadalnix referenced this in commit 7f8be21094 on Feb 22, 2021

deadalnix referenced this in commit 632bf80813 on Feb 22, 2021

deadalnix referenced this in commit c044e9dcf1 on Feb 22, 2021

UdjinM6 referenced this in commit 9029448233 on May 28, 2021

furszy referenced this in commit 5c93f159bc on Jul 5, 2021

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

DrahtBot locked this on Feb 15, 2022