From 0dc87db640c8baa95b22e78060011e13b5d53531 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Tue, 3 Sep 2024 11:24:09 +0200 Subject: [PATCH 1/6] feat: enhance rollbacks logs in 'CardanoChainreaderBlockStreamer' --- .../chain_reader_block_streamer.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/mithril-common/src/cardano_block_scanner/chain_reader_block_streamer.rs b/mithril-common/src/cardano_block_scanner/chain_reader_block_streamer.rs index b8886fc56cf..0397ba9622d 100644 --- a/mithril-common/src/cardano_block_scanner/chain_reader_block_streamer.rs +++ b/mithril-common/src/cardano_block_scanner/chain_reader_block_streamer.rs @@ -59,9 +59,17 @@ impl BlockStreamer for ChainReaderBlockStreamer { .position(|block| block.slot_number == rollback_slot_number); match index_rollback { Some(index_rollback) => { + debug!( + self.logger, + "ChainScannedBlocks handled a buffer RollBackward({rollback_slot_number:?})" + ); roll_forwards.truncate(index_rollback + 1); } None => { + debug!( + self.logger, + "ChainScannedBlocks triggered a full RollBackward({rollback_slot_number:?})" + ); chain_scanned_blocks = ChainScannedBlocks::RollBackward(rollback_slot_number); return Ok(Some(chain_scanned_blocks)); @@ -132,7 +140,7 @@ impl ChainReaderBlockStreamer { Some(ChainBlockNextAction::RollBackward { slot_number: rollback_slot_number, }) => { - debug!( + trace!( self.logger, "ChainReaderBlockStreamer received a RollBackward({rollback_slot_number:?})" ); From 8d4ac38349a583561d6c4bd69e0691880706bf4f Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Thu, 5 Sep 2024 17:16:36 +0200 Subject: [PATCH 2/6] feat: implement 'with_highest_block_number_below_slot_number' for Cardano transaction repository --- .../get_cardano_transaction.rs | 119 ++++++++++++------ 1 file changed, 83 insertions(+), 36 deletions(-) diff --git a/internal/mithril-persistence/src/database/query/cardano_transaction/get_cardano_transaction.rs b/internal/mithril-persistence/src/database/query/cardano_transaction/get_cardano_transaction.rs index b8cf582f08e..177b13a6fbf 100644 --- a/internal/mithril-persistence/src/database/query/cardano_transaction/get_cardano_transaction.rs +++ b/internal/mithril-persistence/src/database/query/cardano_transaction/get_cardano_transaction.rs @@ -72,10 +72,10 @@ impl GetCardanoTransactionQuery { Self { condition } } - pub fn by_slot_number(slot_number: SlotNumber) -> Self { + pub fn with_highest_block_number_below_slot_number(slot_number: SlotNumber) -> Self { Self { condition: WhereCondition::new( - "slot_number = ?*", + "block_number = (select max(block_number) from cardano_tx where slot_number <= ?*)", vec![Value::Integer(*slot_number as i64)], ), } @@ -120,6 +120,18 @@ mod tests { .unwrap(); } + fn transaction_record( + block_number: BlockNumber, + slot_number: SlotNumber, + ) -> CardanoTransactionRecord { + CardanoTransactionRecord::new( + format!("tx-hash-{}", slot_number), + block_number, + slot_number, + format!("block-hash-{}", block_number), + ) + } + #[test] fn with_highest_block_number() { let connection = cardano_tx_db_connection().unwrap(); @@ -132,30 +144,10 @@ mod tests { insert_transactions( &connection, vec![ - CardanoTransactionRecord::new( - "tx-hash-0", - BlockNumber(10), - SlotNumber(50), - "block-hash-10", - ), - CardanoTransactionRecord::new( - "tx-hash-1", - BlockNumber(10), - SlotNumber(51), - "block-hash-10", - ), - CardanoTransactionRecord::new( - "tx-hash-2", - BlockNumber(11), - SlotNumber(54), - "block-hash-11", - ), - CardanoTransactionRecord::new( - "tx-hash-3", - BlockNumber(11), - SlotNumber(55), - "block-hash-11", - ), + transaction_record(BlockNumber(10), SlotNumber(50)), + transaction_record(BlockNumber(10), SlotNumber(51)), + transaction_record(BlockNumber(11), SlotNumber(54)), + transaction_record(BlockNumber(11), SlotNumber(55)), ], ); @@ -164,19 +156,74 @@ mod tests { .unwrap(); assert_eq!( vec![ - CardanoTransactionRecord::new( - "tx-hash-2", - BlockNumber(11), - SlotNumber(54), - "block-hash-11" + transaction_record(BlockNumber(11), SlotNumber(54)), + transaction_record(BlockNumber(11), SlotNumber(55)), + ], + records + ); + } + + #[test] + fn with_highest_block_number_below_slot_number() { + let connection = cardano_tx_db_connection().unwrap(); + + let cursor = connection + .fetch( + GetCardanoTransactionQuery::with_highest_block_number_below_slot_number( + SlotNumber(51), ), - CardanoTransactionRecord::new( - "tx-hash-3", - BlockNumber(11), - SlotNumber(55), - "block-hash-11" + ) + .unwrap(); + assert_eq!(0, cursor.count()); + + insert_transactions( + &connection, + vec![transaction_record(BlockNumber(2), SlotNumber(5))], + ); + + let records: Vec = connection + .fetch_collect( + GetCardanoTransactionQuery::with_highest_block_number_below_slot_number( + SlotNumber(5), ), + ) + .unwrap(); + assert_eq!( + vec![transaction_record(BlockNumber(2), SlotNumber(5)),], + records + ); + + insert_transactions( + &connection, + vec![ + transaction_record(BlockNumber(10), SlotNumber(50)), + transaction_record(BlockNumber(11), SlotNumber(51)), + transaction_record(BlockNumber(14), SlotNumber(54)), + transaction_record(BlockNumber(15), SlotNumber(55)), ], + ); + + let records: Vec = connection + .fetch_collect( + GetCardanoTransactionQuery::with_highest_block_number_below_slot_number( + SlotNumber(53), + ), + ) + .unwrap(); + assert_eq!( + vec![transaction_record(BlockNumber(11), SlotNumber(51)),], + records + ); + + let records: Vec = connection + .fetch_collect( + GetCardanoTransactionQuery::with_highest_block_number_below_slot_number( + SlotNumber(54), + ), + ) + .unwrap(); + assert_eq!( + vec![transaction_record(BlockNumber(14), SlotNumber(54)),], records ); } From b280774fff10225d32fcb19252783cc67ee68b07 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Thu, 5 Sep 2024 17:17:11 +0200 Subject: [PATCH 3/6] fix: remove rolled-back transactions edge cases Does not trigger an error when no block exists above the slot number, also find the closest block above the slot number if rolled back block does not have transaction. --- .../cardano_transaction_repository.rs | 133 +++++++++++++++++- .../cardano_transaction_repository.rs | 8 +- .../cardano_transaction_repository.rs | 8 +- 3 files changed, 128 insertions(+), 21 deletions(-) diff --git a/internal/mithril-persistence/src/database/repository/cardano_transaction_repository.rs b/internal/mithril-persistence/src/database/repository/cardano_transaction_repository.rs index 97e19023ed3..2b24ed46582 100644 --- a/internal/mithril-persistence/src/database/repository/cardano_transaction_repository.rs +++ b/internal/mithril-persistence/src/database/repository/cardano_transaction_repository.rs @@ -215,12 +215,13 @@ impl CardanoTransactionRepository { Ok(()) } - /// Get the block number for a given slot number - pub async fn get_block_number_by_slot_number( + /// Get the closest block number above a given slot number + pub async fn get_closest_block_number_above_slot_number( &self, slot_number: SlotNumber, ) -> StdResult> { - let query = GetCardanoTransactionQuery::by_slot_number(slot_number); + let query = + GetCardanoTransactionQuery::with_highest_block_number_below_slot_number(slot_number); let record = self.connection_pool.connection()?.fetch_first(query)?; Ok(record.map(|r| r.block_number)) @@ -277,7 +278,7 @@ impl CardanoTransactionRepository { /// /// * Remove transactions with block number strictly greater than the given block number /// * Remove block range roots that have lower bound range strictly above the given block number - pub async fn remove_rolled_back_transactions_and_block_range( + pub async fn remove_rolled_back_transactions_and_block_range_by_block_number( &self, block_number: BlockNumber, ) -> StdResult<()> { @@ -293,6 +294,25 @@ impl CardanoTransactionRepository { Ok(()) } + + /// Remove transactions and block range roots that are in a rolled-back fork + /// + /// * Remove transactions with closest block number strictly greater than the given slot number if exists + /// * Remove block range roots that have lower bound range strictly above the aforementioned block number + pub async fn remove_rolled_back_transactions_and_block_range_by_slot_number( + &self, + slot_number: SlotNumber, + ) -> StdResult<()> { + if let Some(block_number) = self + .get_closest_block_number_above_slot_number(slot_number) + .await? + { + self.remove_rolled_back_transactions_and_block_range_by_block_number(block_number) + .await?; + } + + Ok(()) + } } #[async_trait] @@ -910,7 +930,7 @@ mod tests { } #[tokio::test] - async fn repository_get_block_number_by_slot_number() { + async fn repository_get_closest_block_number_by_slot_number() { let connection = cardano_tx_db_connection().unwrap(); let repository = CardanoTransactionRepository::new(Arc::new( SqliteConnectionPool::build_from_connection(connection), @@ -927,7 +947,7 @@ mod tests { .unwrap(); let transaction_block_number_retrieved = repository - .get_block_number_by_slot_number(SlotNumber(500)) + .get_closest_block_number_above_slot_number(SlotNumber(500)) .await .unwrap(); @@ -1215,10 +1235,109 @@ mod tests { .unwrap(); repository - .remove_rolled_back_transactions_and_block_range(BlockRange::LENGTH * 3) + .remove_rolled_back_transactions_and_block_range_by_block_number(BlockRange::LENGTH * 3) .await .unwrap(); assert_eq!(2, repository.get_all_transactions().await.unwrap().len()); assert_eq!(2, repository.get_all_block_range_root().unwrap().len()); } + + #[tokio::test] + async fn remove_rolled_back_transactions_and_block_range_by_slot_number() { + fn transaction_record( + block_number: BlockNumber, + slot_number: SlotNumber, + ) -> CardanoTransactionRecord { + CardanoTransactionRecord::new( + format!("tx-hash-{}", slot_number), + block_number, + slot_number, + format!("block-hash-{}", block_number), + ) + } + + let repository = CardanoTransactionRepository::new(Arc::new( + SqliteConnectionPool::build(1, cardano_tx_db_connection).unwrap(), + )); + + repository + .create_transactions(vec![ + transaction_record(BlockNumber(10), SlotNumber(50)), + transaction_record(BlockNumber(11), SlotNumber(51)), + transaction_record(BlockNumber(13), SlotNumber(52)), + transaction_record(BlockNumber(101), SlotNumber(100)), + transaction_record(BlockNumber(202), SlotNumber(200)), + ]) + .await + .unwrap(); + + { + repository + .remove_rolled_back_transactions_and_block_range_by_slot_number(SlotNumber(110)) + .await + .expect("Failed to remove rolled back transactions"); + + let transactions = repository + .get_all() + .await + .unwrap() + .into_iter() + .map(|v| v.into()) + .collect::>(); + assert_eq!( + vec![ + transaction_record(BlockNumber(10), SlotNumber(50)), + transaction_record(BlockNumber(11), SlotNumber(51)), + transaction_record(BlockNumber(13), SlotNumber(52)), + transaction_record(BlockNumber(101), SlotNumber(100)), + ], + transactions + ); + } + + { + repository + .remove_rolled_back_transactions_and_block_range_by_slot_number(SlotNumber(53)) + .await + .expect("Failed to remove rolled back transactions"); + + let transactions = repository + .get_all() + .await + .unwrap() + .into_iter() + .map(|v| v.into()) + .collect::>(); + assert_eq!( + vec![ + transaction_record(BlockNumber(10), SlotNumber(50)), + transaction_record(BlockNumber(11), SlotNumber(51)), + transaction_record(BlockNumber(13), SlotNumber(52)), + ], + transactions + ); + } + + { + repository + .remove_rolled_back_transactions_and_block_range_by_slot_number(SlotNumber(51)) + .await + .expect("Failed to remove rolled back transactions"); + + let transactions = repository + .get_all() + .await + .unwrap() + .into_iter() + .map(|v| v.into()) + .collect::>(); + assert_eq!( + vec![ + transaction_record(BlockNumber(10), SlotNumber(50)), + transaction_record(BlockNumber(11), SlotNumber(51)), + ], + transactions + ); + } + } } diff --git a/mithril-aggregator/src/database/repository/cardano_transaction_repository.rs b/mithril-aggregator/src/database/repository/cardano_transaction_repository.rs index 01eb25be4a5..08eb4937d6b 100644 --- a/mithril-aggregator/src/database/repository/cardano_transaction_repository.rs +++ b/mithril-aggregator/src/database/repository/cardano_transaction_repository.rs @@ -51,13 +51,7 @@ impl TransactionStore for CardanoTransactionRepository { &self, slot_number: SlotNumber, ) -> StdResult<()> { - let block_number = self - .get_block_number_by_slot_number(slot_number) - .await? - .ok_or_else(|| { - anyhow::anyhow!("No block number found for slot number {}", slot_number) - })?; - self.remove_rolled_back_transactions_and_block_range(block_number) + self.remove_rolled_back_transactions_and_block_range_by_slot_number(slot_number) .await } } diff --git a/mithril-signer/src/database/repository/cardano_transaction_repository.rs b/mithril-signer/src/database/repository/cardano_transaction_repository.rs index 9119e0711d1..7ce47d3409b 100644 --- a/mithril-signer/src/database/repository/cardano_transaction_repository.rs +++ b/mithril-signer/src/database/repository/cardano_transaction_repository.rs @@ -51,13 +51,7 @@ impl TransactionStore for CardanoTransactionRepository { &self, slot_number: SlotNumber, ) -> StdResult<()> { - let block_number = self - .get_block_number_by_slot_number(slot_number) - .await? - .ok_or_else(|| { - anyhow::anyhow!("No block number found for slot number {}", slot_number) - })?; - self.remove_rolled_back_transactions_and_block_range(block_number) + self.remove_rolled_back_transactions_and_block_range_by_slot_number(slot_number) .await } } From ab1d016585337c9471beb107efc3938784b718d6 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Wed, 11 Sep 2024 11:24:37 +0200 Subject: [PATCH 4/6] chore: apply review comments --- .../cardano_transaction_repository.rs | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/internal/mithril-persistence/src/database/repository/cardano_transaction_repository.rs b/internal/mithril-persistence/src/database/repository/cardano_transaction_repository.rs index 2b24ed46582..4d9eea12911 100644 --- a/internal/mithril-persistence/src/database/repository/cardano_transaction_repository.rs +++ b/internal/mithril-persistence/src/database/repository/cardano_transaction_repository.rs @@ -1247,9 +1247,10 @@ mod tests { fn transaction_record( block_number: BlockNumber, slot_number: SlotNumber, + tx_hash: &str, ) -> CardanoTransactionRecord { CardanoTransactionRecord::new( - format!("tx-hash-{}", slot_number), + tx_hash, block_number, slot_number, format!("block-hash-{}", block_number), @@ -1262,11 +1263,12 @@ mod tests { repository .create_transactions(vec![ - transaction_record(BlockNumber(10), SlotNumber(50)), - transaction_record(BlockNumber(11), SlotNumber(51)), - transaction_record(BlockNumber(13), SlotNumber(52)), - transaction_record(BlockNumber(101), SlotNumber(100)), - transaction_record(BlockNumber(202), SlotNumber(200)), + transaction_record(BlockNumber(10), SlotNumber(50), "tx-hash-1"), + transaction_record(BlockNumber(11), SlotNumber(51), "tx-hash-2"), + transaction_record(BlockNumber(13), SlotNumber(52), "tx-hash-3"), + transaction_record(BlockNumber(13), SlotNumber(52), "tx-hash-4"), + transaction_record(BlockNumber(101), SlotNumber(100), "tx-hash-5"), + transaction_record(BlockNumber(202), SlotNumber(200), "tx-hash-56"), ]) .await .unwrap(); @@ -1286,10 +1288,11 @@ mod tests { .collect::>(); assert_eq!( vec![ - transaction_record(BlockNumber(10), SlotNumber(50)), - transaction_record(BlockNumber(11), SlotNumber(51)), - transaction_record(BlockNumber(13), SlotNumber(52)), - transaction_record(BlockNumber(101), SlotNumber(100)), + transaction_record(BlockNumber(10), SlotNumber(50), "tx-hash-1"), + transaction_record(BlockNumber(11), SlotNumber(51), "tx-hash-2"), + transaction_record(BlockNumber(13), SlotNumber(52), "tx-hash-3"), + transaction_record(BlockNumber(13), SlotNumber(52), "tx-hash-4"), + transaction_record(BlockNumber(101), SlotNumber(100), "tx-hash-5"), ], transactions ); @@ -1310,9 +1313,10 @@ mod tests { .collect::>(); assert_eq!( vec![ - transaction_record(BlockNumber(10), SlotNumber(50)), - transaction_record(BlockNumber(11), SlotNumber(51)), - transaction_record(BlockNumber(13), SlotNumber(52)), + transaction_record(BlockNumber(10), SlotNumber(50), "tx-hash-1"), + transaction_record(BlockNumber(11), SlotNumber(51), "tx-hash-2"), + transaction_record(BlockNumber(13), SlotNumber(52), "tx-hash-3"), + transaction_record(BlockNumber(13), SlotNumber(52), "tx-hash-4"), ], transactions ); @@ -1333,8 +1337,8 @@ mod tests { .collect::>(); assert_eq!( vec![ - transaction_record(BlockNumber(10), SlotNumber(50)), - transaction_record(BlockNumber(11), SlotNumber(51)), + transaction_record(BlockNumber(10), SlotNumber(50), "tx-hash-1"), + transaction_record(BlockNumber(11), SlotNumber(51), "tx-hash-2"), ], transactions ); From 95a5329261199b6b6d60cc6c9f0a69d73996cfa7 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Wed, 11 Sep 2024 11:27:39 +0200 Subject: [PATCH 5/6] docs: update CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1778863323..9b7b67f9188 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,8 @@ As a minor extension, we have adopted a slightly different versioning convention - Support infinite preloading of Cardano transactions in signer. +- Fix Cardano transactions rollbacks creating panics in signer and aggregator. + - **UNSTABLE** Cardano stake distribution certification: - Implement the signable and artifact builders for the signed entity type `CardanoStakeDistribution`. From 9eae92defb3a71166b86804ba935c9e47b4ef751 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Wed, 11 Sep 2024 11:29:37 +0200 Subject: [PATCH 6/6] chore: bump crates versions - 'mithril-persistence' from '0.2.25' to '0.2.26' - 'mithril-aggregator' from '0.5.61' to '0.5.62' - 'mithril-common' from '0.4.50' to '0.4.51' - 'mithril-signer' from '0.2.180' to '0.2.181'. --- Cargo.lock | 8 ++++---- internal/mithril-persistence/Cargo.toml | 2 +- mithril-aggregator/Cargo.toml | 8 ++++++-- mithril-common/Cargo.toml | 2 +- mithril-signer/Cargo.toml | 2 +- 5 files changed, 13 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bae39c7be9c..250ac9305ab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3403,7 +3403,7 @@ dependencies = [ [[package]] name = "mithril-aggregator" -version = "0.5.61" +version = "0.5.62" dependencies = [ "anyhow", "async-trait", @@ -3559,7 +3559,7 @@ dependencies = [ [[package]] name = "mithril-common" -version = "0.4.50" +version = "0.4.51" dependencies = [ "anyhow", "async-trait", @@ -3657,7 +3657,7 @@ dependencies = [ [[package]] name = "mithril-persistence" -version = "0.2.25" +version = "0.2.26" dependencies = [ "anyhow", "async-trait", @@ -3704,7 +3704,7 @@ dependencies = [ [[package]] name = "mithril-signer" -version = "0.2.180" +version = "0.2.181" dependencies = [ "anyhow", "async-trait", diff --git a/internal/mithril-persistence/Cargo.toml b/internal/mithril-persistence/Cargo.toml index 8c24a325683..7e3c787eb82 100644 --- a/internal/mithril-persistence/Cargo.toml +++ b/internal/mithril-persistence/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-persistence" -version = "0.2.25" +version = "0.2.26" description = "Common types, interfaces, and utilities to persist data for Mithril nodes." authors = { workspace = true } edition = { workspace = true } diff --git a/mithril-aggregator/Cargo.toml b/mithril-aggregator/Cargo.toml index efc81a510dd..4d83d7f065b 100644 --- a/mithril-aggregator/Cargo.toml +++ b/mithril-aggregator/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-aggregator" -version = "0.5.61" +version = "0.5.62" description = "A Mithril Aggregator server" authors = { workspace = true } edition = { workspace = true } @@ -51,7 +51,11 @@ thiserror = "1.0.63" tokio = { version = "1.40.0", features = ["full"] } tokio-util = { version = "0.7.12", features = ["codec"] } typetag = "0.2.18" -uuid = { version = "1.10.0", features = ["v4", "fast-rng", "macro-diagnostics"] } +uuid = { version = "1.10.0", features = [ + "v4", + "fast-rng", + "macro-diagnostics", +] } warp = "0.3.7" zstd = { version = "0.13.2", features = ["zstdmt"] } diff --git a/mithril-common/Cargo.toml b/mithril-common/Cargo.toml index 6c88cb42ba5..3863cfe6358 100644 --- a/mithril-common/Cargo.toml +++ b/mithril-common/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-common" -version = "0.4.50" +version = "0.4.51" description = "Common types, interfaces, and utilities for Mithril nodes." authors = { workspace = true } edition = { workspace = true } diff --git a/mithril-signer/Cargo.toml b/mithril-signer/Cargo.toml index 46bdba55349..bcc96e048b2 100644 --- a/mithril-signer/Cargo.toml +++ b/mithril-signer/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-signer" -version = "0.2.180" +version = "0.2.181" description = "A Mithril Signer" authors = { workspace = true } edition = { workspace = true }