Ms.wallet refactor #10146

mariano54 · 2022-02-08T17:02:33Z

This is a significant change in how syncing works in the wallet. It is now organized as a queue where one of four things can get processed at once:

New puzzle hash subscription
New coin ID subscription
new_peak_wallet message
state_update message

The subscriptions are prioritized to ensure we don't miss data. State updates are cached by header hash (not height) and reorgs are now processed correctly. Many different bugs and race conditions are fixed by this change. Also, there is a performance improvement in untrusted sync.

A full node bug in coin store is also fixed (affected wallet protocol), where the min height was not being looked up correctly.

TODO:

fix flaky tests
Test on larger wallets, and test switching between trusted/untrusted
Fix oom issue (also present in main)
Potentially remove unnecessary fork_point argument

# Conflicts: # chia/wallet/wallet_node.py # chia/wallet/wallet_state_manager.py

lgtm-com · 2022-02-09T16:06:59Z

This pull request introduces 1 alert when merging 9adfe90 into 3b16013 - view on LGTM.com

new alerts:

1 for Clear-text logging of sensitive information

Nikolaj-K · 2022-02-09T16:23:57Z

chia/wallet/util/filter_coin_states.py

+    final_coin_state: List[CoinState] = []
+    for coin_state_entry in all_coins_state:
+        if coin_state_entry.spent_height is not None:
+            if coin_state_entry.spent_height <= fork_height:
+                final_coin_state.append(coin_state_entry)
+        elif coin_state_entry.created_height is not None:
+            if coin_state_entry.created_height <= fork_height:
+                final_coin_state.append(coin_state_entry)
+    return final_coin_state


If the function is called filter_foo, why not use filter :^)
I think this is

def accept(coin_state): sh = coin_state.spent_height if sh is not None: return sh <= fork_height ch = coin_state.created_height return ch is not None and ch <= fork_height return list(filter(accept, all_coins_state))

(And I said this before, but I think half of the list casts in the code base are unnecessary.)

lgtm-com · 2022-02-09T16:52:43Z

This pull request introduces 1 alert when merging 9091b7d into 3b16013 - view on LGTM.com

new alerts:

1 for Clear-text logging of sensitive information

lgtm-com · 2022-02-10T04:15:25Z

This pull request introduces 1 alert when merging a0e7346 into 419f88f - view on LGTM.com

new alerts:

1 for Clear-text logging of sensitive information

lgtm-com · 2022-02-10T16:53:21Z

This pull request introduces 2 alerts when merging de68b58 into 419f88f - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'
1 for Clear-text logging of sensitive information

lgtm-com · 2022-02-10T18:21:01Z

This pull request introduces 2 alerts when merging 532c6c5 into 419f88f - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'
1 for Clear-text logging of sensitive information

lgtm-com · 2022-02-10T19:40:22Z

This pull request introduces 2 alerts when merging cecd4fa into 419f88f - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'
1 for Clear-text logging of sensitive information

lgtm-com · 2022-02-10T20:44:09Z

This pull request introduces 2 alerts when merging fa140c7 into 419f88f - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'
1 for Clear-text logging of sensitive information

lgtm-com · 2022-02-11T00:18:51Z

This pull request introduces 2 alerts when merging 4fa85c9 into 231ef6f - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'
1 for Clear-text logging of sensitive information

arvidn · 2022-02-11T01:45:13Z

chia/full_node/coin_store.py

@@ -364,8 +364,7 @@ async def get_coin_state_by_ids(
        self,
        include_spent_coins: bool,
        coin_ids: List[bytes32],
-        start_height: uint32 = uint32(0),
-        end_height: uint32 = uint32((2 ** 32) - 1),
+        min_height: uint32 = uint32(0),


I notice there's no unit test for this function, in tests/core/full_node/test_coin_store.py. It would be good to have one

Thank you, yes that would be good considering it's in the coin store.

lgtm-com · 2022-02-11T16:57:15Z

This pull request introduces 2 alerts when merging 7a612e3 into 231ef6f - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'
1 for Clear-text logging of sensitive information

lgtm-com · 2022-02-11T19:13:57Z

This pull request introduces 2 alerts when merging 2ee4143 into 231ef6f - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'
1 for Clear-text logging of sensitive information

lgtm-com · 2022-02-12T16:53:05Z

This pull request introduces 2 alerts when merging 0af30de into e479586 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'
1 for Clear-text logging of sensitive information

mariano54 · 2022-02-14T14:58:39Z

Waiting for merge @Yostra @wjblanke

Yostra · 2022-02-14T17:54:23Z

chia/full_node/full_node.py

                peak = self.blockchain.get_peak()
                if len(coin_states) > 0 and fork_height is not None:
                    await self.update_wallets(peak.height, fork_height, peak.header_hash, coin_states)
+                await self.send_peak_to_wallets()


Does this change the order of state update <-> new peak messages ?

yeah but only in the case where the full node is syncing up, which is not the usual case. It was wrong before, it should be first the new state then the new_peak_wallet

Yostra · 2022-02-14T18:17:42Z

tests/core/server/test_rate_limits.py

@@ -23,11 +23,11 @@ async def test_too_many_messages(self):
        # Too many messages
        r = RateLimiter(incoming=True)
        new_tx_message = make_msg(ProtocolMessageTypes.new_transaction, bytes([1] * 40))
-        for i in range(3000):
+        for i in range(4900):


Is this related to the rest of the changes in some way?

No it is not, it was just a flaky test that needed a fix.

mariano54 added 12 commits February 3, 2022 15:14

wallet fixes

188a314

Don't show false positive synched

8b8b37e

Code cleanup and lint

7882c6d

Fixes

7df906b

Revert issue

60534bd

Fix reorg issue

19b5e22

WIP wallet node

a42cf73

More wallet refactor

bb9ee13

More wallet changes

54dbec7

More

5dcdafa

Fix subscription bugs

c4913a9

Fix some tests

831c32c

mariano54 marked this pull request as draft February 8, 2022 17:02

mariano54 added 10 commits February 8, 2022 15:53

Fix pool tests

c908306

More tweaks

f07648d

Lint and small issues

a81e58e

call update_ui at the correct points

25f461b

Small changes

c36f434

new peak queue

599ce94

Fix peer height issue

ad9be44

Rollback more for safety, and tweak logging

dd935fe

Small WSM style fixes

5820a1d

Merge branch 'main' into ms.wallet_refactor

9adfe90

# Conflicts: # chia/wallet/wallet_node.py # chia/wallet/wallet_state_manager.py

Nikolaj-K reviewed Feb 9, 2022

View reviewed changes

Change fork point in long_sync

9091b7d

mariano54 requested a review from Yostra February 9, 2022 18:17

More fixes with real world testing

a0e7346

Lint

de68b58

mariano54 added 2 commits February 10, 2022 12:34

Merge branch 'main' into ms.wallet_refactor

efc8a31

Small fix

532c6c5

mariano54 added 2 commits February 10, 2022 14:14

Fix CAT issue

95873b9

Fix test trades race condition

cecd4fa

mariano54 marked this pull request as ready for review February 10, 2022 19:25

mariano54 marked this pull request as draft February 10, 2022 19:31

Fix test trades race condition

fa140c7

mariano54 marked this pull request as ready for review February 10, 2022 20:30

mariano54 changed the title ~~Ms.wallet refactor (WIP)~~ Ms.wallet refactor Feb 10, 2022

Try to reduce flakiness

4fa85c9

arvidn reviewed Feb 11, 2022

View reviewed changes

Test coin store and fix additional method

7a612e3

esaung linked an issue Feb 11, 2022 that may be closed by this pull request

[enhancement] improve and optimize light wallet sync performance #10201

Closed

Improve flakiness

2ee4143

Merge branch 'main' into ms.wallet_refactor

0af30de

Yostra approved these changes Feb 14, 2022

View reviewed changes

wjblanke merged commit 7fa1861 into main Feb 14, 2022

wjblanke deleted the ms.wallet_refactor branch February 14, 2022 19:28

xdustinface mentioned this pull request Mar 22, 2023

wallet: Cleanup get_timestamp_for_height + Drop height_to_time #14885

Merged

arvidn mentioned this pull request Feb 15, 2024

optimize validate_removals() #17560

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ms.wallet refactor #10146

Ms.wallet refactor #10146

mariano54 commented Feb 8, 2022 •

edited

Loading

lgtm-com bot commented Feb 9, 2022

Nikolaj-K Feb 9, 2022

lgtm-com bot commented Feb 9, 2022

lgtm-com bot commented Feb 10, 2022

lgtm-com bot commented Feb 10, 2022

lgtm-com bot commented Feb 10, 2022

lgtm-com bot commented Feb 10, 2022

lgtm-com bot commented Feb 10, 2022

lgtm-com bot commented Feb 11, 2022

arvidn Feb 11, 2022

mariano54 Feb 11, 2022

lgtm-com bot commented Feb 11, 2022

lgtm-com bot commented Feb 11, 2022

lgtm-com bot commented Feb 12, 2022

mariano54 commented Feb 14, 2022

Yostra Feb 14, 2022

mariano54 Feb 14, 2022

Yostra Feb 14, 2022

mariano54 Feb 14, 2022

Ms.wallet refactor #10146

Ms.wallet refactor #10146

Conversation

mariano54 commented Feb 8, 2022 • edited Loading

lgtm-com bot commented Feb 9, 2022

Nikolaj-K Feb 9, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Feb 9, 2022

lgtm-com bot commented Feb 10, 2022

lgtm-com bot commented Feb 10, 2022

lgtm-com bot commented Feb 10, 2022

lgtm-com bot commented Feb 10, 2022

lgtm-com bot commented Feb 10, 2022

lgtm-com bot commented Feb 11, 2022

arvidn Feb 11, 2022

Choose a reason for hiding this comment

mariano54 Feb 11, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Feb 11, 2022

lgtm-com bot commented Feb 11, 2022

lgtm-com bot commented Feb 12, 2022

mariano54 commented Feb 14, 2022

Yostra Feb 14, 2022

Choose a reason for hiding this comment

mariano54 Feb 14, 2022

Choose a reason for hiding this comment

Yostra Feb 14, 2022

Choose a reason for hiding this comment

mariano54 Feb 14, 2022

Choose a reason for hiding this comment

mariano54 commented Feb 8, 2022 •

edited

Loading