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

GH-552: max_block_count defaults to 100_000 #558

Merged
merged 3 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion masq_lib/src/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::data_version::DataVersion;
use const_format::concatcp;

pub const DEFAULT_CHAIN: Chain = Chain::PolyMainnet;
pub const CURRENT_SCHEMA_VERSION: usize = 9;
pub const CURRENT_SCHEMA_VERSION: usize = 10;

pub const HIGHEST_RANDOM_CLANDESTINE_PORT: u16 = 9999;
pub const HTTP_PORT: u16 = 80;
Expand Down Expand Up @@ -215,3 +215,5 @@ mod tests {
})
}
}

pub const DEFAULT_MAX_BLOCK_COUNT: u64 = 100_000u64;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's maybe unnecessary that you use the numeric type in the literal. Because constants require the type to be mentioned right behind its name in the declaration. 🤷‍♂️

I'm saying that just because it's so unusual in our team using this format of numbers. Only if forced.

25 changes: 14 additions & 11 deletions node/src/blockchain/blockchain_bridge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use actix::Message;
use actix::{Addr, Recipient};
use itertools::Itertools;
use masq_lib::blockchains::chains::Chain;
use masq_lib::constants::DEFAULT_MAX_BLOCK_COUNT;
use masq_lib::logger::Logger;
use masq_lib::messages::ScanType;
use masq_lib::ui_gateway::NodeFromUiMessage;
Expand Down Expand Up @@ -287,7 +288,7 @@ impl BlockchainBridge {
};
let max_block_count = match self.persistent_config.max_block_count() {
Ok(Some(mbc)) => mbc,
_ => u64::MAX,
_ => DEFAULT_MAX_BLOCK_COUNT,
};
let use_unlimited_block_count_range = u64::MAX == max_block_count;
let use_latest_block = u64::MAX == start_block_nbr;
Expand Down Expand Up @@ -1019,7 +1020,7 @@ mod tests {
)))
.lower_interface_results(Box::new(lower_interface));
let persistent_config = PersistentConfigurationMock::new()
.max_block_count_result(Ok(Some(100_000)))
.max_block_count_result(Ok(Some(DEFAULT_MAX_BLOCK_COUNT)))
.start_block_result(Ok(Some(5))); // no set_start_block_result: set_start_block() must not be called
let mut subject = BlockchainBridge::new(
Box::new(blockchain_interface),
Expand Down Expand Up @@ -1277,11 +1278,12 @@ mod tests {
}

#[test]
fn handle_retrieve_transactions_uses_latest_block_number_upon_get_block_number_error() {
fn handle_retrieve_transactions_uses_default_max_block_count_for_ending_block_number_upon_get_block_number_error(
) {
init_test_logging();
let retrieve_transactions_params_arc = Arc::new(Mutex::new(vec![]));
let system = System::new(
"handle_retrieve_transactions_uses_latest_block_number_upon_get_block_number_error",
"handle_retrieve_transactions_uses_default_max_block_count_for_ending_block_number_upon_get_block_number_error",
);
let (accountant, _, accountant_recording_arc) = make_recorder();
let earning_wallet = make_wallet("somewallet");
Expand Down Expand Up @@ -1311,7 +1313,7 @@ mod tests {
.retrieve_transactions_result(Ok(expected_transactions.clone()))
.lower_interface_results(Box::new(lower_interface));
let persistent_config = PersistentConfigurationMock::new()
.max_block_count_result(Ok(None))
.max_block_count_result(Ok(Some(DEFAULT_MAX_BLOCK_COUNT)))
.start_block_result(Ok(Some(6)));
let subject = BlockchainBridge::new(
Box::new(blockchain_interface_mock),
Expand Down Expand Up @@ -1341,7 +1343,7 @@ mod tests {
*retrieve_transactions_params,
vec![(
BlockNumber::Number(6u64.into()),
BlockNumber::Latest,
BlockNumber::Number((DEFAULT_MAX_BLOCK_COUNT + 6u64).into()),
earning_wallet
)]
);
Expand All @@ -1361,7 +1363,7 @@ mod tests {
}),
}
);
TestLogHandler::new().exists_log_containing("DEBUG: BlockchainBridge: Using 'latest' block number instead of a literal number. QueryFailed(\"Failed to read the latest block number\")");
TestLogHandler::new().exists_log_containing("DEBUG: BlockchainBridge: Using '100006' ending block number. QueryFailed(\"Failed to read the latest block number\")");
}

#[test]
Expand Down Expand Up @@ -1400,7 +1402,7 @@ mod tests {
.retrieve_transactions_result(Ok(expected_transactions.clone()))
.lower_interface_results(Box::new(lower_interface));
let persistent_config = PersistentConfigurationMock::new()
.max_block_count_result(Ok(None))
.max_block_count_result(Ok(Some(DEFAULT_MAX_BLOCK_COUNT)))
.start_block_result(Ok(None));
let subject = BlockchainBridge::new(
Box::new(blockchain_interface_mock),
Expand Down Expand Up @@ -1449,7 +1451,8 @@ mod tests {
}

#[test]
fn handle_retrieve_transactions_with_latest_for_start_and_end_block_is_supported() {
fn handle_retrieve_transactions_when_get_block_number_fails_uses_latest_for_start_and_end_block(
) {
let retrieve_transactions_params_arc = Arc::new(Mutex::new(vec![]));
let earning_wallet = make_wallet("somewallet");
let amount = 42;
Expand All @@ -1471,11 +1474,11 @@ mod tests {
};

let system = System::new(
"handle_retrieve_transactions_with_latest_for_start_and_end_block_is_supported",
"handle_retrieve_transactions_when_get_block_number_fails_uses_latest_for_start_and_end_block",
);
let (accountant, _, accountant_recording_arc) = make_recorder();
let persistent_config = PersistentConfigurationMock::new()
.max_block_count_result(Ok(None))
.max_block_count_result(Ok(Some(DEFAULT_MAX_BLOCK_COUNT)))
.start_block_result(Ok(None));
let latest_block_number = LatestBlockNumber::Err(BlockchainError::QueryFailed(
"Failed to read from block chain service".to_string(),
Expand Down
2 changes: 1 addition & 1 deletion node/src/database/db_initializer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ mod tests {
#[test]
fn constants_have_correct_values() {
assert_eq!(DATABASE_FILE, "node-data.db");
assert_eq!(CURRENT_SCHEMA_VERSION, 9);
assert_eq!(CURRENT_SCHEMA_VERSION, 10);
}

#[test]
Expand Down
2 changes: 2 additions & 0 deletions node/src/database/db_migrations/db_migrator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::database::db_migrations::migrations::migration_5_to_6::Migrate_5_to_6
use crate::database::db_migrations::migrations::migration_6_to_7::Migrate_6_to_7;
use crate::database::db_migrations::migrations::migration_7_to_8::Migrate_7_to_8;
use crate::database::db_migrations::migrations::migration_8_to_9::Migrate_8_to_9;
use crate::database::db_migrations::migrations::migration_9_to_10::Migrate_9_to_10;
use crate::database::db_migrations::migrator_utils::{
DBMigDeclarator, DBMigrationUtilities, DBMigrationUtilitiesReal, DBMigratorInnerConfiguration,
};
Expand Down Expand Up @@ -78,6 +79,7 @@ impl DbMigratorReal {
&Migrate_6_to_7,
&Migrate_7_to_8,
&Migrate_8_to_9,
&Migrate_9_to_10,
]
}

Expand Down
71 changes: 71 additions & 0 deletions node/src/database/db_migrations/migrations/migration_9_to_10.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
use crate::database::db_migrations::db_migrator::DatabaseMigration;
use crate::database::db_migrations::migrator_utils::DBMigDeclarator;

#[allow(non_camel_case_types)]
pub struct Migrate_9_to_10;

impl DatabaseMigration for Migrate_9_to_10 {
fn migrate<'a>(
&self,
declaration_utils: Box<dyn DBMigDeclarator + 'a>,
) -> rusqlite::Result<()> {
declaration_utils.execute_upon_transaction(&[
&"INSERT INTO config (name, value, encrypted) VALUES ('max_block_count', 100000, 0) ON CONFLICT(name) DO UPDATE SET value = 100000 WHERE name = 'max_block_count'"
])
}

fn old_version(&self) -> usize {
9
}
}

#[cfg(test)]
mod tests {
use crate::database::db_initializer::{
DbInitializationConfig, DbInitializer, DbInitializerReal, DATABASE_FILE,
};
use crate::test_utils::database_utils::{
bring_db_0_back_to_life_and_return_connection, make_external_data, retrieve_config_row,
};
use masq_lib::test_utils::logging::{init_test_logging, TestLogHandler};
use masq_lib::test_utils::utils::ensure_node_home_directory_exists;
use std::fs::create_dir_all;

#[test]
fn migration_from_9_to_10_is_properly_set() {
init_test_logging();
let dir_path = ensure_node_home_directory_exists(
"db_migrations",
"migration_from_9_to_10_is_properly_set",
);
create_dir_all(&dir_path).unwrap();
let db_path = dir_path.join(DATABASE_FILE);
let _ = bring_db_0_back_to_life_and_return_connection(&db_path);
let subject = DbInitializerReal::default();

let result = subject.initialize_to_version(
&dir_path,
10,
DbInitializationConfig::create_or_migrate(make_external_data()),
);
let connection = result.unwrap();
let (mp_value, mp_encrypted) = retrieve_config_row(connection.as_ref(), "max_block_count");
let (cs_value, cs_encrypted) = retrieve_config_row(connection.as_ref(), "schema_version");
assert_eq!(mp_value, Some(100_000u64.to_string()));
assert_eq!(mp_encrypted, false);
assert_eq!(cs_value, Some(10.to_string()));
assert_eq!(cs_encrypted, false);
TestLogHandler::new().assert_logs_contain_in_order(vec![
"DbMigrator: Database successfully migrated from version 0 to 1",
"DbMigrator: Database successfully migrated from version 1 to 2",
"DbMigrator: Database successfully migrated from version 2 to 3",
"DbMigrator: Database successfully migrated from version 3 to 4",
"DbMigrator: Database successfully migrated from version 4 to 5",
"DbMigrator: Database successfully migrated from version 5 to 6",
"DbMigrator: Database successfully migrated from version 6 to 7",
"DbMigrator: Database successfully migrated from version 7 to 8",
"DbMigrator: Database successfully migrated from version 8 to 9",
"DbMigrator: Database successfully migrated from version 9 to 10",
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I'm all okay with this test. But on the other hand, it also isn't exactly as I would expect it to be. I noticed you got inspired from your own migration "8 to 9", and the truth is there it was where you've begun writing in a different style than in which the other migrations are done.

I understand these are almost cosmetics but still, maybe you could have some time to do these simple changes.

  1. Mostly for formality, hmm, still reasonable, we tended to arrange it so that there is the act properly isolated. The issue in your case is that you make it look as if you were testing all the steps from 0 to 10. It shouldn't be necessary, you agree with me.
    So again, for it's better structured like that, we first ran all the migration but yours, where we stopped, and left the database in the state as it would be if you were truly in need of this last migration. Let's say from 0 to 9.

Then, the only thing left for the act is to run the function initialize_to_version where you specify the target as "10".

This way the test is clear about what it is testing.

  1. Also based on the insight I just threw in, the logs you assert on excessively, are somewhat too much for me, seen as so many records, but together of not much value, bland, only stealing somebody's attention.
    The process of gradual migrations is heavily tested on its own, separately. Here I could argue you're not testing the code added but rather the machinery around, which doesn't sound right.

Therefore I'd recommend you not to assert on logs for these database migrations unless it is an actual new log straight from within your implementation of that new migration. Which this is not the case. My opinion is the test would be better free of this massive log assertion.

I want to ask you to implement my ideas here (9 to 10) as well as for the previous migration (8 to 9). Thank you 🙏

}
}
1 change: 1 addition & 0 deletions node/src/database/db_migrations/migrations/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ pub mod migration_5_to_6;
pub mod migration_6_to_7;
pub mod migration_7_to_8;
pub mod migration_8_to_9;
pub mod migration_9_to_10;
Loading