Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: validate blockchain fetch from DNS server with local state #6658

Merged
merged 14 commits into from
Nov 19, 2024

Conversation

MCozhusheck
Copy link
Contributor

Description

Add new service which will periodically get checkpoints from a DNS server and validate with it's own state. If we fail this check we will include this information in get_new_block_template request so the sender is aware that this node is out of sync.

Motivation and Context

This will allow miners including one from Tari Universe to avoid mining on the orphan chain. This implementation is
modeled after the Monero Pulse

How Has This Been Tested?

Run app on the nextnet and look for logs with tari_pulse in the base_layer.log file like this:
tail -f base_layer.log | grep tari_pulse

Make get_new_block_template and verify if new fields are present.

Create orphan chain (eg. by turning off tor and banning all TCP peers) and check (with above methods) if nodes detects orphan chain.

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

Same as above

Breaking Changes

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

@MCozhusheck MCozhusheck changed the base branch from development to nextnet November 15, 2024 10:41
@MCozhusheck MCozhusheck changed the base branch from nextnet to development November 15, 2024 10:42
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, a few comments

base_layer/core/src/base_node/tari_pulse_service/mod.rs Outdated Show resolved Hide resolved
applications/minotari_app_grpc/proto/base_node.proto Outdated Show resolved Hide resolved
base_layer/core/src/base_node/tari_pulse_service/mod.rs Outdated Show resolved Hide resolved
base_layer/core/src/base_node/tari_pulse_service/mod.rs Outdated Show resolved Hide resolved
base_layer/core/src/base_node/tari_pulse_service/mod.rs Outdated Show resolved Hide resolved
@MCozhusheck MCozhusheck marked this pull request as ready for review November 15, 2024 15:29
@MCozhusheck MCozhusheck requested a review from a team as a code owner November 15, 2024 15:29
@MCozhusheck
Copy link
Contributor Author

Support for other chains than nextnet have yet to be implemented after these dns servers are up and running.

Copy link

github-actions bot commented Nov 17, 2024

Test Results (CI)

    3 files    129 suites   40m 44s ⏱️
1 344 tests 1 344 ✅ 0 💤 0 ❌
4 030 runs  4 030 ✅ 0 💤 0 ❌

Results for commit 877ec62.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 17, 2024

Test Results (Integration tests)

 2 files  + 2  11 suites  +11   16m 58s ⏱️ + 16m 58s
36 tests +36  35 ✅ +35  0 💤 ±0  1 ❌ +1 
37 runs  +37  36 ✅ +36  0 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 877ec62. ± Comparison against base commit 10f4077.

♻️ This comment has been updated with latest results.

base_layer/core/src/base_node/tari_pulse_service/mod.rs Outdated Show resolved Hide resolved
base_layer/core/src/base_node/tari_pulse_service/mod.rs Outdated Show resolved Hide resolved
base_layer/core/src/base_node/tari_pulse_service/mod.rs Outdated Show resolved Hide resolved
base_layer/core/src/base_node/tari_pulse_service/mod.rs Outdated Show resolved Hide resolved
applications/minotari_node/src/config.rs Outdated Show resolved Hide resolved
base_layer/core/src/base_node/tari_pulse_service/mod.rs Outdated Show resolved Hide resolved
@hansieodendaal
Copy link
Contributor

I am unsure about this approach altogether, as it centralizes authority and appoints the DNS server as an oracle that can influence outcomes.

@MCozhusheck
Copy link
Contributor Author

I am unsure about this approach altogether, as it centralizes authority and appoints the DNS server as an oracle that can influence outcomes.

This is only used to hint that this node might be not connected to the rest of the network. This was common issue in Tari Universe where most of the network was connected through Tor but after disabling this feature in the settings, users where trying to connect through TCP where there no other nodes.
There are some other concerns that required Monero to introduce Monero Pulse which solved 1 vulnerability regarding invalid historical block in the chain.

Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SWvheerden SWvheerden merged commit 0610bcc into tari-project:development Nov 19, 2024
16 of 17 checks passed
hansieodendaal added a commit to hansieodendaal/tari that referenced this pull request Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants