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.

Sign up for GitHub

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

Jump to bottom

feat: timestamp validation #4887

Merged
merged 5 commits into from
Nov 22, 2022
Merged

feat: timestamp validation #4887

merged 5 commits into from
Nov 22, 2022

Conversation

Cifko
Copy link
Contributor

@Cifko Cifko commented Nov 4, 2022

Description

Disable timestamp validation for orphan.
Enable during orphan new tips creation. It should be checked during the new tips creation so we don't select invalid tip in favor of lower difficulty but valid tip.

@stringhandler
Copy link
Collaborator

Related to #4870

@CjS77 CjS77 added the P-merge Process - Queued for merging label Nov 7, 2022
@stringhandler stringhandler added the P-clippy_failed Process - Clippy has failed label Nov 7, 2022
stringhandler
stringhandler reviewed Nov 7, 2022
View reviewed changes
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

NACK. I don't think this is what @SWvheerden had in mind when logging the issue

@stringhandler stringhandler added P-do_not_merge Process - Not ready for merging and removed P-merge Process - Queued for merging labels Nov 7, 2022
SWvheerden
SWvheerden reviewed Nov 7, 2022
View reviewed changes
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.

utACK

I had an easier fix in mind with just not running this with the header validation. And only running this when we actually add the block, but this is fine.

I do however want to test run this for a day to see if it works as intended.

@SWvheerden SWvheerden added the P-high-risk Process - High risk label Nov 7, 2022
@SWvheerden
Copy link
Collaborator

I did manage to sync one and tan through the night without any issues so far.

@stringhandler stringhandler added the P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues label Nov 8, 2022
@Cifko
Copy link
Contributor Author

Cifko commented Nov 9, 2022

I'm going through the code. And I don't see any validation of the median timestamp outside of the orphan tree construction. In the whole code we validate the median timestamp at two location, header sync and orphan tree construction. I can add the timestamp validation if the block added is not an orphan. But once we add anything from the stored orphan block there is no validation. Currently there is some validation that is not consistent across nodes.
When we decide that some orphan chain will be added we do only body validation. I can't find the header validation. Any tips where it is, or what direction should I take now?

@CjS77 CjS77 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 Nov 9, 2022
@SWvheerden
Copy link
Collaborator

I would just add the check here:

tari/base_layer/core/src/validation/block_validators/body_only.rs

Line 58 in 55d1334

fn validate_body_for_valid_orphan(

This should be the last validator that's gets called right before a block is added to the database.

@Cifko Cifko force-pushed the timestamp-validation branch from cc81ea6 to eae7b18 Compare November 11, 2022 18:10
SWvheerden
SWvheerden reviewed Nov 14, 2022
View reviewed changes
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.

utACK

This is only done for block propagation and not syncing.
But I think this should be fine as header sync will add and validate blocks in order.

But perhaps just in terms of keeping header/block sync and block propagation validation in sync, we should make the the same changes there?

@Cifko
Copy link
Contributor Author

Cifko commented Nov 15, 2022

utACK

This is only done for block propagation and not syncing. But I think this should be fine as header sync will add and validate blocks in order.

But perhaps just in terms of keeping header/block sync and block propagation validation in sync, we should make the the same changes there?

Added. But IMHO I would not go this way. The sync has the validation in the headersync where it belongs. Now we do it double.

SWvheerden
SWvheerden reviewed Nov 16, 2022
View reviewed changes
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.

utACK

@sdbondi
Copy link
Member

sdbondi commented Nov 18, 2022 •
edited
Loading

Yeah, agree with @Cifko, the async block validator does not need to validate headers, and that is efficiently done in the header sync by knowing that it will receive sequential headers.

That said, the perf hit is minor so this could go in as is

stringhandler
stringhandler previously approved these changes Nov 18, 2022
View reviewed changes
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Can be merged once CI passes

sdbondi
sdbondi previously approved these changes Nov 18, 2022
View reviewed changes
@Cifko Cifko dismissed stale reviews from sdbondi and stringhandler via df78bdd November 18, 2022 10:45
@Cifko Cifko force-pushed the timestamp-validation branch from df78bdd to 80d7562 Compare November 21, 2022 07:33
@sdbondi
Copy link
Member

sdbondi commented Nov 21, 2022

utACK

@stringhandler stringhandler merged commit 4be02b6 into tari-project:development Nov 22, 2022
sdbondi added a commit to sdbondi/tari that referenced this pull request Nov 23, 2022
@sdbondi
Merge branch 'development' into merge-dev-feature-dan
0eda691 
* development:
  fix: add hidden types and seed words to key manager (tari-project#4925)
  feat: timestamp validation (tari-project#4887)
  fix: deleted_txo_mmr_position_to_height_index  already exists error (tari-project#4924)
  feat: add default grpc for localnet (tari-project#4937)
  first commit (tari-project#4926)
  v0.40.2
  fix(dht): use limited ban period for invalid peer (tari-project#4933)
  feat: upgrade tari_crypto sign api (tari-project#4932)
  v0.40.1
  chore: fix depreciated timestamp clippy (tari-project#4929)
  chore: fix naming of ffi functions and comments  (tari-project#4930)
  fix: set wallet start scan height to birthday and not 0 (see issue tari-project#4807) (tari-project#4911)
  v0.40.0
  chore: remove unused methods (tari-project#4922)
  fix: updates for SafePassword API change (tari-project#4927)
@Cifko Cifko deleted the timestamp-validation branch April 18, 2023 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SWvheerden SWvheerden SWvheerden left review comments

@sdbondi sdbondi sdbondi approved these changes

@stringhandler stringhandler stringhandler left review comments

Assignees
No one assigned
Labels
mq-failed P-acks_required Process - Requires more ACKs or utACKs P-clippy_failed Process - Clippy has failed P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues P-do_not_merge Process - Not ready for merging P-high-risk Process - High risk P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Cifko @stringhandler @SWvheerden @sdbondi @CjS77