feat(sync-v2): sync-v2 implemented, sync-v1 still default #275

jansegre · 2021-07-21T00:59:23Z

These changes were separated from #236, it should now have mostly the sync-v2 base code itself. RFC

Acceptance Criteria

Fix DepsIndex and its both implementations (memory and rocksdb), allowing voided vertices and using the correct scope when retrieving vertices from the storage.
Change SyncVersion.V2 to 'v2' (previously 'v2-fake').
Add SyncV2Factory and use it on ConnectionsManager._sync_factories.
Add NodeBlockSync that sync blocks and its transactions.
Add BlockchainStreaming and TransactionsStreaming to stream vertices as requested by peers. They are used by NodeBlockSync.
Add TransactionStorage.iter_mempool_tips_from_best_index().
Refactor some tests to run with both sync-v1 and sync-v2.

Current issues

Sync checkpoint seems to hang when syncing from zero (happens sometimes and not always at the same height);
Sync checkpoint seems to not resume properly sometimes after restarting (happens sometimes and not always at the same height);
Stats counters aren't being update (they might have to be redesigned since they were originally made for sync-v1 and some counters can't have the same semantics);
When using rocksdb-indexes, the memory-deps index is not re-initialized correctly because the iterator will ignore partially validated transactions;
Deal with syncing soft-voided txs, which didn't exist when sync-v2 was designed, and since sync-v2 doesn't sync voided txs/blocks they need special treatment;
Some tx-reward-lock validation could fail during checkpoint-sync;
Sync blocks seems to hang requesting blocks from the same height: with checkpoints disabled the node was only able to sync about 30% of the mainnet;

hathor/consensus.py

hathor/transaction/storage/transaction_storage.py

hathor/p2p/manager.py

hathor/transaction/storage/block_height_index.py

hathor/transaction/transaction.py

hathor/manager.py

hathor/transaction/base_transaction.py

hathor/transaction/storage/transaction_storage.py

tests/p2p/test_capabilities.py

tests/simulation/test_simulator.py

tests/unittest.py

tests/utils.py

hathor/consensus.py

tests/p2p/test_protocol.py

hathor/p2p/sync_checkpoints.py

hathor/p2p/sync_mempool.py

hathor/p2p/node_sync_v2.py

hathor/manager.py

hathor/transaction/storage/transaction_storage.py

hathor/transaction/transaction.py

tests/p2p/test_capabilities.py

luislhl · 2023-07-24T15:21:48Z

hathor/p2p/sync_v2/manager.py

+        if self._started:
+            raise Exception('NodeSyncBlock is already running')
+        self._started = True
+        self._lc_run.start(5)


Would there be any penalty for us if we decrease this time from 5s to maybe 1s or 2s?

I have the impression we could be losing some seconds between the end of the current streaming and the start of the next one.

We might just move it to a variable and let us change it through syscall.

That makes sense, I'll move it to sysctl.

hathor/p2p/sync_v2/manager.py

glevco · 2023-07-24T21:44:06Z

hathor/p2p/sync_v2/manager.py

+        tx_bytes = base64.b64decode(payload)
+        tx = tx_or_block_from_bytes(tx_bytes)
+        assert tx.hash is not None
+        if not isinstance(tx, Transaction):
+            self.log.warn('not a transaction', hash=tx.hash_hex)
+            # Not a transaction. Punish peer?
+            return
+
+        self._tx_received += 1
+        if self._tx_received > self._tx_max_quantity + 1:
+            self.log.warn('too many txs received')
+            self.state = PeerState.ERROR
+            return
+
+        try:
+            # this methods takes care of checking if the tx already exists, it will take care of doing at least
+            # a basic validation
+            # self.log.debug('add new tx', tx=tx.hash_hex)
+            if self.partial_vertex_exists(tx.hash):
+                # XXX: early terminate?
+                self.log.debug('tx early terminate?', tx_id=tx.hash.hex())
+            else:
+                self.log.debug('tx received', tx_id=tx.hash.hex())
+            self.on_new_tx(tx, propagate_to_peers=False, quiet=True, reject_locked_reward=True)
+        except HathorError:
+            self.log.warn('invalid new tx', exc_info=True)
+            # Invalid block?!
+            # Invalid transaction?!
+            # Maybe stop syncing and punish peer.
+            self.state = PeerState.ERROR
+            return


If we are stopping sync and/or punishing a peer for sending invalid txs, we shouldn't forget about handling invalid payloads on lines 961 and 962. The same for handle_blocks()

Maybe refactor the following lines to a _parse_tx_bytes(tx_bytes: bytes) -> BaseTransaction method.

tx_bytes = base64.b64decode(payload) tx = tx_or_block_from_bytes(tx_bytes)

So, handlers can check vertex types only.

glevco · 2023-07-24T21:47:03Z

hathor/p2p/sync_v2/manager.py

+            if tx is None:
+                self.log.error('failed to get tx', tx_id=tx_id.hex())
+                self.protocol.send_error_and_close_connection(f'DATA mempool {tx_id.hex()} not found')
+                raise
+            if tx.hash != tx_id:
+                self.protocol.send_error_and_close_connection(f'DATA mempool {tx_id.hex()} hash mismatch')
+                raise


There are multiple places where we call send_error_and_close_connection() but do not raise. Considering consistency, is this expected? Should we raise in the other places too, or not raise here?

I think it makes more sense to return instead.

glevco · 2023-07-24T21:47:25Z

hathor/p2p/sync_v2/manager.py

+                raise
+        return tx
+
+    def get_data(self, tx_id: bytes, origin: str) -> Deferred:


Should origin be an enum?

Maybe. Not sure whether it has the peer id or its more related to the type of origin (sync-v2-mempool, sync-v2-blocks, sync-v2-mempool).

I think it makes sense to use an enum, I'll also refactor the methods a little bit.

hathor/manager.py

tests/event/test_event_simulation_scenarios.py

Co-authored-by: Marcelo Salhab Brogliato <[email protected]> Co-authored-by: Pedro Ferreira <[email protected]>

jansegre self-assigned this Jul 21, 2021

jansegre force-pushed the feat/sync-v2-mvp branch 2 times, most recently from d3fda51 to aea60ec Compare July 21, 2021 03:10

jansegre mentioned this pull request Jul 21, 2021

refactor(sync-v2): introduce all structures needed without any sync-v2 #236

Merged

jansegre force-pushed the feat/sync-v2-mvp branch from aea60ec to 124a00e Compare July 22, 2021 20:48

jansegre force-pushed the feat/sync-v2-mvp2 branch from 266c111 to 4cae7ef Compare July 22, 2021 20:49

jansegre commented Jul 26, 2021

View reviewed changes

hathor/consensus.py Outdated Show resolved Hide resolved

jansegre commented Jul 26, 2021

View reviewed changes

hathor/transaction/storage/transaction_storage.py Outdated Show resolved Hide resolved

jansegre force-pushed the feat/sync-v2-mvp branch from 124a00e to 1e30cd3 Compare July 27, 2021 23:45

jansegre force-pushed the feat/sync-v2-mvp2 branch 2 times, most recently from 67cd607 to 92c4036 Compare July 28, 2021 01:06

jansegre force-pushed the feat/sync-v2-mvp branch from 1e30cd3 to 336cdbc Compare July 28, 2021 01:31

jansegre force-pushed the feat/sync-v2-mvp2 branch from 92c4036 to 7648a4b Compare July 28, 2021 03:04

jansegre force-pushed the feat/sync-v2-mvp branch 2 times, most recently from 1584396 to 6abbfed Compare July 28, 2021 04:20

jansegre force-pushed the feat/sync-v2-mvp2 branch from 7648a4b to 768047f Compare July 28, 2021 04:27

jansegre force-pushed the feat/sync-v2-mvp branch from 6abbfed to 02ffc0d Compare July 29, 2021 02:06

Base automatically changed from feat/sync-v2-mvp to dev July 29, 2021 05:42

msbrogli force-pushed the feat/sync-v2-mvp2 branch from 768047f to b1d6f5a Compare July 29, 2021 06:04

jansegre changed the title ~~feat(sync-v2): minimal sync-v2 implemented, sync-v1 still default~~ feat(sync-v2): sync-v2 implemented, sync-v1 still default Jul 29, 2021

jansegre force-pushed the feat/sync-v2-mvp2 branch 2 times, most recently from 4aaec0f to e11ff9d Compare July 30, 2021 01:02

msbrogli requested changes Jul 30, 2021

View reviewed changes

msbrogli requested changes Jul 31, 2021

View reviewed changes

hathor/consensus.py Outdated Show resolved Hide resolved

tests/p2p/test_protocol.py Outdated Show resolved Hide resolved

msbrogli requested changes Jul 31, 2021

View reviewed changes

jansegre force-pushed the feat/sync-v2-mvp2 branch from e11ff9d to 4245bb4 Compare July 31, 2021 01:50

msbrogli requested changes Jul 31, 2021

View reviewed changes

luislhl reviewed Jul 24, 2023

View reviewed changes

hathor/p2p/sync_v2/manager.py Show resolved Hide resolved

glevco reviewed Jul 24, 2023

View reviewed changes

jansegre force-pushed the feat/sync-v2-mvp2 branch from a9dc05b to a7581ef Compare July 25, 2023 22:32

feat(sync-v2): sync-v2 implemented, sync-v1 still default

f63f3b8

Co-authored-by: Marcelo Salhab Brogliato <[email protected]> Co-authored-by: Pedro Ferreira <[email protected]>

jansegre force-pushed the feat/sync-v2-mvp2 branch from a7581ef to f63f3b8 Compare July 25, 2023 22:43

jansegre merged commit bdfc5fc into master Jul 25, 2023

jansegre deleted the feat/sync-v2-mvp2 branch July 25, 2023 22:44

This was referenced Jul 26, 2023

Improve bandwith consumption of sync-v2 messages #724

Open

Improve typing of sync-v2 messages #725

Open

Improve sync-v2's deferred_by_key implementation #726

Closed

Improve typing of sync-v2 status API #727

Open

Improve modularization of sync-v2 #728

Open

luislhl mentioned this pull request Jul 26, 2023

Several seconds taken to log the receival of BLOCKS-END in sync-v2 #731

Closed

This was referenced Jul 27, 2023

Release-candidate v0.55.0-rc.2 #723

Merged

Release v0.55.0 #740

Closed

Release v0.55.0 #741

Merged

jansegre mentioned this pull request Aug 22, 2023

refactor(sync-v2): cleanup after main PR was merged [part 1] #752

Draft

1 task

jansegre added a commit that referenced this pull request Sep 5, 2023

address #275 (comment)

2bb36ae

jansegre added a commit that referenced this pull request Sep 5, 2023

address #275 (comment)

2423061

jansegre added a commit that referenced this pull request Sep 5, 2023

address #275 (comment)

2495082

jansegre added a commit that referenced this pull request Sep 8, 2023

address #275 (comment)

dc650f3

jansegre added a commit that referenced this pull request Sep 8, 2023

address #275 (comment)

6e41808

jansegre added a commit that referenced this pull request Sep 8, 2023

address #275 (comment)

d5765bd

jansegre added a commit that referenced this pull request Sep 8, 2023

address #275 (comment)

612b3b1

jansegre added a commit that referenced this pull request Sep 15, 2023

address #275 (comment)

141c000

jansegre added a commit that referenced this pull request Sep 15, 2023

address #275 (comment)

32da8dd

jansegre added a commit that referenced this pull request Sep 15, 2023

address #275 (comment)

522c0b6

jansegre added a commit that referenced this pull request Sep 15, 2023

address #275 (comment)

8a22f19

jansegre mentioned this pull request Jul 29, 2024

Refactor NodeBlockSync and SyncMempoolManager for better dependency injection #1099

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sync-v2): sync-v2 implemented, sync-v1 still default #275

feat(sync-v2): sync-v2 implemented, sync-v1 still default #275

jansegre commented Jul 21, 2021 •

edited by msbrogli

Loading

luislhl Jul 24, 2023 •

edited

Loading

msbrogli Jul 24, 2023

jansegre Jul 25, 2023

glevco Jul 24, 2023

msbrogli Jul 25, 2023

glevco Jul 24, 2023

jansegre Sep 4, 2023

glevco Jul 24, 2023

msbrogli Jul 24, 2023 •

edited

Loading

jansegre Sep 4, 2023

feat(sync-v2): sync-v2 implemented, sync-v1 still default #275

feat(sync-v2): sync-v2 implemented, sync-v1 still default #275

Conversation

jansegre commented Jul 21, 2021 • edited by msbrogli Loading

Acceptance Criteria

Current issues

luislhl Jul 24, 2023 • edited Loading

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

msbrogli Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jansegre commented Jul 21, 2021 •

edited by msbrogli

Loading

luislhl Jul 24, 2023 •

edited

Loading

msbrogli Jul 24, 2023 •

edited

Loading