[Feat:] Time consensus implementation

[jaoleal]

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]

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

[Davidson-Souza]

Overall looks good, some implementation comments.

[jaoleal]

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.

[Davidson-Souza]

[2025-01-27 13:10:49 ERROR floresta_wire::p2p_wire::sync_node] Invalid block Header { block_hash: 0000000000000151a3e5f74671a15f9d92c2a40d6280a0284cd01cfebc4260df, version: Version(2), prev_blockhash: 000000000000016476e1614b3b47374fabdb68926c67cc6f3c6ae7187bde4ac3, merkle_root: 1ff7571e96d8c35cffdd452a6eacc308d6b704de8a598d5751534de3bbfa91dc, time: 1358114045, bits: CompactTarget(436545969), nonce: 3551187880 } received by peer 334 reason: TransactionError(TransactionError { txid: 0c7de5f54c5ceadc5754098d687f40b9378d174e63cd4946ca0dd9dfd16e54e9, error: BadAbsoluteLockTime })

[jaoleal]

[2025-01-27 13:10:49 ERROR floresta_wire::p2p_wire::sync_node] Invalid block Header { block_hash: 0000000000000151a3e5f74671a15f9d92c2a40d6280a0284cd01cfebc4260df, version: Version(2), prev_blockhash: 000000000000016476e1614b3b47374fabdb68926c67cc6f3c6ae7187bde4ac3, merkle_root: 1ff7571e96d8c35cffdd452a6eacc308d6b704de8a598d5751534de3bbfa91dc, time: 1358114045, bits: CompactTarget(436545969), nonce: 3551187880 } received by peer 334 reason: TransactionError(TransactionError { txid: 0c7de5f54c5ceadc5754098d687f40b9378d174e63cd4946ca0dd9dfd16e54e9, error: BadAbsoluteLockTime })

This shouldnt be caught by just test ?

Anyway... Ill do better local tests, sorry. My next PR will be #260

[Davidson-Souza]

Looking very cool! One thing I'm not sure is whether get_mtp should be on UpdatableChainState. Usually these methods are used by who's driving the chainstate only, but mtp is useful elsewhere (e.g. the rpc).

[Davidson-Souza]

[2025-02-03 15:22:33 ERROR floresta_wire::p2p_wire::sync_node] Invalid block Header { block_hash: 00000000000000020ec35955d56c7742fe8655e1d7fff97398b39967267c765a, version: Version(2), prev_blockhash: 0000000000000001c522f786f1e7447117df4f601784205a8222b93ea47c10d7, merkle_root: a0ac4ffbd82588038d93b47ce0ecb2d334d7e4e0a0db83f5a06502d5cde0d813, time: 1389127783, bits: CompactTarget(419628831), nonce: 2587597736 } received by peer 401 reason: TransactionError(TransactionError { txid: 6e1fec76fd9c6b407665a75bd7ff04e3854de8098cdf7e5ce32a83872b98375d, error: BadRelativeLockTime })

[jaoleal]

Looking very cool! One thing I'm not sure is whether get_mtp should be on UpdatableChainState. Usually these methods are used by who's driving the chainstate only, but mtp is useful elsewhere (e.g. the rpc).

So would it be better located at BlockchainInterface ?

[Davidson-Souza]

Looking very cool! One thing I'm not sure is whether get_mtp should be on UpdatableChainState. Usually these methods are used by who's driving the chainstate only, but mtp is useful elsewhere (e.g. the rpc).

So would it be better located at BlockchainInterface ?

Yes!

[Davidson-Souza]

Some small things left. Also, please apply the following formatting diff.

Big scary diff

diff --git a/crates/floresta-chain/src/pruned_utreexo/chain_state.rs b/crates/floresta-chain/src/pruned_utreexo/chain_state.rs
index a35b68c..171bdcd 100644
--- a/crates/floresta-chain/src/pruned_utreexo/chain_state.rs
+++ b/crates/floresta-chain/src/pruned_utreexo/chain_state.rs
@@ -1006,6 +1006,7 @@ impl<PersistedState: ChainStore> BlockchainInterface for ChainState<PersistedSta
       let mut inner = write_lock!(self);
       inner.broadcast_queue.drain(..).collect()
   }
+
   fn get_mtp(&self, height: u32) -> Result<u32, BlockchainError> {
       let mut initial_array = [0u32; 11];

diff --git a/crates/floresta-chain/src/pruned_utreexo/consensus.rs b/crates/floresta-chain/src/pruned_utreexo/consensus.rs
index 4ca1fb3..019ce31 100644
--- a/crates/floresta-chain/src/pruned_utreexo/consensus.rs
+++ b/crates/floresta-chain/src/pruned_utreexo/consensus.rs
@@ -486,6 +486,7 @@ mod tests {
           )),
       );
   }
+
   /// Validates a relative timelock transaction and return its errors if any.
   ///
   /// utxo_lock: is the lock value which can be either a height or a timestamp. Defines the creation time/height of the UTXO.
@@ -607,17 +608,18 @@ mod tests {
       )
       .err()
   }
+
   #[test]
   fn test_timelock_validation() {
-        assert!(test_abstimelock(800, 800 + 1, 0).is_none()); //height locked sucess case.
+        assert!(test_abstimelock(800, 800 + 1, 0).is_none()); // height locked success case.
       assert_eq!(
-            test_abstimelock(800, 800 - 1, 0), //height locked fail case.
+            test_abstimelock(800, 800 - 1, 0), // height locked fail case.
           Some(BlockValidationErrors::BadAbsoluteLockTime)
       );

-        assert!(test_abstimelock(1358114045, 0, 1358114045 + 1).is_none()); //time locked sucess case
+        assert!(test_abstimelock(1358114045, 0, 1358114045 + 1).is_none()); // time locked success case
       assert_eq!(
-            test_abstimelock(1358114045, 0, 1358114045 - 1), //time locked fail case
+            test_abstimelock(1358114045, 0, 1358114045 - 1), // time locked fail case
           Some(BlockValidationErrors::BadAbsoluteLockTime)
       );

@@ -636,7 +638,7 @@ mod tests {
           Some(BlockValidationErrors::BadRelativeBlockLock)
       );

-        // relative time locked sucess case
+        // relative time locked success case
       let intervals_to_wait: u16 = 1;
       let sequence = Sequence::from_512_second_intervals(intervals_to_wait);
       assert!(test_reltimelock(
@@ -658,6 +660,7 @@ mod tests {
           Some(BlockValidationErrors::BadRelativeTimeLock)
       );
   }
+
   #[test]
   fn test_timelock_tx() {
       let utxo = UtxoData {
diff --git a/crates/floresta-chain/src/pruned_utreexo/mod.rs b/crates/floresta-chain/src/pruned_utreexo/mod.rs
index 0de7012..5b0d46e 100644
--- a/crates/floresta-chain/src/pruned_utreexo/mod.rs
+++ b/crates/floresta-chain/src/pruned_utreexo/mod.rs
@@ -366,6 +366,7 @@ impl<T: BlockchainInterface> BlockchainInterface for Arc<T> {
       T::get_fork_point(self, block)
   }
}
+
/// Module to delegate local-time context.
///
/// The consumer of `Floresta-chain` has the option to implement [`NodeTime`] if on a non-std environment.([`get_time()`] implementation that returns 0u32 will disable time checks.)
@@ -376,8 +377,10 @@ pub mod nodetime {
   ///
   /// Meant to be used in cases to disable time verifications
   pub struct DisableTime;
+    
   /// One Hour in seconds constant.
   pub const HOUR: u32 = 60 * 60;
+    
   /// Trait to return time-related context of the chain.
   ///
   /// [`get_time()`] should return a the latest [unix timestamp](https://en.wikipedia.org/wiki/Unix_time) when the consumer has time-notion.
@@ -387,12 +390,14 @@ pub mod nodetime {
       /// Should return a unix timestamp or 0 to skip any time related validation.
       fn get_time(&self) -> u32;
   }
+
   impl NodeTime for DisableTime {
       fn get_time(&self) -> u32 {
           // we simply return zero to disable time checks
           0
       }
   }
+
   #[cfg(feature = "std")]
   /// A module to provide the standard implementation of [`NodeTime`] trait. It uses [`std::time::SystemTime`] to get the current time.
   pub mod standard_node_time {
@@ -410,6 +415,7 @@ pub mod nodetime {
       }
   }
}
+
///Module to hold methods and structs related to UTXO data.
pub mod utxo_data {

[Davidson-Souza]

Looks like f082575 is doing it. Only need a rebase and there's two places where get_mtp reverted the formatting changes (empty line between items)

[JoseSK999]

Hi @jaoleal, the issue with the CI tests seems to be that the correct attribute is test, as pointed below. The tests were using the 'canonical' get_mtp because of this.

Actually the non_canonical feature is used only for ignoring the mtp for the benches right?

[jaoleal]

Hi @jaoleal, the issue with the CI tests seems to be that the correct attribute is test

It was my idea too, this was a broken PR that i had to upload just to move the code to another machine so i can test this branch.

Looks like that even if i set #[cfg(test)] it will not evaluate for sub crates. I didnt looked further but i tried more than once using exactly #[cfg(test)] and the only alternative that i found was setting the "non_canonical" feature for floresta-chain as a dev dependency.

[JoseSK999]

I mean using
#[cfg(any(feature = "non_canonical", test))] instead of
#[cfg(any(feature = "non_canonical", tests))]

[jaoleal]

I mean using #[cfg(any(feature = "non_canonical", test))] instead of #[cfg(any(feature = "non_canonical", tests))]

I know.

This resolves other tests but fail on others.

[JoseSK999]

Nice! You fixed the other issue by adding the dev dependency to wire (test attr still needed).

I will do a deeper review tomorrow but this seems almost ready.

[JoseSK999]

Actually there's a straightforward way of removing the non_canonical feature. The issue we have is that test and benches for verify_block_no_acc aren't designed to keep the previous 11 blocks for mtp, so let's instead move get_mtp outside of the function:

    pub fn validate_block_no_acc(
        &self,
        block: &Block,
        height: u32,
        inputs: UtxoMap,
        // NEW
        mtp: u32,
    ) -> Result<(), BlockchainError> {
        #[cfg(not(feature = "std"))]
        let time = super::nodetime::DisableTime;

        #[cfg(feature = "std")]
        let time = super::nodetime::standard_node_time::StdNodeTime;
        Consensus::validate_block_time(block.header.time, mtp, time)?;

        // Rest of the function

Then we call get_mtp inside validate_block and connect_block, just before validate_block_no_acc:

// Compute the Median Time Past of the 11 previous blocks
let mtp = self.get_mtp(height - 1)?;
self.validate_block_no_acc(block, height, inputs, mtp)

Then for the tests and benches we simply pass a mtp of 0, and fixed!

[JoseSK999]

Unresolved nit: remove this line

[JoseSK999]

Since we call get_mtp to validate the block it feels like we would already have all the hashes from this block and the previous 10. Or if the height is < 11, we would repeat the genesis time and that's all.

So I feel like the _ => { info! ... } shouldn't be there, and should be an error. Am I missing something?

[JoseSK999]

Nit: It should be comitted_height and comitted_time, and add a comment for each (in height vs in unix time)