Improve streamable #3031

richardkiss · 2021-04-28T18:18:45Z

No description provided.

chia/util/streamable.py

This no longer requires an extra `_parse_functions` attribute on a `Streamable`, as it may be confusing serializers or other functions that use `__annotations__`.

…time.

Revert an earlier commit and error out on class creation in case a Streamable subclass is instantiated incorrectly, e.g. containing a non-serializable member. Fix cases where Streamable parent class was forgotten.

aqk

Looks good. One thing I don't understand in the first question.

aqk · 2021-04-29T23:24:28Z

chia/util/streamable.py

+
+def parse_bool(f):
+    bool_byte = f.read(1)
+    assert bool_byte is not None and len(bool_byte) == 1  # Checks for EOF


Will these asserts activate in any run mode, or is it like C, where they can be deactivated in some run modes?

assert is removed in python -OO mode, which we don't do. So we shouldn't use assert for flow control. However, this problem was here before this change, so it doesn't make these semantics any worse.

These asserts are important. If we don't have them, people can Deserialize a bytes object which contains extra data.

In the bool example, read(1) which returns empty would also succeed, so we need to check that the exact size was read, any time we do a read.

aqk · 2021-04-29T23:26:03Z

chia/util/streamable.py

-            bytes_read = f.read(list_size)
-            assert bytes_read is not None and len(bytes_read) == list_size
-            return bytes_read
+            return parse_bytes


This section is nice.

aqk · 2021-04-29T23:31:09Z

chia/wallet/wallet_coin_store.py

+    def coin_record_from_row(self, row: sqlite3.Row) -> WalletCoinRecord:
+        coin = Coin(bytes32(bytes.fromhex(row[6])), bytes32(bytes.fromhex(row[5])), uint64.from_bytes(row[7]))
+        return WalletCoinRecord(
+            coin, uint32(row[1]), uint32(row[2]), bool(row[3]), bool(row[4]), WalletType(row[8]), row[9]


What is the type of row[9] - I notice it is the only one not explicitly cast

a comment with the schema of this table would be very helpful, but I suppose this is not quite in scope for this PR

aqk · 2021-04-29T23:32:17Z

chia/wallet/wallet_puzzle_store.py

                G1Element.from_bytes(bytes.fromhex(row[1])),
-                row[3],
-                row[4],
+                WalletType(row[3]),


WalletType: nice.

Was this broken before?

aqk · 2021-04-29T23:37:02Z

chia/util/streamable.py

+        fields = {}
+
+    for _, f_type in fields.items():
+        parse_functions.append(cls.function_to_parse_one_item(f_type))


Is the intent here to automatically add parse functions for only the primitive types that are contained as members?

The general idea is to traverse the __annotations__ tree just once at class instantiation time, and put into a list the sub-parsing functions which do parsing and not introspection.

arvidn

I think this approach is a lot nicer than the previous one. It especially makes it easy to test the parser for each primitive type, and really cover all the edge cases. I really think we should do that. This code has better work, everything else relies on it.

I would also have expected to see tests where we ensure that some (or maybe all) network messages encode to specific bytes. Just passing a round-trip test isn't enough to ensure we don't break the protocol.

I'll take a look at adding some tests today.

arvidn · 2021-04-30T08:38:43Z

chia/protocols/timelord_protocol.py

@@ -74,7 +74,7 @@ class NewEndOfSubSlotVDF(Streamable):

 @dataclass(frozen=True)
 @streamable
-class RequestCompactProofOfTime:
+class RequestCompactProofOfTime(Streamable):


I would expect the annotation could add the base class too, no? (maybe for later)

The annotation could do the dataclass wrapping as well, but the comment in def streamable states that this is intended for linters so they could recognize the methods of the base class as well as the __init__ method provided by dataclass.

arvidn · 2021-04-30T08:42:59Z

chia/wallet/wallet_coin_store.py

+    def coin_record_from_row(self, row: sqlite3.Row) -> WalletCoinRecord:
+        coin = Coin(bytes32(bytes.fromhex(row[6])), bytes32(bytes.fromhex(row[5])), uint64.from_bytes(row[7]))
+        return WalletCoinRecord(
+            coin, uint32(row[1]), uint32(row[2]), bool(row[3]), bool(row[4]), WalletType(row[8]), row[9]


a comment with the schema of this table would be very helpful, but I suppose this is not quite in scope for this PR

arvidn · 2021-04-30T08:48:09Z

chia/wallet/wallet_puzzle_store.py

-                row[0],
-                bytes.fromhex(row[2]),
+                uint32(row[0]),
+                bytes32.fromhex(row[2]),


we really store binary data in hex form in the database?

Yes, we currently do. See add_derivation_paths() in the same file.

arvidn · 2021-04-30T08:53:39Z

chia/util/streamable.py

-            return bytes.decode(str_read_bytes, "utf-8")
-        raise RuntimeError(f"Type {f_type} does not have parse")
+            return parse_str
+        raise NotImplementedError(f"Type {f_type} does not have parse")


this message doesn't look right to me. Shouldn't it say "Type {f_type} does not have from_bytes()"?

I see, parse() is the main way of supporting the streamable protocol

Currently from_bytes() is used only for sized integer types. All other types that have custom serialization need to provide their own .parse() function.

rostislav · 2021-04-30T09:46:25Z

I performed the following benchmark on old and new Streamable to compare their performance: parsing the first 10000 mainnet blocks. The old result: 1.39s; the new result: 1.08s. The speed improvement is 23%.

mariano54 · 2021-04-30T06:07:21Z

chia/protocols/full_node_protocol.py

@@ -76,6 +76,7 @@ class RejectBlock(Streamable):


 @dataclass(frozen=True)
+@streamable
 class RequestBlocks(Streamable):


How did this work before without streamable decorator?

mariano54 · 2021-04-30T11:09:33Z

chia/util/streamable.py

+
+def parse_bool(f):
+    bool_byte = f.read(1)
+    assert bool_byte is not None and len(bool_byte) == 1  # Checks for EOF


These asserts are important. If we don't have them, people can Deserialize a bytes object which contains extra data.

mariano54 · 2021-04-30T11:10:44Z

chia/util/streamable.py

+
+def parse_bool(f):
+    bool_byte = f.read(1)
+    assert bool_byte is not None and len(bool_byte) == 1  # Checks for EOF


In the bool example, read(1) which returns empty would also succeed, so we need to check that the exact size was read, any time we do a read.

mariano54 · 2021-04-30T11:18:11Z

chia/util/streamable.py

-            fields = {}
-        for _, f_type in fields.items():
-            values.append(cls.parse_one_item(f_type, f))  # type: ignore
+        values = [parse_f(f) for parse_f in PARSE_FUNCTIONS_FOR_STREAMABLE_CLASS[cls]]


I don't remember why I had a try clause here, maybe it was to support Empty streamables? (for example RequestPeers) message. Have we confirmed that those still work?

mariano54 · 2021-04-30T11:20:14Z

chia/wallet/wallet_puzzle_store.py

                G1Element.from_bytes(bytes.fromhex(row[1])),
-                row[3],
-                row[4],
+                WalletType(row[3]),


Was this broken before?

mariano54 · 2021-04-30T11:21:50Z

Nice work. If nothing is broken with the wallet, I think we can probable merge it (and if arvids tests pass)

mariano54

I've been running the spend bot with it, no issues

* Added new issue templates (#3033) * added new issue templates for support and requestes * spelling is hard... * Add lock, keep cache consistent (#3051) * execute task decorator * use blockchain lock * indentation * lint * execute_task * Ms.empty blocks3 (#3064) * Fix empty blocks * Remove error log * Missing imports * .header_hash instead of .prev_hash in simulator * Fix typo (#3069) * Use secure coin ids (#3095) * Add announcements for standard tx (#3023) * changed bahviour of wallet and wallet tools for standard solutions * black formatting wallet_tools * added a test for hypothetical stolen zero_output coin * swap to sha256 from sha256tree for message * fix wallet_tools version, address lint issues * correctly int_from_bytes * fix uninstantiated key in dict * Comment out broken test * Fix types (used SerializedProgram) Co-authored-by: Mariano <sorgente711@gmail.com> * cache VDF validation results (per process) (#3110) * Update GUI for plot refresh issue (#3135) Co-authored-by: Adam Kelly <aqk@aqk.im> * remove commit and not needed query (#3148) * Copyright 2021 Chia Network in LICENSE (#3153) * just filter limit (#3152) * fix plot dupe (#3154) * increase ratio (#3155) * wait to end of process to avoid zombies (#3160) * fix takes 1 positional argument but 2 were given (#3202) * Update CLI tools to use EiB when appropriate. (#3117) * Improve streamable (#3031) * Avoid importing `test_constants` as it takes a long time. * Factor out `parse_*` functions. * First crack at refactoring `Streamable.parse`. * Don't add `_parse_functions` attribute to `Streamable`. This no longer requires an extra `_parse_functions` attribute on a `Streamable`, as it may be confusing serializers or other functions that use `__annotations__`. * Fix lint problems with `black`. * Fix `parse_tuple`. * Defer some parsing failures to parse time rather than class-creation time. * Tidy up & remove some obsolete stuff. * Decorate `RequestBlocks` as `streamable`. * Fix wrong uses of Streamable class Revert an earlier commit and error out on class creation in case a Streamable subclass is instantiated incorrectly, e.g. containing a non-serializable member. Fix cases where Streamable parent class was forgotten. * Fix wrong types when creating DerivationRecord and WalletCoinRecord * additional unit tests for streamable parsers * add type annotations (#3222) Co-authored-by: Rostislav <rostislav@users.noreply.github.com> Co-authored-by: arvidn <arvid@libtorrent.org> * Tests skipping mempool (#3065) * Avoid importing `test_constants` as it takes a long time. * Some new tests that circumvents mempool. * Fix lint problems. Co-authored-by: J Eckert <sargonas@users.noreply.github.com> Co-authored-by: Yostra <straya@chia.net> Co-authored-by: Mariano Sorgente <3069354+mariano54@users.noreply.github.com> Co-authored-by: Antonio Borrero Granell <me@antoniobg.com> Co-authored-by: matt-o-how <48453825+matt-o-how@users.noreply.github.com> Co-authored-by: Mariano <sorgente711@gmail.com> Co-authored-by: Arvid Norberg <arvid@libtorrent.org> Co-authored-by: Adam Kelly <338792+aqk@users.noreply.github.com> Co-authored-by: Adam Kelly <aqk@aqk.im> Co-authored-by: Kyle Altendorf <sda@fstab.net> Co-authored-by: Joel <34619326+joelcho@users.noreply.github.com> Co-authored-by: Gene Hoffman <30377676+hoffmang9@users.noreply.github.com> Co-authored-by: Richard Kiss <him@richardkiss.com> Co-authored-by: Rostislav <rostislav@users.noreply.github.com>

richardkiss marked this pull request as draft April 28, 2021 18:18

mariano54 reviewed Apr 29, 2021

View reviewed changes

chia/util/streamable.py Outdated Show resolved Hide resolved

richardkiss added 9 commits April 29, 2021 16:47

Avoid importing test_constants as it takes a long time.

7f4bd66

Factor out parse_* functions.

0b0217b

First crack at refactoring Streamable.parse.

a3a5dda

Don't add _parse_functions attribute to Streamable.

97e6d03

This no longer requires an extra `_parse_functions` attribute on a `Streamable`, as it may be confusing serializers or other functions that use `__annotations__`.

Fix lint problems with black.

31d18b6

Fix parse_tuple.

5d97454

Defer some parsing failures to parse time rather than class-creation …

f9e350f

…time.

Tidy up & remove some obsolete stuff.

fc5e19e

Decorate RequestBlocks as streamable.

68aedfc

rostislav force-pushed the improve_streamable branch 2 times, most recently from 8ea1b38 to fc159fe Compare April 29, 2021 16:15

Fix wrong uses of Streamable class

e350aae

Revert an earlier commit and error out on class creation in case a Streamable subclass is instantiated incorrectly, e.g. containing a non-serializable member. Fix cases where Streamable parent class was forgotten.

rostislav force-pushed the improve_streamable branch from fc159fe to e350aae Compare April 29, 2021 16:57

Fix wrong types when creating DerivationRecord and WalletCoinRecord

6fec83c

rostislav marked this pull request as ready for review April 29, 2021 22:58

aqk previously approved these changes Apr 29, 2021

View reviewed changes

arvidn reviewed Apr 30, 2021

View reviewed changes

mariano54 reviewed Apr 30, 2021

View reviewed changes

additional unit tests for streamable parsers

a7d64b9

arvidn dismissed aqk’s stale review via a7d64b9 April 30, 2021 11:57

add type annotations (#3222)

f7ea098

mariano54 approved these changes Apr 30, 2021

View reviewed changes

wjblanke merged commit b084813 into main Apr 30, 2021

wjblanke deleted the improve_streamable branch April 30, 2021 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve streamable #3031

Improve streamable #3031

richardkiss commented Apr 28, 2021

aqk left a comment

aqk Apr 29, 2021

richardkiss Apr 30, 2021

mariano54 Apr 30, 2021

mariano54 Apr 30, 2021

aqk Apr 29, 2021

aqk Apr 29, 2021

arvidn Apr 30, 2021

aqk Apr 29, 2021

mariano54 Apr 30, 2021

aqk Apr 29, 2021

richardkiss Apr 30, 2021

arvidn left a comment

arvidn Apr 30, 2021

rostislav Apr 30, 2021

arvidn Apr 30, 2021

arvidn Apr 30, 2021

rostislav Apr 30, 2021

arvidn Apr 30, 2021

arvidn Apr 30, 2021

rostislav Apr 30, 2021

rostislav commented Apr 30, 2021

mariano54 Apr 30, 2021

mariano54 Apr 30, 2021

mariano54 Apr 30, 2021

mariano54 Apr 30, 2021

mariano54 Apr 30, 2021

mariano54 commented Apr 30, 2021

mariano54 left a comment

Improve streamable #3031

Improve streamable #3031

Conversation

richardkiss commented Apr 28, 2021

aqk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvidn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rostislav commented Apr 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariano54 commented Apr 30, 2021

mariano54 left a comment

Choose a reason for hiding this comment