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:] Time consensus implementation #300

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jaoleal
Copy link
Contributor

@jaoleal jaoleal commented Dec 5, 2024

Since #198 became abandoned and wasnt updated i decided to reimplement what was done from that PR.

This PR tries to add the validation for Time related data to floresta.
To implement this i ran trough some solutions and here they are:

  • locktime inside the transactions: To validate them i need the MTP and the timestamp of the block when the utxo was created.
  • Block Timestamp: To validate if a block is inserting the correct timestamp i need the MTP of the chain.

This changes implements a new struct and a trait on mod and a new function on Consensus .

  • Trait NodeTime was made just to delegate real-time notion of time.
  • Struct Utxo_Data for holding the utxo together with block-height and time data of the commitment of the utxo, when it was created.
  • validate_locktime method inside Consensus to consume the other solutions (MTP, UTXO TIME-DATA) and to validate time itself.

The work from now on in this PR is:

  • Include validate_locktime and populate support for it, changing some methods signature(including validate_transactions, validate_block and etc...).
  • Implement a way of having MTP notion. Its a problematic because of how we organize our IBD and i dont have a good solution for it, besides including MTP validation only after the chain is validated(any thoughts on this ?).

@jaoleal jaoleal force-pushed the time_consensus_implementation branch from 4772a7d to f602686 Compare December 5, 2024 19:27
@jaoleal
Copy link
Contributor Author

jaoleal commented Dec 5, 2024

My master was behind 0.7.0 bump.
@Davidson-Souza you can tag this as 0.8.0 milestone

@Davidson-Souza Davidson-Souza added this to the v0.8.0 milestone Dec 5, 2024
@Davidson-Souza Davidson-Souza added the enhancement New feature or request label Dec 5, 2024
Copy link
Collaborator

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

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

Overall looks good, some implementation comments.

crates/floresta-chain/src/pruned_utreexo/consensus.rs Outdated Show resolved Hide resolved
crates/floresta-chain/src/pruned_utreexo/consensus.rs Outdated Show resolved Hide resolved
@jaoleal jaoleal force-pushed the time_consensus_implementation branch from 14dced6 to 4cc0df3 Compare January 15, 2025 14:25
@jaoleal
Copy link
Contributor Author

jaoleal commented Jan 15, 2025

Applied changes by @Davidson-Souza.

4cc0df3 Tries to implement get_mtp() to be used after IBD due to the fact that a PartialChainState isnt too reliable to process this data.

Note to reviewers

Im still not glad with this solution to validate time-related data and still a lot of work to do around this changes.

I would like to:

  1. Completely delete UtxoMap to stop the conflict between hashmaps.
  2. (Possibly) Implement get_mtp() for PartialChainState using blockheaders but i still need to understand its behavior in runtime.
  3. Remove the usage of UtxoData too, but this would need to store height and time info on a separate location since its crucial to validate locktime.

@jaoleal jaoleal force-pushed the time_consensus_implementation branch 3 times, most recently from b8b66b6 to 6bbe2c9 Compare January 22, 2025 14:28
…s: NodeTime.

add: get_mtp() method for Persisted ChainState.
fix: time-related validation for Persisted ChainState.
@jaoleal jaoleal force-pushed the time_consensus_implementation branch from 6bbe2c9 to c58203d Compare January 22, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants