feat: consistent handling of edge cases for header sync #5837

hansieodendaal · 2023-10-11T16:07:09Z

Description

This PR fixes an edge case for header sync where the local node has more headers, but the remote node has better proof-of-work (accumulated difficulty), thus higher actual blockchain height. It also simplifies the attempted header sync logic and error handling when a peer does not return headers and introduces consistent naming, variable re-use and use of information

Supersedes #5756

Motivation and Context

When doing header sync, and determining from which peer to sync, we need to consider our actual blockchain state when comparing chains. It is possible that our node has valid block headers that will translate into a higher proof of work when fully synced than the remote syncing peer, but lacking the blocks to back the block headers, thus the remote chain has an actual higher proof of work blockchain.
With the current implementation, race conditions can exist when determining if the peer is misbehaving, i.e. lying about their proof-of-work or not wanting to supply block headers, due to a mismatch in tip height used for the check.

This PR fixes the race conditions by always using the latest local chain metadata and block headers, ignoring all sync peers that do not exceed our own accumulated difficulty and using consistent information in all the checks.

How Has This Been Tested?

Added integration-level unit tests:

test_header_sync_happy_path
test_header_sync_with_fork_happy_path
test_header_sync_uneven_headers_and_blocks_happy_path
test_header_sync_uneven_headers_and_blocks_peer_lies_about_pow_no_ban
test_header_sync_even_headers_and_blocks_peer_lies_about_pow_with_ban
test_header_sync_even_headers_and_blocks_peer_metadata_improve_with_reorg

What process can a PR reviewer use to test or verify this change?

Code walk-through
Review unit tests

Breaking Changes

None
Requires data directory on base node to be deleted
Requires hard fork
Other - Please specify

github-actions · 2023-10-11T16:22:33Z

Test Results (CI)

1 239 tests 1 239 ✔️ 9m 28s ⏱️
    39 suites       0 💤
      1 files       0 ❌

Results for commit 323e3f2.

♻️ This comment has been updated with latest results.

github-actions · 2023-10-11T16:24:55Z

Test Results (Integration tests)

  2 files +  2 11 suites +11 22m 0s ⏱️ + 22m 0s
34 tests +34 33 ✔️ +33 0 💤 ±0 1 ❌ +1
35 runs +35 34 ✔️ +34 0 💤 ±0 1 ❌ +1

For more details on these failures, see this check.

Results for commit 323e3f2. ± Comparison against base commit 43b994e.

♻️ This comment has been updated with latest results.

base_layer/core/src/base_node/sync/header_sync/synchronizer.rs

sdbondi

Nice - utACK

SWvheerden

looks good, just needs a few commented out lines removed

applications/minotari_console_wallet/src/ui/components/transactions_tab.rs

base_layer/core/src/base_node/sync/header_sync/synchronizer.rs

base_layer/core/tests/tests/header_sync.rs

base_layer/core/src/base_node/sync/header_sync/synchronizer.rs

stringhandler · 2023-10-18T12:44:40Z

base_layer/core/src/base_node/sync/header_sync/synchronizer.rs

-                         {})",
-                        header_tip_height,
-                        metadata.height_of_longest_chain(),
+                         {}), peer has lied about chain metadata or did not want to provide headers",


This is not true. We only checked if our best_block < our best_headers

Its true by deduction.
For us to have entered Header_sync the peer needed tot have a better pow(it claimed it did)
Then we check that out pow did not update and that they still have a claimed higher pow.

We then asked the peer to provide us the chain which had a better pow, but it could not. So we have the same or more headers on the exact same chain as they have. But we can have the headers, not the block, so if we check if we have the same blocks than headers, we must have the same or more blocks than they. Its not possible to have blocks and not the headers.

So the only conclusion we can make is one of the following 3:
They lied about their pow
They did not want to provide us with a higher chain
The swapped to a lower-difficulty pow chain.

So we are only left with 1 and 2 realisticly.

Ok, I provided better data for a better decision

Please provide a link to the new code or more detail about what you did, I can't find it among the other changes

The conditions are now tested separately and both will be ban-able offences.
See

// Basic sanity check about the peer's claimed chain metadata if chain_split_result.peer_accumulated_difficulty < sync_peer.claimed_chain_metadata().accumulated_difficulty()

and

match header_sync_status.clone() { HeaderSyncStatus::InSyncOrAhead => { // Our POW is less than the peer's POW, as verified before the attempted header sync, therefore, if the // peer did not supply any headers and we know we are behind based on the peer's claimed metadata, then // we can ban the peer. if best_header.height() == best_block.height() {

base_layer/core/src/base_node/sync/header_sync/synchronizer.rs

stringhandler · 2023-10-18T12:56:55Z

base_layer/core/tests/helpers/nodes.rs

@@ -105,6 +108,7 @@ pub struct BaseNodeBuilder {
    mempool_config: Option<MempoolConfig>,
    mempool_service_config: Option<MempoolServiceConfig>,
    liveness_service_config: Option<LivenessConfig>,
+    p2p_config: Option<P2pConfig>,


Not related to this PR, but this struct should really be called "TestBaseNodeBuilder"

stringhandler · 2023-10-18T12:59:09Z

base_layer/p2p/src/initialization.rs

@@ -135,7 +135,7 @@ pub async fn initialize_local_test_comms<P: AsRef<Path>>(
    discovery_request_timeout: Duration,
    seed_peers: Vec<Peer>,
    shutdown_signal: ShutdownSignal,
-) -> Result<(CommsNode, Dht, MessagingEventSender), CommsInitializationError> {
+) -> Result<(UnspawnedCommsNode, Dht, MessagingEventSender), CommsInitializationError> {


Unrelated: This test method should also be moved into a file that shows it's a test method. Because the method is so long, it is difficult to see that it's test code

SWvheerden

adding in a new field to the proto for the peer to also send is meaningless.
They already sent us one that they claim is higher than ours. All we have to do now is verify it, we should not ask for a new one.

base_layer/core/src/base_node/proto/rpc.proto

base_layer/core/src/base_node/sync/header_sync/synchronizer.rs

Bumps [rustix](https://github.com/bytecodealliance/rustix) from 0.37.23 to 0.37.26. - [Release notes](https://github.com/bytecodealliance/rustix/releases) - [Commits](bytecodealliance/rustix@v0.37.23...v0.37.26) --- updated-dependencies: - dependency-name: rustix dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>

This PR fixes an edge case for header sync where the local node has a higher chain header, but the remote node has a higher actual blockchain height. It also simplifies the attempted header sync logic and error handling when a peer does not return headers and introduces consistent naming, variable re-use and use of information. When doing header sync, and determining from which peer to sync, we need to consider our actual blockchain state when comparing chains. It is possible that our node has valid block headers that will translate into a higher proof of work when fully synced than the remote syncing peer, but lacking the blocks to back the block headers, thus the remote chain has an actual higher proof of work blockchain. With the current implementation, race conditions can exist when determining if the peer is misbehaving, i.e. lying about their proof-of-work or not wanting to supply block headers, due to a mismatch in tip height used for the check. This PR fixes the race conditions by always using the latest local chain metadata and block headers, ignoring all sync peers that do not exceed our own accumulated difficulty and using consistent information in all the checks. Added a header sync tests integration unit test.

SWvheerden · 2023-10-23T16:07:34Z

Fixes: #5816

SWvheerden

Looking good

base_layer/core/src/base_node/sync/header_sync/error.rs

base_layer/core/tests/helpers/sync.rs

base_layer/core/tests/tests/header_sync.rs

ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Oct 11, 2023

SWvheerden mentioned this pull request Oct 12, 2023

fix: edge case about sync #5756

Closed

4 tasks

hansieodendaal force-pushed the ho_header_sync branch 7 times, most recently from 495f44a to 97879b3 Compare October 17, 2023 18:09

hansieodendaal changed the title ~~[wip] feat: consistent handling of edge cases for header sync~~ feat: consistent handling of edge cases for header sync Oct 17, 2023

hansieodendaal force-pushed the ho_header_sync branch 2 times, most recently from b5adc65 to febf997 Compare October 17, 2023 19:22

hansieodendaal commented Oct 18, 2023

View reviewed changes

base_layer/core/src/base_node/sync/header_sync/synchronizer.rs Outdated Show resolved Hide resolved

hansieodendaal requested review from sdbondi, SWvheerden and stringhandler October 18, 2023 06:05

sdbondi previously approved these changes Oct 18, 2023

View reviewed changes

ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Oct 18, 2023

hansieodendaal dismissed sdbondi’s stale review via 4caa193 October 18, 2023 10:31

hansieodendaal force-pushed the ho_header_sync branch from 88e5830 to 4caa193 Compare October 18, 2023 10:31

SWvheerden reviewed Oct 18, 2023

View reviewed changes

ghpbot-tari-project added P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Oct 18, 2023

stringhandler reviewed Oct 18, 2023

View reviewed changes

hansieodendaal force-pushed the ho_header_sync branch 2 times, most recently from 6326491 to 8e46cc5 Compare October 19, 2023 02:42

hansieodendaal force-pushed the ho_header_sync branch from 8e46cc5 to 6affe84 Compare October 19, 2023 02:52

SWvheerden requested changes Oct 19, 2023

View reviewed changes

base_layer/core/src/base_node/proto/rpc.proto Outdated Show resolved Hide resolved

base_layer/core/src/base_node/sync/header_sync/synchronizer.rs Outdated Show resolved Hide resolved

base_layer/core/src/base_node/sync/header_sync/synchronizer.rs Outdated Show resolved Hide resolved

hansieodendaal force-pushed the ho_header_sync branch 2 times, most recently from b400759 to 2535e04 Compare October 23, 2023 08:15

dependabot bot and others added 7 commits October 23, 2023 11:04

review comments

592da2e

added more unit tests

caf3302

review comments

09151a2

Simplification

7591bda

more simplification

4eeb873

hansieodendaal force-pushed the ho_header_sync branch from 971da89 to 4eeb873 Compare October 23, 2023 12:08

hansieodendaal requested review from stringhandler and SWvheerden October 23, 2023 12:38

licence check

3b7664d

hansieodendaal force-pushed the ho_header_sync branch from 5b774c5 to 3b7664d Compare October 23, 2023 12:51

Finding TARI-038

9f3b3ff

SWvheerden reviewed Oct 23, 2023

View reviewed changes

hansieodendaal added 2 commits October 24, 2023 08:24

further review comments

d91a5e0

expand comment

323e3f2

SWvheerden approved these changes Oct 24, 2023

View reviewed changes

SWvheerden merged commit 3e1ec1f into tari-project:development Oct 24, 2023
12 of 13 checks passed

hansieodendaal deleted the ho_header_sync branch October 25, 2023 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: consistent handling of edge cases for header sync #5837

feat: consistent handling of edge cases for header sync #5837

hansieodendaal commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023 •

edited

Loading

sdbondi left a comment

SWvheerden left a comment

stringhandler Oct 18, 2023

SWvheerden Oct 18, 2023 •

edited

Loading

hansieodendaal Oct 18, 2023

stringhandler Oct 19, 2023

hansieodendaal Oct 20, 2023

stringhandler Oct 18, 2023

stringhandler Oct 18, 2023

SWvheerden left a comment

SWvheerden commented Oct 23, 2023

SWvheerden left a comment

feat: consistent handling of edge cases for header sync #5837

feat: consistent handling of edge cases for header sync #5837

Conversation

hansieodendaal commented Oct 11, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

What process can a PR reviewer use to test or verify this change?

Breaking Changes

github-actions bot commented Oct 11, 2023 • edited Loading

Test Results (CI)

github-actions bot commented Oct 11, 2023 • edited Loading

Test Results (Integration tests)

sdbondi left a comment

Choose a reason for hiding this comment

SWvheerden left a comment

Choose a reason for hiding this comment

stringhandler Oct 18, 2023

Choose a reason for hiding this comment

SWvheerden Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

hansieodendaal Oct 18, 2023

Choose a reason for hiding this comment

stringhandler Oct 19, 2023

Choose a reason for hiding this comment

hansieodendaal Oct 20, 2023

Choose a reason for hiding this comment

stringhandler Oct 18, 2023

Choose a reason for hiding this comment

stringhandler Oct 18, 2023

Choose a reason for hiding this comment

SWvheerden left a comment

Choose a reason for hiding this comment

SWvheerden commented Oct 23, 2023

SWvheerden left a comment

Choose a reason for hiding this comment

hansieodendaal commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023 •

edited

Loading

SWvheerden Oct 18, 2023 •

edited

Loading