Full node changes required for CAT / Standalone #8616

Yostra · 2021-09-27T22:48:19Z

No description provided.

chia/consensus/blockchain.py

lgtm-com · 2021-09-27T23:05:44Z

This pull request introduces 3 alerts when merging b198f84 into de67c42 - view on LGTM.com

new alerts:

3 for Unused local variable

arvidn · 2021-09-27T23:08:06Z

chia/consensus/blockchain.py

@@ -164,7 +169,9 @@ async def receive_block(
        block: FullBlock,
        pre_validation_result: Optional[PreValidationResult] = None,
        fork_point_with_peak: Optional[uint32] = None,
-    ) -> Tuple[ReceiveBlockResult, Optional[Err], Optional[uint32], List[CoinRecord]]:
+    ) -> Tuple[
+        ReceiveBlockResult, Optional[Err], Optional[uint32], Tuple[List[CoinRecord], Dict[bytes, List[CoinRecord]]]


where is the call to this function where you do something with the additional return values?

receive_block_batch and respond_block in ful_node.py call this function and then pass the results to update_wallets

chia/consensus/blockchain.py

mariano54 · 2021-09-28T13:38:13Z

chia/consensus/blockchain.py

+                        lastest_coin_state[record.name] = record
+
+                    if npc_res is not None:
+                        hint_list = self.get_hint_list(npc_res)


Can you add a type annotation here? Not sure what type this list is.

chia/consensus/blockchain.py

chia/full_node/hint_store.py

chia/consensus/blockchain.py

arvidn

perhaps the tuple of coin updates and new hints should be put in a new class holding state updates/subscription updates. Such a class could also do the deduplication and aggregation that you're currently doing "ad hoc" (or, sprinkled into the existing code)

arvidn · 2021-09-29T10:37:32Z

chia/consensus/blockchain.py

+        ReceiveBlockResult,
+        Optional[Err],
+        Optional[uint32],
+        Tuple[List[CoinRecord], Dict[bytes, Dict[bytes32, CoinRecord]]],


this data structure is quite deep. I would think it warrants its own type to add some names and constraints to how it's used.
From the code below it seems like it's coin records that were added or removed, and then hints that were added (but not removed, right?). It doesn't seem necessary to bundle these up in a tuple.

I took a shortcut here, receive_block get's called on 100s of places in tests and if another arg was added in the call I would have to refactor all of those.

so maybe a dedicated type for updates would make sense then. then new updates could be added without affecting this signature

I'm not sure how this is passing the tests, given that the response is checked in a lot of tests.
I agree with arvid, this response is getting quite complex. Perhaps we can put all of these things into one struct (including the fork_height).

We can leave this for the future, but this is a pretty important method, so it would be good to keep it simple

arvidn · 2021-09-29T10:38:22Z

chia/consensus/blockchain.py

@@ -174,18 +183,13 @@ async def receive_block(
        """


it would probably make sense to extend this comment to describe what the last tuple in the return value is and what it can be expected to contain

chia/consensus/blockchain.py

arvidn · 2021-09-29T12:04:17Z

chia/full_node/hint_store.py

+        self.db_wrapper = db_wrapper
+        self.coin_record_db = db_wrapper.db
+        # the coin_id is unique, same hint can be used for multiple coins
+        await self.coin_record_db.execute(("CREATE TABLE IF NOT EXISTS hints(coin_id blob PRIMARY KEY,  hint blob)"))


if we ever want to support multiple hints per coin, changing this schema may be the biggest hurdle

good catch, I'll add another auto-increment id column there so that coin id is not unique

chia/full_node/hint_store.py

chia/protocols/protocol_message_types.py

chia/protocols/shared_protocol.py

tests/wallet/simple_sync/test_simple_sync_protocol.py

Co-authored-by: Arvid Norberg <arvid.norberg@gmail.com>

arvidn · 2021-09-29T21:30:10Z

I retriggered the failing test. Is there a risk this patch slowed down the execution to cause that timing constraint to be exceeded?
If so, we could bump it further under the understanding that we're increasing the tech debt

arvidn · 2021-09-29T21:31:15Z

Reviewing this, I haven't found any obvious errors, but I'm also not really convinced that all new edge cases introduced by this are covered by tests.

mariano54

LGTM after addressing arvid's feedback (and maybe adding a rate limit)

mariano54 · 2021-09-29T21:30:52Z

chia/consensus/blockchain.py

+        ReceiveBlockResult,
+        Optional[Err],
+        Optional[uint32],
+        Tuple[List[CoinRecord], Dict[bytes, Dict[bytes32, CoinRecord]]],


I'm not sure how this is passing the tests, given that the response is checked in a lot of tests.
I agree with arvid, this response is getting quite complex. Perhaps we can put all of these things into one struct (including the fork_height).

We can leave this for the future, but this is a pretty important method, so it would be good to keep it simple

mariano54 · 2021-09-29T21:34:20Z

chia/consensus/blockchain.py

                    if fetched_block_record.header_hash == block_record.header_hash:
-                        tx_removals, tx_additions = await self.get_tx_removals_and_additions(
+                        tx_removals, tx_additions, npc_res = await self.get_tx_removals_and_additions(
                            fetched_full_block, npc_result
                        )


We don't run it again. If the npc_result already exists (it's Optional, and not present when syncing) then we will not run the generator. So we are guaranteed to only run it once.

mariano54 · 2021-09-29T21:37:08Z

chia/full_node/full_node_api.py

@@ -1391,3 +1407,39 @@ async def request_children(self, request: wallet_protocol.RequestChildren) -> Op
        response = wallet_protocol.RespondChildren(states)
        msg = make_msg(ProtocolMessageTypes.respond_children, response)
        return msg
+
+    @api_request
+    async def request_ses_hashes(self, request: wallet_protocol.RequestSESInfo):


Blockchain.get_ses_heights() is very fast since it stores these things in memory (the height of each sub-epoch)

mariano54 · 2021-09-29T21:38:01Z

chia/full_node/full_node_api.py

@@ -1391,3 +1407,39 @@ async def request_children(self, request: wallet_protocol.RequestChildren) -> Op
        response = wallet_protocol.RespondChildren(states)
        msg = make_msg(ProtocolMessageTypes.respond_children, response)
        return msg
+
+    @api_request
+    async def request_ses_hashes(self, request: wallet_protocol.RequestSESInfo):


Everything here seems safe and very fast. However, we should have a rate limit like other messages.

* full node changes * hint store * finish coin state tracking, mypy, flake8 * type hints * bump protocol version * change wallet tool for testing hint list * mypy * add check for state, future proof hint db for multiple hints per coin * get hint rename * clean * Update chia/consensus/blockchain.py Co-authored-by: Arvid Norberg <arvid.norberg@gmail.com> Co-authored-by: Mariano Sorgente <3069354+mariano54@users.noreply.github.com> Co-authored-by: Arvid Norberg <arvid.norberg@gmail.com>

arvidn reviewed Sep 27, 2021

View reviewed changes

chia/consensus/blockchain.py Outdated Show resolved Hide resolved

arvidn reviewed Sep 27, 2021

View reviewed changes

chia/consensus/blockchain.py Outdated Show resolved Hide resolved

arvidn reviewed Sep 27, 2021

View reviewed changes

Yostra marked this pull request as ready for review September 28, 2021 02:32

mariano54 reviewed Sep 28, 2021

View reviewed changes

chia/consensus/blockchain.py Show resolved Hide resolved

mariano54 reviewed Sep 28, 2021

View reviewed changes

chia/full_node/hint_store.py Outdated Show resolved Hide resolved

mariano54 reviewed Sep 28, 2021

View reviewed changes

chia/consensus/blockchain.py Show resolved Hide resolved

Yostra added 6 commits September 28, 2021 20:03

full node changes

78ce604

hint store

9af8b7f

finish coin state tracking, mypy, flake8

4c70fe9

type hints

071ceb6

bump protocol version

490b71b

change wallet tool for testing hint list

54c0eba

Yostra force-pushed the standalon_validation_full_node_only branch from c9232ee to 54c0eba Compare September 29, 2021 00:47

mypy

898994c

arvidn reviewed Sep 29, 2021

View reviewed changes

Yostra and others added 4 commits September 29, 2021 14:16

add check for state, future proof hint db for multiple hints per coin

89cdda8

get hint rename

0bb2a5e

clean

2ee6c84

Update chia/consensus/blockchain.py

21c9429

Co-authored-by: Arvid Norberg <arvid.norberg@gmail.com>

mariano54 approved these changes Sep 29, 2021

View reviewed changes

wjblanke merged commit 4fddb5c into main Sep 29, 2021

wjblanke deleted the standalon_validation_full_node_only branch September 29, 2021 22:04

archiviz added a commit to Tranzact-Network/tranzact-blockchain that referenced this pull request Oct 6, 2021

Fix cherry_pick Chia-Network#8616

3828a47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full node changes required for CAT / Standalone #8616

Full node changes required for CAT / Standalone #8616

Yostra commented Sep 27, 2021

lgtm-com bot commented Sep 27, 2021

arvidn Sep 27, 2021

Yostra Sep 28, 2021

mariano54 Sep 28, 2021

arvidn left a comment

arvidn Sep 29, 2021

Yostra Sep 29, 2021

arvidn Sep 29, 2021

mariano54 Sep 29, 2021

arvidn Sep 29, 2021

arvidn Sep 29, 2021

Yostra Sep 29, 2021

Yostra Sep 29, 2021

arvidn commented Sep 29, 2021

arvidn commented Sep 29, 2021

mariano54 left a comment

mariano54 Sep 29, 2021

mariano54 Sep 29, 2021

mariano54 Sep 29, 2021

mariano54 Sep 29, 2021

Full node changes required for CAT / Standalone #8616

Full node changes required for CAT / Standalone #8616

Conversation

Yostra commented Sep 27, 2021

lgtm-com bot commented Sep 27, 2021

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

arvidn commented Sep 29, 2021

arvidn commented Sep 29, 2021

mariano54 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