From 482ea5c889b2c510dbf15710c21c15c0e06c00a0 Mon Sep 17 00:00:00 2001 From: Nick Santana Date: Mon, 20 Nov 2023 10:58:33 -0800 Subject: [PATCH 1/6] Add `timestamp` field to `Block` The `Block` now has a `timestamp` field. This field is meant to represent a consensed timestamp for the block that all the nodes will sign. Currently the timestamp is *not* being populated. The logic to properly set the timestamp is expected to come in a follow on PR. --- api/proto/blockchain.proto | 3 + api/src/convert/block.rs | 7 + blockchain/test-utils/src/lib.rs | 14 +- blockchain/types/src/block.rs | 140 +++++++++++++++++- consensus/enclave/impl/src/lib.rs | 3 +- consensus/enclave/mock/src/lib.rs | 9 +- .../src/byzantine_ledger/pending_values.rs | 27 ++-- .../service/src/byzantine_ledger/worker.rs | 4 +- fog/test_infra/src/db_tests.rs | 1 + fog/view/server/test-utils/src/lib.rs | 1 + fog/view/server/tests/unary_smoke_tests.rs | 6 + ledger/db/src/ledger_db.rs | 8 + ledger/db/src/test_utils/mod.rs | 8 +- .../src/ledger_sync/ledger_sync_service.rs | 1 + .../relayer/tests/block_processing.rs | 1 + light-client/verifier/src/verifier.rs | 6 + tools/local-network/local_network.py | 2 +- transaction/types/src/block_version.rs | 5 + util/generate-sample-ledger/src/lib.rs | 2 +- 19 files changed, 218 insertions(+), 30 deletions(-) diff --git a/api/proto/blockchain.proto b/api/proto/blockchain.proto index 205ceb4505..60e185bd86 100644 --- a/api/proto/blockchain.proto +++ b/api/proto/blockchain.proto @@ -43,6 +43,9 @@ message Block { // Hash of the block's contents. BlockContentsHash contents_hash = 7; + + // The block's timestamp. ms since Unix epoch + uint64 timestamp = 8; } message BlockContents { diff --git a/api/src/convert/block.rs b/api/src/convert/block.rs index 1194ee5f98..38ad008c50 100644 --- a/api/src/convert/block.rs +++ b/api/src/convert/block.rs @@ -17,6 +17,7 @@ impl From<&Block> for blockchain::Block { block.set_cumulative_txo_count(other.cumulative_txo_count); block.set_root_element((&other.root_element).into()); block.set_contents_hash(blockchain::BlockContentsHash::from(&other.contents_hash)); + block.set_timestamp(other.timestamp); block } } @@ -39,6 +40,7 @@ impl TryFrom<&blockchain::Block> for Block { cumulative_txo_count: value.cumulative_txo_count, root_element, contents_hash, + timestamp: value.timestamp, }; Ok(block) } @@ -65,6 +67,7 @@ mod tests { hash: TxOutMembershipHash::from([12u8; 32]), }, contents_hash: BlockContentsHash::try_from(&[66u8; 32][..]).unwrap(), + timestamp: 357, }; let block = blockchain::Block::from(&source_block); @@ -77,6 +80,7 @@ mod tests { assert_eq!(block.get_root_element().get_range().get_to(), 20); assert_eq!(block.get_root_element().get_hash().get_data(), &[12u8; 32]); assert_eq!(block.get_contents_hash().get_data(), [66u8; 32]); + assert_eq!(block.get_timestamp(), 357); } #[test] @@ -103,6 +107,7 @@ mod tests { source_block.set_index(2); source_block.set_root_element(root_element); source_block.set_contents_hash(contents_hash); + source_block.set_timestamp(411); let block = Block::try_from(&source_block).unwrap(); assert_eq!(block.id.as_ref(), [10u8; 32]); @@ -113,6 +118,7 @@ mod tests { assert_eq!(block.root_element.range.to, 20); assert_eq!(block.root_element.hash.as_ref(), &[13u8; 32]); assert_eq!(block.contents_hash.as_ref(), [66u8; 32]); + assert_eq!(block.timestamp, 411); } #[test] @@ -131,6 +137,7 @@ mod tests { hash: TxOutMembershipHash::from([12u8; 32]), }, contents_hash: BlockContentsHash::try_from(&[66u8; 32][..]).unwrap(), + timestamp: 911, }; // Encode using `protobuf`, decode using `prost`. diff --git a/blockchain/test-utils/src/lib.rs b/blockchain/test-utils/src/lib.rs index f877a0547a..3c3fb4f2b3 100644 --- a/blockchain/test-utils/src/lib.rs +++ b/blockchain/test-utils/src/lib.rs @@ -111,7 +111,18 @@ pub fn get_blocks_with_recipients( let block = match &prev_block { Some(parent) => { - Block::new_with_parent(block_version, parent, &Default::default(), &block_contents) + let timestamp = if block_version.timestamps_are_supported() { + parent.timestamp + 1 + } else { + 0 + }; + Block::new_with_parent( + block_version, + parent, + &Default::default(), + &block_contents, + timestamp, + ) } None => Block::new_origin_block(&block_contents.outputs), }; @@ -242,6 +253,7 @@ mod tests { block.cumulative_txo_count, &block.root_element, &block.contents_hash, + block.timestamp, ); assert_eq!(derived_block_id, block.id); diff --git a/blockchain/types/src/block.rs b/blockchain/types/src/block.rs index aa63d59bd9..5e6c7c96be 100644 --- a/blockchain/types/src/block.rs +++ b/blockchain/types/src/block.rs @@ -45,6 +45,11 @@ pub struct Block { /// Hash of the block's contents. #[prost(message, required, tag = "7")] pub contents_hash: BlockContentsHash, + + /// Timestamp of the block. ms since Unix epoch + #[prost(uint64, tag = "8")] + #[digestible(omit_when = 0)] + pub timestamp: u64, } impl Block { @@ -58,6 +63,7 @@ impl Block { let index: BlockIndex = 0; let cumulative_txo_count = outputs.len() as u64; let root_element = TxOutMembershipElement::default(); + let timestamp = 0; // The origin block does not contain anything but TxOuts. let block_contents = BlockContents { @@ -73,6 +79,7 @@ impl Block { cumulative_txo_count, &root_element, &contents_hash, + timestamp, ); Self { id, @@ -82,6 +89,7 @@ impl Block { cumulative_txo_count, root_element, contents_hash, + timestamp, } } @@ -100,6 +108,7 @@ impl Block { parent: &Block, root_element: &TxOutMembershipElement, block_contents: &BlockContents, + timestamp: u64, ) -> Self { Block::new( version, @@ -108,6 +117,7 @@ impl Block { parent.cumulative_txo_count + block_contents.outputs.len() as u64, root_element, block_contents, + timestamp, ) } @@ -123,6 +133,7 @@ impl Block { /// block* /// * `root_element` - The root element for membership proofs /// * `block_contents` - Contents of the block. + /// * `timestamp` - The timestamp of the block in ms. pub fn new( version: BlockVersion, parent_id: &BlockID, @@ -130,6 +141,7 @@ impl Block { cumulative_txo_count: u64, root_element: &TxOutMembershipElement, block_contents: &BlockContents, + timestamp: u64, ) -> Self { let contents_hash = block_contents.hash(); let id = compute_block_id( @@ -139,6 +151,7 @@ impl Block { cumulative_txo_count, root_element, &contents_hash, + timestamp, ); Self { @@ -149,6 +162,7 @@ impl Block { cumulative_txo_count, root_element: root_element.clone(), contents_hash, + timestamp, } } @@ -165,6 +179,7 @@ impl Block { self.cumulative_txo_count, &self.root_element, &self.contents_hash, + self.timestamp, ); self.id == expected_id @@ -182,6 +197,7 @@ pub fn compute_block_id( cumulative_txo_count: u64, root_element: &TxOutMembershipElement, contents_hash: &BlockContentsHash, + timestamp: u64, ) -> BlockID { let mut transcript = MerlinTranscript::new(b"mobilecoin-block-id"); @@ -192,6 +208,12 @@ pub fn compute_block_id( root_element.append_to_transcript(b"root_element", &mut transcript); contents_hash.append_to_transcript(b"contents_hash", &mut transcript); + let timestamps_supported = + BlockVersion::try_from(version).map(|v| v.timestamps_are_supported()); + if timestamps_supported.unwrap_or_default() { + timestamp.append_to_transcript(b"timestamp", &mut transcript); + } + let mut result = [0u8; 32]; transcript.extract_digest(&mut result); @@ -252,7 +274,7 @@ mod block_tests { (key_images, outputs) } - fn get_block(rng: &mut RNG) -> Block { + fn get_block_version_1(rng: &mut RNG) -> Block { let bytes = [14u8; 32]; let parent_id = BlockID::try_from(&bytes[..]).unwrap(); @@ -270,6 +292,29 @@ mod block_tests { 400, &root_element, &block_contents, + 0, // timestamp of 0 for earlier block versions + ) + } + + fn get_block_version_4(rng: &mut RNG) -> Block { + let bytes = [14u8; 32]; + let parent_id = BlockID::try_from(&bytes[..]).unwrap(); + + let root_element = TxOutMembershipElement { + range: Range::new(0, 15).unwrap(), + hash: TxOutMembershipHash::from([0u8; 32]), + }; + + let block_contents = get_block_contents(rng); + + Block::new( + BlockVersion::FOUR, + &parent_id, + 3, + 400, + &root_element, + &block_contents, + 10, ) } @@ -298,6 +343,7 @@ mod block_tests { 400, &root_element, &block_contents, + 0, ) } @@ -305,7 +351,7 @@ mod block_tests { /// The block returned by `get_block` should have a valid BlockID. fn test_get_block_has_valid_id() { let mut rng = get_seeded_rng(); - let block = get_block(&mut rng); + let block = get_block_version_1(&mut rng); assert!(block.is_block_id_valid()); } @@ -313,7 +359,7 @@ mod block_tests { /// The block ID should depend on the block version. fn test_block_id_includes_version() { let mut rng = get_seeded_rng(); - let mut block = get_block(&mut rng); + let mut block = get_block_version_1(&mut rng); block.version += 1; assert!(!block.is_block_id_valid()); } @@ -322,7 +368,7 @@ mod block_tests { /// The block ID should depend on the parent_id. fn test_block_id_includes_parent_id() { let mut rng = get_seeded_rng(); - let mut block = get_block(&mut rng); + let mut block = get_block_version_1(&mut rng); let mut bytes = [0u8; 32]; rng.fill_bytes(&mut bytes); @@ -336,7 +382,7 @@ mod block_tests { /// The block ID should depend on the block's index. fn test_block_id_includes_block_index() { let mut rng = get_seeded_rng(); - let mut block = get_block(&mut rng); + let mut block = get_block_version_1(&mut rng); block.index += 1; assert!(!block.is_block_id_valid()); } @@ -345,7 +391,7 @@ mod block_tests { /// The block ID should depend on the root element. fn test_block_id_includes_root_element() { let mut rng = get_seeded_rng(); - let mut block = get_block(&mut rng); + let mut block = get_block_version_1(&mut rng); let wrong_root_element = TxOutMembershipElement { range: Range::new(13, 17).unwrap(), @@ -359,7 +405,7 @@ mod block_tests { /// The block ID should depend on the content_hash. fn test_block_id_includes_content_hash() { let mut rng = get_seeded_rng(); - let mut block = get_block(&mut rng); + let mut block = get_block_version_1(&mut rng); let mut bytes = [0u8; 32]; rng.fill_bytes(&mut bytes); @@ -386,7 +432,7 @@ mod block_tests { let mut rng = get_seeded_rng(); //Check hash with memo - let block = get_block(&mut rng); + let block = get_block_version_1(&mut rng); assert_eq!( block.id.as_ref(), &[ @@ -422,4 +468,82 @@ mod block_tests { ] ); } + #[test] + /// The block ID hash do not change as the code evolves. + /// This test was written by writing a failed assert and then copying the + /// actual block id into the test. This should hopefully catches cases where + /// we add/change Block/BlockContents and accidentally break id + /// calculation of old blocks. + fn test_hashing_is_consistent_block_version_four() { + let mut rng = get_seeded_rng(); + + let block = get_block_version_4(&mut rng); + assert_eq!( + block.id.as_ref(), + &[ + 156, 155, 244, 98, 84, 234, 204, 146, 224, 142, 236, 197, 11, 69, 5, 74, 109, 160, + 123, 173, 206, 100, 224, 171, 72, 35, 208, 137, 150, 168, 43, 93 + ] + ); + } + + #[test] + fn test_block_version_3_ignores_timestamp_in_id() { + let parent_id = BlockID::try_from(&[1u8; 32][..]).unwrap(); + let index = 1; + let cumulative_txo_count = 1; + let root_element = TxOutMembershipElement::default(); + let contents_hash = BlockContentsHash::default(); + let timestamp_1 = 1; + let id_1 = compute_block_id( + 3, + &parent_id, + index, + cumulative_txo_count, + &root_element, + &contents_hash, + timestamp_1, + ); + let timestamp_2 = 2; + let id_2 = compute_block_id( + 3, + &parent_id, + index, + cumulative_txo_count, + &root_element, + &contents_hash, + timestamp_2, + ); + assert_eq!(id_1, id_2); + } + + #[test] + fn test_block_version_4_takes_timestamp_into_account() { + let parent_id = BlockID::try_from(&[1u8; 32][..]).unwrap(); + let index = 1; + let cumulative_txo_count = 1; + let root_element = TxOutMembershipElement::default(); + let contents_hash = BlockContentsHash::default(); + let timestamp_1 = 1; + let id_1 = compute_block_id( + 4, + &parent_id, + index, + cumulative_txo_count, + &root_element, + &contents_hash, + timestamp_1, + ); + let timestamp_2 = 2; + let id_2 = compute_block_id( + 4, + &parent_id, + index, + cumulative_txo_count, + &root_element, + &contents_hash, + timestamp_2, + ); + assert_ne!(id_1, id_2); + } } diff --git a/consensus/enclave/impl/src/lib.rs b/consensus/enclave/impl/src/lib.rs index fc1b9695f8..8b99377065 100644 --- a/consensus/enclave/impl/src/lib.rs +++ b/consensus/enclave/impl/src/lib.rs @@ -937,13 +937,14 @@ impl ConsensusEnclave for SgxConsensusEnclave { validated_mint_config_txs, mint_txs, }; - // + // Form the block. let block = Block::new_with_parent( config.block_version, parent_block, root_element, &block_contents, + 0, ); // Sign the block. diff --git a/consensus/enclave/mock/src/lib.rs b/consensus/enclave/mock/src/lib.rs index 63417a5f35..e1ea7434b7 100644 --- a/consensus/enclave/mock/src/lib.rs +++ b/consensus/enclave/mock/src/lib.rs @@ -350,8 +350,13 @@ impl ConsensusEnclave for ConsensusServiceMockEnclave { validated_mint_config_txs, }; - let block = - Block::new_with_parent(block_version, parent_block, root_element, &block_contents); + let block = Block::new_with_parent( + block_version, + parent_block, + root_element, + &block_contents, + parent_block.timestamp + 1, + ); let signature = BlockSignature::from_block_and_keypair(&block, &self.signing_keypair)?; diff --git a/consensus/service/src/byzantine_ledger/pending_values.rs b/consensus/service/src/byzantine_ledger/pending_values.rs index a518ad0039..45e0aee5eb 100644 --- a/consensus/service/src/byzantine_ledger/pending_values.rs +++ b/consensus/service/src/byzantine_ledger/pending_values.rs @@ -22,15 +22,16 @@ pub struct PendingValues { /// We need to store pending values vec so we can process values /// on a first-come first-served basis. However, we want to be able to: - /// 1) Efficiently see if we already have a given transaction and ignore - /// duplicates 2) Track how long each transaction took to externalize. + /// 1. Efficiently see if we already have a given transaction and ignore + /// duplicates + /// 2. Track how long each transaction took to externalize. /// /// To accomplish these goals we store, in addition to the queue of pending /// values, a map that maps a value to when we first encountered it. /// This essentially gives us an ordered HashMap. /// - /// Note that we only store a timestamp for values that were handed to us - /// directly from a client. That behavior is enforced by + /// Note that we only store a received time for values that were handed to + /// us directly from a client. That behavior is enforced by /// ByzantineLedger. We skip tracking processing times for relayed /// values since we want to track the time from when the network first /// saw a value, and not when a specific node saw it. @@ -65,17 +66,17 @@ impl PendingValues { self.pending_values.len() } - /// Try and add a pending value, associated with a given timestamp, to the - /// list. Returns `true` if the value is valid and not already on the - /// list, false otherwise. - pub fn push(&mut self, value: ConsensusValue, timestamp: Option) -> bool { + /// Try and add a pending value, associated with the time the value was + /// received at, to the list. Returns `true` if the value is valid and + /// not already on the list, false otherwise. + pub fn push(&mut self, value: ConsensusValue, received_time: Option) -> bool { if let Vacant(entry) = self.pending_values_map.entry(value.clone()) { match value { ConsensusValue::TxHash(tx_hash) => { // A new transaction. if self.tx_manager.validate(&tx_hash).is_ok() { // The transaction is well-formed and valid. - entry.insert(timestamp); + entry.insert(received_time); self.pending_values.push(value); true } else { @@ -90,7 +91,7 @@ impl PendingValues { .is_ok() { // The transaction is well-formed and valid. - entry.insert(timestamp); + entry.insert(received_time); self.pending_values.push(value); true } else { @@ -101,7 +102,7 @@ impl PendingValues { ConsensusValue::MintTx(ref mint_tx) => { if self.mint_tx_manager.validate_mint_tx(mint_tx).is_ok() { // The transaction is well-formed and valid. - entry.insert(timestamp); + entry.insert(received_time); self.pending_values.push(value); true } else { @@ -119,8 +120,8 @@ impl PendingValues { self.pending_values.iter() } - /// Try and get the timestamp associated with a given value. - pub fn get_timestamp_for_value(&self, tx_hash: &ConsensusValue) -> Option { + /// Try and get the received time associated with a given value. + pub fn get_received_time_for_value(&self, tx_hash: &ConsensusValue) -> Option { self.pending_values_map.get(tx_hash).cloned().flatten() } diff --git a/consensus/service/src/byzantine_ledger/worker.rs b/consensus/service/src/byzantine_ledger/worker.rs index e525ffee2a..06de21323f 100644 --- a/consensus/service/src/byzantine_ledger/worker.rs +++ b/consensus/service/src/byzantine_ledger/worker.rs @@ -546,8 +546,8 @@ impl< // Update pending value processing time metrics. for value in externalized.iter() { - if let Some(timestamp) = self.pending_values.get_timestamp_for_value(value) { - let duration = Instant::now().saturating_duration_since(timestamp); + if let Some(received_time) = self.pending_values.get_received_time_for_value(value) { + let duration = Instant::now().saturating_duration_since(received_time); counters::PENDING_VALUE_PROCESSING_TIME.observe(duration.as_secs_f64()); } } diff --git a/fog/test_infra/src/db_tests.rs b/fog/test_infra/src/db_tests.rs index d750533a0e..3cc29eb075 100644 --- a/fog/test_infra/src/db_tests.rs +++ b/fog/test_infra/src/db_tests.rs @@ -675,6 +675,7 @@ pub fn random_block( 0, &Default::default(), &Default::default(), + 0, ); let test_rows: Vec = (0..num_txs).map(|_| random_tx_row(rng)).collect(); (block, test_rows) diff --git a/fog/view/server/test-utils/src/lib.rs b/fog/view/server/test-utils/src/lib.rs index cb88374685..2911e179a5 100644 --- a/fog/view/server/test-utils/src/lib.rs +++ b/fog/view/server/test-utils/src/lib.rs @@ -357,6 +357,7 @@ pub fn add_block_data( cumulative_tx_out_count, &Default::default(), &Default::default(), + 0, ), 0, txs, diff --git a/fog/view/server/tests/unary_smoke_tests.rs b/fog/view/server/tests/unary_smoke_tests.rs index 3c87ff0488..9a12111d0f 100644 --- a/fog/view/server/tests/unary_smoke_tests.rs +++ b/fog/view/server/tests/unary_smoke_tests.rs @@ -75,6 +75,7 @@ fn test_view_integration(view_omap_capacity: u64, store_count: usize, blocks_per 2, &Default::default(), &Default::default(), + 0, ), 0, &txs[..2], @@ -90,6 +91,7 @@ fn test_view_integration(view_omap_capacity: u64, store_count: usize, blocks_per 6, &Default::default(), &Default::default(), + 0, ), 0, &txs[2..6], @@ -113,6 +115,7 @@ fn test_view_integration(view_omap_capacity: u64, store_count: usize, blocks_per 12, &Default::default(), &Default::default(), + 0, ), 0, &txs[6..12], @@ -148,6 +151,7 @@ fn test_view_integration(view_omap_capacity: u64, store_count: usize, blocks_per 12, &Default::default(), &Default::default(), + 0, ), 0, &[], @@ -163,6 +167,7 @@ fn test_view_integration(view_omap_capacity: u64, store_count: usize, blocks_per 16, &Default::default(), &Default::default(), + 0, ), 0, &txs[12..16], @@ -180,6 +185,7 @@ fn test_view_integration(view_omap_capacity: u64, store_count: usize, blocks_per 20, &Default::default(), &Default::default(), + 0, ), 0, &txs[16..20], diff --git a/ledger/db/src/ledger_db.rs b/ledger/db/src/ledger_db.rs index b6172a64dd..c8986dc5d4 100644 --- a/ledger/db/src/ledger_db.rs +++ b/ledger/db/src/ledger_db.rs @@ -2100,6 +2100,7 @@ mod ledger_db_test { origin.block(), &Default::default(), &block_contents, + 1, ); assert_eq!( @@ -2400,6 +2401,7 @@ mod ledger_db_test { &last_block, &Default::default(), &block_contents, + last_block.timestamp + 1, ); assert_eq!( ledger_db.append_block(&invalid_block, &block_contents, None, None), @@ -2412,6 +2414,7 @@ mod ledger_db_test { &last_block, &Default::default(), &block_contents, + last_block.timestamp + 1, ); assert_eq!( ledger_db.append_block(&invalid_block, &block_contents, None, None), @@ -2450,6 +2453,7 @@ mod ledger_db_test { &last_block, &Default::default(), &block_contents, + last_block.timestamp + 1, ); let metadata = make_block_metadata(last_block.id.clone(), &mut rng); @@ -2504,6 +2508,7 @@ mod ledger_db_test { origin.cumulative_txo_count, &Default::default(), &block_contents, + 1, ); assert_eq!( ledger_db.append_block(&new_block, &block_contents, None, None), @@ -2584,6 +2589,7 @@ mod ledger_db_test { origin.block(), &Default::default(), &block_one_contents, + origin.block().timestamp + 1, ); assert_eq!( @@ -2645,6 +2651,7 @@ mod ledger_db_test { origin.block().cumulative_txo_count, &Default::default(), &block_contents, + origin.block().timestamp + 1, ); assert_eq!( @@ -2660,6 +2667,7 @@ mod ledger_db_test { origin.block().cumulative_txo_count, &Default::default(), &block_contents, + origin.block().timestamp + 1, ); assert_eq!( diff --git a/ledger/db/src/test_utils/mod.rs b/ledger/db/src/test_utils/mod.rs index c11846f920..c365a412d8 100644 --- a/ledger/db/src/test_utils/mod.rs +++ b/ledger/db/src/test_utils/mod.rs @@ -429,7 +429,13 @@ pub fn add_block_contents_to_ledger( let new_block = if num_blocks > 0 { let parent = ledger_db.get_block(num_blocks - 1)?; - Block::new_with_parent(block_version, &parent, &Default::default(), &block_contents) + Block::new_with_parent( + block_version, + &parent, + &Default::default(), + &block_contents, + parent.timestamp + 1, + ) } else { Block::new_origin_block(&block_contents.outputs) }; diff --git a/ledger/sync/src/ledger_sync/ledger_sync_service.rs b/ledger/sync/src/ledger_sync/ledger_sync_service.rs index 6f2b12f9fe..8a772fb450 100644 --- a/ledger/sync/src/ledger_sync/ledger_sync_service.rs +++ b/ledger/sync/src/ledger_sync/ledger_sync_service.rs @@ -831,6 +831,7 @@ pub fn identify_safe_blocks( block.cumulative_txo_count, &block.root_element, &block_contents.hash(), + block.timestamp, ); if block.id != derived_block_id { log::error!( diff --git a/light-client/relayer/tests/block_processing.rs b/light-client/relayer/tests/block_processing.rs index 95c8abcc41..7be4924269 100644 --- a/light-client/relayer/tests/block_processing.rs +++ b/light-client/relayer/tests/block_processing.rs @@ -45,6 +45,7 @@ fn append_tx_outs_as_block( &last_block, &Default::default(), &block_contents, + last_block.timestamp + 1, ); // Sign this block with all the signer identities diff --git a/light-client/verifier/src/verifier.rs b/light-client/verifier/src/verifier.rs index 5aa01ea5e9..ec31c33f8f 100644 --- a/light-client/verifier/src/verifier.rs +++ b/light-client/verifier/src/verifier.rs @@ -200,6 +200,7 @@ mod tests { 88, &Default::default(), &Default::default(), + 88, ); let lcv = get_light_client_verifier(BTreeSet::from([block88.id.clone()])); @@ -211,6 +212,7 @@ mod tests { 99, &Default::default(), &Default::default(), + 99, ); let block9999 = Block::new( Default::default(), @@ -219,6 +221,7 @@ mod tests { 9999, &Default::default(), &Default::default(), + 9999, ); let block99999 = Block::new( Default::default(), @@ -227,6 +230,7 @@ mod tests { 99999, &Default::default(), &Default::default(), + 99999, ); // Block 88 verifies even without any signatures, because we put it in the @@ -365,6 +369,7 @@ mod tests { 9999, &Default::default(), &bc, + 9999, ); let metadata = sign_block_id_for_test_node_ids(&block9999.id, &[1, 2, 3]); @@ -459,6 +464,7 @@ mod tests { 9999, &Default::default(), &bc, + 9999, ); let metadata = sign_block_id_for_test_node_ids(&block9999.id, &[1, 2, 3]); diff --git a/tools/local-network/local_network.py b/tools/local-network/local_network.py index f42097e08b..e9e311729e 100755 --- a/tools/local-network/local_network.py +++ b/tools/local-network/local_network.py @@ -162,7 +162,7 @@ def __init__(self, name, node_num, client_port, peer_port, admin_port, admin_htt self.peers = peers self.quorum_set = quorum_set self.minimum_fee = 400_000_000 - self.block_version = block_version or 3 + self.block_version = block_version or 4 self.consensus_process = None self.ledger_distribution_process = None diff --git a/transaction/types/src/block_version.rs b/transaction/types/src/block_version.rs index 0277aa8b1b..d3ba49de56 100644 --- a/transaction/types/src/block_version.rs +++ b/transaction/types/src/block_version.rs @@ -154,6 +154,11 @@ impl BlockVersion { pub fn nested_multisigs_are_supported(&self) -> bool { self >= &Self::THREE } + + /// Timestamps are supported starting from v4. + pub fn timestamps_are_supported(&self) -> bool { + self >= &Self::FOUR + } } impl Deref for BlockVersion { diff --git a/util/generate-sample-ledger/src/lib.rs b/util/generate-sample-ledger/src/lib.rs index 868b67f96c..bafd2982da 100644 --- a/util/generate-sample-ledger/src/lib.rs +++ b/util/generate-sample-ledger/src/lib.rs @@ -65,7 +65,7 @@ pub fn bootstrap_ledger( ); let block_version = if max_token_id > 0 { - BlockVersion::THREE + BlockVersion::MAX } else { // This is historically the version created by bootstrap BlockVersion::ZERO From d59b7d8cbba186734f0def504013709f31e96e45 Mon Sep 17 00:00:00 2001 From: Nick Santana Date: Tue, 21 Nov 2023 10:01:43 -0800 Subject: [PATCH 2/6] Prevent timestamp other than 0 for unsupported block versions Previously one could set the `Block.timestamp` to something other than 0 when the block version didn't support timestamps. Now the timestamp will be set to 0 if the block version doesn't support timestamps --- api/src/convert/block.rs | 16 ++++++++++++++-- blockchain/test-utils/src/lib.rs | 7 +------ blockchain/types/src/block.rs | 26 ++++++++++++++++++++++++-- 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/api/src/convert/block.rs b/api/src/convert/block.rs index 38ad008c50..a9d922c31b 100644 --- a/api/src/convert/block.rs +++ b/api/src/convert/block.rs @@ -17,7 +17,13 @@ impl From<&Block> for blockchain::Block { block.set_cumulative_txo_count(other.cumulative_txo_count); block.set_root_element((&other.root_element).into()); block.set_contents_hash(blockchain::BlockContentsHash::from(&other.contents_hash)); - block.set_timestamp(other.timestamp); + + // It should be 0 if timestamps are not supported, but being a + // publicly accessible field means anyone could set it, so we guard + // against that here in the conversion. + if block.version.timestamps_are_supported() { + block.set_timestamp(other.timestamp); + } block } } @@ -32,6 +38,12 @@ impl TryFrom<&blockchain::Block> for Block { let root_element = TxOutMembershipElement::try_from(value.get_root_element())?; let contents_hash = BlockContentsHash::try_from(value.get_contents_hash())?; + let timestamp = if value.get_version().timestamps_are_supported() { + value.get_timestamp() + } else { + 0 + }; + let block = Block { id: block_id, version: value.version, @@ -40,7 +52,7 @@ impl TryFrom<&blockchain::Block> for Block { cumulative_txo_count: value.cumulative_txo_count, root_element, contents_hash, - timestamp: value.timestamp, + timestamp, }; Ok(block) } diff --git a/blockchain/test-utils/src/lib.rs b/blockchain/test-utils/src/lib.rs index 3c3fb4f2b3..966b03f688 100644 --- a/blockchain/test-utils/src/lib.rs +++ b/blockchain/test-utils/src/lib.rs @@ -111,17 +111,12 @@ pub fn get_blocks_with_recipients( let block = match &prev_block { Some(parent) => { - let timestamp = if block_version.timestamps_are_supported() { - parent.timestamp + 1 - } else { - 0 - }; Block::new_with_parent( block_version, parent, &Default::default(), &block_contents, - timestamp, + parent.timestamp + 1, ) } None => Block::new_origin_block(&block_contents.outputs), diff --git a/blockchain/types/src/block.rs b/blockchain/types/src/block.rs index 5e6c7c96be..96bedbfe45 100644 --- a/blockchain/types/src/block.rs +++ b/blockchain/types/src/block.rs @@ -103,6 +103,8 @@ impl Block { /// * `parent` - The parent block /// * `root_element` - The root element for membership proofs /// * `block_contents - The Contents of the block. + /// * `timestamp` - The timestamp of the block in ms since Unix epoch. Only + /// supported for BlockVersion 4 and higher. pub fn new_with_parent( version: BlockVersion, parent: &Block, @@ -117,6 +119,7 @@ impl Block { parent.cumulative_txo_count + block_contents.outputs.len() as u64, root_element, block_contents, + // timestamp is normalized in Block::new() timestamp, ) } @@ -133,7 +136,8 @@ impl Block { /// block* /// * `root_element` - The root element for membership proofs /// * `block_contents` - Contents of the block. - /// * `timestamp` - The timestamp of the block in ms. + /// * `timestamp` - The timestamp of the block in ms since Unix epoch. Only + /// supported for BlockVersion 4 and higher. pub fn new( version: BlockVersion, parent_id: &BlockID, @@ -143,6 +147,13 @@ impl Block { block_contents: &BlockContents, timestamp: u64, ) -> Self { + + let timestamp = if version.timestamps_are_supported() { + timestamp + } else { + 0 + }; + let contents_hash = block_contents.hash(); let id = compute_block_id( *version, @@ -488,7 +499,7 @@ mod block_tests { } #[test] - fn test_block_version_3_ignores_timestamp_in_id() { + fn test_block_version_3_ignores_timestamp() { let parent_id = BlockID::try_from(&[1u8; 32][..]).unwrap(); let index = 1; let cumulative_txo_count = 1; @@ -515,6 +526,17 @@ mod block_tests { timestamp_2, ); assert_eq!(id_1, id_2); + + let block = Block::new( + BlockVersion::THREE, + &parent_id, + index, + cumulative_txo_count, + &root_element, + &BlockContents::default(), + timestamp_1, + ); + assert_eq!(block.timestamp, 0); } #[test] From bbdd021b48d56e77a4469ef8b03760abcc008bff Mon Sep 17 00:00:00 2001 From: Nick Santana Date: Tue, 21 Nov 2023 11:02:02 -0800 Subject: [PATCH 3/6] Turn check for 0 timestamp in Block::new() into assert --- api/src/convert/block.rs | 16 ++-------------- blockchain/test-utils/src/lib.rs | 7 ++++++- blockchain/types/src/block.rs | 31 +++++++------------------------ ledger/db/src/test_utils/mod.rs | 8 +++++++- 4 files changed, 22 insertions(+), 40 deletions(-) diff --git a/api/src/convert/block.rs b/api/src/convert/block.rs index a9d922c31b..38ad008c50 100644 --- a/api/src/convert/block.rs +++ b/api/src/convert/block.rs @@ -17,13 +17,7 @@ impl From<&Block> for blockchain::Block { block.set_cumulative_txo_count(other.cumulative_txo_count); block.set_root_element((&other.root_element).into()); block.set_contents_hash(blockchain::BlockContentsHash::from(&other.contents_hash)); - - // It should be 0 if timestamps are not supported, but being a - // publicly accessible field means anyone could set it, so we guard - // against that here in the conversion. - if block.version.timestamps_are_supported() { - block.set_timestamp(other.timestamp); - } + block.set_timestamp(other.timestamp); block } } @@ -38,12 +32,6 @@ impl TryFrom<&blockchain::Block> for Block { let root_element = TxOutMembershipElement::try_from(value.get_root_element())?; let contents_hash = BlockContentsHash::try_from(value.get_contents_hash())?; - let timestamp = if value.get_version().timestamps_are_supported() { - value.get_timestamp() - } else { - 0 - }; - let block = Block { id: block_id, version: value.version, @@ -52,7 +40,7 @@ impl TryFrom<&blockchain::Block> for Block { cumulative_txo_count: value.cumulative_txo_count, root_element, contents_hash, - timestamp, + timestamp: value.timestamp, }; Ok(block) } diff --git a/blockchain/test-utils/src/lib.rs b/blockchain/test-utils/src/lib.rs index 966b03f688..3c3fb4f2b3 100644 --- a/blockchain/test-utils/src/lib.rs +++ b/blockchain/test-utils/src/lib.rs @@ -111,12 +111,17 @@ pub fn get_blocks_with_recipients( let block = match &prev_block { Some(parent) => { + let timestamp = if block_version.timestamps_are_supported() { + parent.timestamp + 1 + } else { + 0 + }; Block::new_with_parent( block_version, parent, &Default::default(), &block_contents, - parent.timestamp + 1, + timestamp, ) } None => Block::new_origin_block(&block_contents.outputs), diff --git a/blockchain/types/src/block.rs b/blockchain/types/src/block.rs index 96bedbfe45..e4b519fcf7 100644 --- a/blockchain/types/src/block.rs +++ b/blockchain/types/src/block.rs @@ -102,9 +102,9 @@ impl Block { /// * `version` - The block format version /// * `parent` - The parent block /// * `root_element` - The root element for membership proofs - /// * `block_contents - The Contents of the block. - /// * `timestamp` - The timestamp of the block in ms since Unix epoch. Only - /// supported for BlockVersion 4 and higher. + /// * `block_contents` - The Contents of the block. + /// * `timestamp` - The timestamp of the block in ms since Unix epoch. + /// should be 0 for block versions 3 and below. pub fn new_with_parent( version: BlockVersion, parent: &Block, @@ -119,7 +119,6 @@ impl Block { parent.cumulative_txo_count + block_contents.outputs.len() as u64, root_element, block_contents, - // timestamp is normalized in Block::new() timestamp, ) } @@ -136,8 +135,8 @@ impl Block { /// block* /// * `root_element` - The root element for membership proofs /// * `block_contents` - Contents of the block. - /// * `timestamp` - The timestamp of the block in ms since Unix epoch. Only - /// supported for BlockVersion 4 and higher. + /// * `timestamp` - The timestamp of the block in ms since Unix epoch. + /// should be 0 for block versions 3 and below. pub fn new( version: BlockVersion, parent_id: &BlockID, @@ -147,12 +146,7 @@ impl Block { block_contents: &BlockContents, timestamp: u64, ) -> Self { - - let timestamp = if version.timestamps_are_supported() { - timestamp - } else { - 0 - }; + assert!(timestamp == 0 || version.timestamps_are_supported()); let contents_hash = block_contents.hash(); let id = compute_block_id( @@ -499,7 +493,7 @@ mod block_tests { } #[test] - fn test_block_version_3_ignores_timestamp() { + fn test_block_version_3_ignores_timestamp_in_id() { let parent_id = BlockID::try_from(&[1u8; 32][..]).unwrap(); let index = 1; let cumulative_txo_count = 1; @@ -526,17 +520,6 @@ mod block_tests { timestamp_2, ); assert_eq!(id_1, id_2); - - let block = Block::new( - BlockVersion::THREE, - &parent_id, - index, - cumulative_txo_count, - &root_element, - &BlockContents::default(), - timestamp_1, - ); - assert_eq!(block.timestamp, 0); } #[test] diff --git a/ledger/db/src/test_utils/mod.rs b/ledger/db/src/test_utils/mod.rs index c365a412d8..75c998bdca 100644 --- a/ledger/db/src/test_utils/mod.rs +++ b/ledger/db/src/test_utils/mod.rs @@ -429,12 +429,18 @@ pub fn add_block_contents_to_ledger( let new_block = if num_blocks > 0 { let parent = ledger_db.get_block(num_blocks - 1)?; + let timestamp = if block_version.timestamps_are_supported() { + parent.timestamp + 1 + } else { + 0 + }; + Block::new_with_parent( block_version, &parent, &Default::default(), &block_contents, - parent.timestamp + 1, + timestamp, ) } else { Block::new_origin_block(&block_contents.outputs) From 405b0478bcd8701e072bdc9a3d8bd568b783b67c Mon Sep 17 00:00:00 2001 From: Nick Santana Date: Tue, 21 Nov 2023 12:35:57 -0800 Subject: [PATCH 4/6] Require Block timestamp to be 0 or non zero and block version 4 --- blockchain/types/src/block.rs | 4 +-- consensus/enclave/mock/src/lib.rs | 8 +++++- ledger/db/src/ledger_db.rs | 41 ++++++++++++++++++++------- light-client/verifier/src/verifier.rs | 12 ++++---- 4 files changed, 45 insertions(+), 20 deletions(-) diff --git a/blockchain/types/src/block.rs b/blockchain/types/src/block.rs index e4b519fcf7..d332b0de73 100644 --- a/blockchain/types/src/block.rs +++ b/blockchain/types/src/block.rs @@ -136,7 +136,7 @@ impl Block { /// * `root_element` - The root element for membership proofs /// * `block_contents` - Contents of the block. /// * `timestamp` - The timestamp of the block in ms since Unix epoch. - /// should be 0 for block versions 3 and below. + /// should be 0 for block versions 3 and below, and set for block versions 4 and above. pub fn new( version: BlockVersion, parent_id: &BlockID, @@ -146,7 +146,7 @@ impl Block { block_contents: &BlockContents, timestamp: u64, ) -> Self { - assert!(timestamp == 0 || version.timestamps_are_supported()); + assert!((timestamp == 0) ^ version.timestamps_are_supported()); let contents_hash = block_contents.hash(); let id = compute_block_id( diff --git a/consensus/enclave/mock/src/lib.rs b/consensus/enclave/mock/src/lib.rs index e1ea7434b7..44c59da9d9 100644 --- a/consensus/enclave/mock/src/lib.rs +++ b/consensus/enclave/mock/src/lib.rs @@ -350,12 +350,18 @@ impl ConsensusEnclave for ConsensusServiceMockEnclave { validated_mint_config_txs, }; + let timestamp = if block_version.timestamps_are_supported() { + parent_block.timestamp + 1 + } else { + 0 + }; + let block = Block::new_with_parent( block_version, parent_block, root_element, &block_contents, - parent_block.timestamp + 1, + timestamp, ); let signature = BlockSignature::from_block_and_keypair(&block, &self.signing_keypair)?; diff --git a/ledger/db/src/ledger_db.rs b/ledger/db/src/ledger_db.rs index c8986dc5d4..e39aec1cae 100644 --- a/ledger/db/src/ledger_db.rs +++ b/ledger/db/src/ledger_db.rs @@ -2100,7 +2100,7 @@ mod ledger_db_test { origin.block(), &Default::default(), &block_contents, - 1, + 0, ); assert_eq!( @@ -2335,7 +2335,7 @@ mod ledger_db_test { } #[test] - /// Appending blocks that have ever-increasing and continous version numbers + /// Appending blocks that have ever-increasing and continuous version numbers /// should work as long as it is <= MAX_BLOCK_VERSION. /// Appending a block > MAX_BLOCK_VERSION should fail even if it is after a /// block with version == MAX_BLOCK_VERSION. @@ -2396,12 +2396,19 @@ mod ledger_db_test { // Note: unsafe transmute is being used to skirt the invariant that BlockVersion // does not exceed MAX_BLOCK_VERSION + let version: BlockVersion = unsafe { core::mem::transmute(last_block.version + 1) }; + let timestamp = if version.timestamps_are_supported() { + last_block.timestamp + 1 + } else { + 0 + }; + let invalid_block = Block::new_with_parent( - unsafe { core::mem::transmute(last_block.version + 1) }, + version, &last_block, &Default::default(), &block_contents, - last_block.timestamp + 1, + timestamp, ); assert_eq!( ledger_db.append_block(&invalid_block, &block_contents, None, None), @@ -2409,12 +2416,19 @@ mod ledger_db_test { ); if last_block.version > 0 { + let version = BlockVersion::try_from(last_block.version - 1).unwrap(); + let timestamp = if version.timestamps_are_supported() { + last_block.timestamp + 1 + } else { + 0 + }; + let invalid_block = Block::new_with_parent( - BlockVersion::try_from(last_block.version - 1).unwrap(), + version, &last_block, &Default::default(), &block_contents, - last_block.timestamp + 1, + timestamp, ); assert_eq!( ledger_db.append_block(&invalid_block, &block_contents, None, None), @@ -2448,12 +2462,17 @@ mod ledger_db_test { outputs, ..Default::default() }; + let timestamp = if block_version.timestamps_are_supported() { + last_block.timestamp + 1 + } else { + 0 + }; last_block = Block::new_with_parent( block_version, &last_block, &Default::default(), &block_contents, - last_block.timestamp + 1, + timestamp, ); let metadata = make_block_metadata(last_block.id.clone(), &mut rng); @@ -2508,7 +2527,7 @@ mod ledger_db_test { origin.cumulative_txo_count, &Default::default(), &block_contents, - 1, + 0, ); assert_eq!( ledger_db.append_block(&new_block, &block_contents, None, None), @@ -2589,7 +2608,7 @@ mod ledger_db_test { origin.block(), &Default::default(), &block_one_contents, - origin.block().timestamp + 1, + 0, ); assert_eq!( @@ -2651,7 +2670,7 @@ mod ledger_db_test { origin.block().cumulative_txo_count, &Default::default(), &block_contents, - origin.block().timestamp + 1, + 0, ); assert_eq!( @@ -2667,7 +2686,7 @@ mod ledger_db_test { origin.block().cumulative_txo_count, &Default::default(), &block_contents, - origin.block().timestamp + 1, + 0, ); assert_eq!( diff --git a/light-client/verifier/src/verifier.rs b/light-client/verifier/src/verifier.rs index ec31c33f8f..ac0870a304 100644 --- a/light-client/verifier/src/verifier.rs +++ b/light-client/verifier/src/verifier.rs @@ -200,7 +200,7 @@ mod tests { 88, &Default::default(), &Default::default(), - 88, + 0, ); let lcv = get_light_client_verifier(BTreeSet::from([block88.id.clone()])); @@ -212,7 +212,7 @@ mod tests { 99, &Default::default(), &Default::default(), - 99, + 0, ); let block9999 = Block::new( Default::default(), @@ -221,7 +221,7 @@ mod tests { 9999, &Default::default(), &Default::default(), - 9999, + 0, ); let block99999 = Block::new( Default::default(), @@ -230,7 +230,7 @@ mod tests { 99999, &Default::default(), &Default::default(), - 99999, + 0, ); // Block 88 verifies even without any signatures, because we put it in the @@ -369,7 +369,7 @@ mod tests { 9999, &Default::default(), &bc, - 9999, + 0, ); let metadata = sign_block_id_for_test_node_ids(&block9999.id, &[1, 2, 3]); @@ -464,7 +464,7 @@ mod tests { 9999, &Default::default(), &bc, - 9999, + 0, ); let metadata = sign_block_id_for_test_node_ids(&block9999.id, &[1, 2, 3]); From 0296b951ac278c389d6dea6f793aabb8d7fe035f Mon Sep 17 00:00:00 2001 From: Nick Santana Date: Tue, 21 Nov 2023 13:37:59 -0800 Subject: [PATCH 5/6] Fixed lint errors --- blockchain/types/src/block.rs | 3 ++- ledger/db/src/ledger_db.rs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/blockchain/types/src/block.rs b/blockchain/types/src/block.rs index d332b0de73..59eb391847 100644 --- a/blockchain/types/src/block.rs +++ b/blockchain/types/src/block.rs @@ -136,7 +136,8 @@ impl Block { /// * `root_element` - The root element for membership proofs /// * `block_contents` - Contents of the block. /// * `timestamp` - The timestamp of the block in ms since Unix epoch. - /// should be 0 for block versions 3 and below, and set for block versions 4 and above. + /// should be 0 for block versions 3 and below, and set for block versions + /// 4 and above. pub fn new( version: BlockVersion, parent_id: &BlockID, diff --git a/ledger/db/src/ledger_db.rs b/ledger/db/src/ledger_db.rs index e39aec1cae..d56a591bfc 100644 --- a/ledger/db/src/ledger_db.rs +++ b/ledger/db/src/ledger_db.rs @@ -2335,8 +2335,8 @@ mod ledger_db_test { } #[test] - /// Appending blocks that have ever-increasing and continuous version numbers - /// should work as long as it is <= MAX_BLOCK_VERSION. + /// Appending blocks that have ever-increasing and continuous version + /// numbers should work as long as it is <= MAX_BLOCK_VERSION. /// Appending a block > MAX_BLOCK_VERSION should fail even if it is after a /// block with version == MAX_BLOCK_VERSION. /// Appending a block with a version < last block's version should fail. From 55e6939387abf5077b5382bf0e531231605a39b8 Mon Sep 17 00:00:00 2001 From: Nick Santana Date: Tue, 21 Nov 2023 13:49:19 -0800 Subject: [PATCH 6/6] Remove assert for timestamp conforming to blockversion Currently the logic to populate the timestamp has not been implemented thus it's not practical to enforce the timestamp value at this time. --- blockchain/types/src/block.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/blockchain/types/src/block.rs b/blockchain/types/src/block.rs index 59eb391847..02e276ccf4 100644 --- a/blockchain/types/src/block.rs +++ b/blockchain/types/src/block.rs @@ -147,8 +147,6 @@ impl Block { block_contents: &BlockContents, timestamp: u64, ) -> Self { - assert!((timestamp == 0) ^ version.timestamps_are_supported()); - let contents_hash = block_contents.hash(); let id = compute_block_id( *version,