Skip to content

GH-552: Eliminate extra block increment #576

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.

Sign up for GitHub

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

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

GH-552: Eliminate extra block increment #576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
205 changes: 203 additions & 2 deletions node/src/blockchain/blockchain_bridge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,7 @@ mod tests {
let amount = 42;
let amount2 = 55;
let expected_transactions = RetrievedBlockchainTransactions {
new_start_block: BlockNumber::Number(8675309u64.into()),
new_start_block: BlockNumber::Number(100_006u64.into()),
transactions: vec![
BlockchainTransaction {
block_number: 7,
Expand Down Expand Up @@ -1356,7 +1356,7 @@ mod tests {
&ReceivedPayments {
timestamp: received_payments.timestamp,
payments: expected_transactions.transactions,
new_start_block: 8675309u64,
new_start_block: 100_006u64,
response_skeleton_opt: Some(ResponseSkeleton {
client_id: 1234,
context_id: 4321
Expand Down Expand Up @@ -1703,6 +1703,207 @@ mod tests {
let _ = subject.handle_retrieve_transactions(retrieve_transactions);
}

#[test]
fn handle_retrieve_transactions_repeated_calling_does_not_skip_blocks() {
let retrieve_transactions_params_arc = Arc::new(Mutex::new(vec![]));
let system =
System::new("handle_retrieve_transactions_repeated_calling_does_not_skip_blocks");
let (accountant, _, accountant_recording_arc) = make_recorder();
let earning_wallet = make_wallet("earningwallet");
let amount1 = 42;
let amount2 = 55;
let amount3 = 75;
let amount4 = 99;
let initial_start_block: u64 = 0x45f2f3; // 4_584_179
let new_start_block_1 = initial_start_block + 1 + DEFAULT_MAX_BLOCK_COUNT;
let expected_transactions1 = RetrievedBlockchainTransactions {
new_start_block: BlockNumber::Number(new_start_block_1.into()),
transactions: vec![BlockchainTransaction {
block_number: initial_start_block + 0x14382,
from: earning_wallet.clone(),
wei_amount: amount1,
}],
};
let new_start_block_2 = initial_start_block + 2 + (2 * DEFAULT_MAX_BLOCK_COUNT);
let expected_transactions2 = RetrievedBlockchainTransactions {
new_start_block: BlockNumber::Number(new_start_block_2.into()),
transactions: vec![BlockchainTransaction {
block_number: initial_start_block + 0x308b3,
from: earning_wallet.clone(),
wei_amount: amount2,
}],
};
let new_start_block_3 = initial_start_block + 3 + (3 * DEFAULT_MAX_BLOCK_COUNT);
let expected_transactions3 = RetrievedBlockchainTransactions {
new_start_block: BlockNumber::Number(new_start_block_3.into()),
transactions: vec![BlockchainTransaction {
block_number: initial_start_block + 0x34248,
from: earning_wallet.clone(),
wei_amount: amount3,
}],
};
let new_start_block_4 = 0x4be667;
let expected_transactions4 = RetrievedBlockchainTransactions {
new_start_block: BlockNumber::Number(new_start_block_4.into()),
transactions: vec![BlockchainTransaction {
block_number: initial_start_block + 0x49b7e,
from: earning_wallet.clone(),
wei_amount: amount4,
}],
};
let lower_interface = LowBlockchainIntMock::default()
.get_block_number_result(Ok(0x4be663.into()))
.get_block_number_result(Ok(0x4be664.into()))
.get_block_number_result(Ok(0x4be665.into()))
.get_block_number_result(Ok(new_start_block_4.into()));
let blockchain_interface = BlockchainInterfaceMock::default()
.lower_interface_results(Box::new(lower_interface))
.retrieve_transactions_params(&retrieve_transactions_params_arc)
.retrieve_transactions_result(Ok(expected_transactions1.clone()))
.retrieve_transactions_result(Ok(expected_transactions2.clone()))
.retrieve_transactions_result(Ok(expected_transactions3.clone()))
.retrieve_transactions_result(Ok(expected_transactions4.clone()));
let persistent_config = PersistentConfigurationMock::new()
.max_block_count_result(Ok(Some(DEFAULT_MAX_BLOCK_COUNT)))
.start_block_result(Ok(Some(initial_start_block)))
.start_block_result(Ok(Some(new_start_block_1)))
.start_block_result(Ok(Some(new_start_block_2)))
.start_block_result(Ok(Some(new_start_block_3)))
.start_block_result(Ok(Some(new_start_block_4)));
let subject = BlockchainBridge::new(
Box::new(blockchain_interface),
Box::new(persistent_config),
false,
);
let addr = subject.start();
let subject_subs = BlockchainBridge::make_subs_from(&addr);
let peer_actors = peer_actors_builder().accountant(accountant).build();
send_bind_message!(subject_subs, peer_actors);
let retrieve_transactions1 = RetrieveTransactions {
recipient: earning_wallet.clone(),
response_skeleton_opt: Some(ResponseSkeleton {
client_id: 1234,
context_id: 1,
}),
};
let retrieve_transactions2 = RetrieveTransactions {
recipient: earning_wallet.clone(),
response_skeleton_opt: Some(ResponseSkeleton {
client_id: 1234,
context_id: 2,
}),
};
let retrieve_transactions3 = RetrieveTransactions {
recipient: earning_wallet.clone(),
response_skeleton_opt: Some(ResponseSkeleton {
client_id: 1234,
context_id: 3,
}),
};
let retrieve_transactions4 = RetrieveTransactions {
recipient: earning_wallet.clone(),
response_skeleton_opt: Some(ResponseSkeleton {
client_id: 1234,
context_id: 4,
}),
};
let before = SystemTime::now();

let _ = addr.try_send(retrieve_transactions1).unwrap();
let _ = addr.try_send(retrieve_transactions2).unwrap();
let _ = addr.try_send(retrieve_transactions3).unwrap();
let _ = addr.try_send(retrieve_transactions4).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly as I pointed out in the comment after this one, you could refactor some parts out like this:

The replacement passage starts by send_bind_message!().

let requests = 1..=4.map(|context_id|{
     RetrieveTransactions {
            recipient: earning_wallet.clone(),
            response_skeleton_opt: Some(ResponseSkeleton {
                client_id: 1234,
                context_id,
     })
}).collect_vec();
let before = SystemTime::now();

requests.into_iter().for_each(|request|{
     let _ = addr.try_send(request).unwrap();
})

System::current().stop();
system.run();
let after = SystemTime::now();

😉


System::current().stop();
system.run();
let after = SystemTime::now();

let retrieve_transactions_params = retrieve_transactions_params_arc.lock().unwrap();
assert_eq!(
*retrieve_transactions_params,
vec![
(
BlockNumber::Number(initial_start_block.into()),
BlockNumber::Number((initial_start_block + DEFAULT_MAX_BLOCK_COUNT).into()),
earning_wallet.clone()
),
(
BlockNumber::Number(new_start_block_1.into()),
BlockNumber::Number((new_start_block_1 + DEFAULT_MAX_BLOCK_COUNT).into()),
earning_wallet.clone()
),
(
BlockNumber::Number(new_start_block_2.into()),
BlockNumber::Number((new_start_block_2 + DEFAULT_MAX_BLOCK_COUNT).into()),
earning_wallet.clone()
),
(
BlockNumber::Number(new_start_block_3.into()),
BlockNumber::Number(new_start_block_4.into()),
earning_wallet.clone()
),
]
);
let accountant_received_payment = accountant_recording_arc.lock().unwrap();
assert_eq!(accountant_received_payment.len(), 4);
let received_payments1 = accountant_received_payment.get_record::<ReceivedPayments>(0);
check_timestamp(before, received_payments1.timestamp, after);
assert_eq!(
received_payments1,
&ReceivedPayments {
timestamp: received_payments1.timestamp,
payments: expected_transactions1.transactions,
new_start_block: new_start_block_1,
response_skeleton_opt: Some(ResponseSkeleton {
client_id: 1234,
context_id: 1
}),
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind if we rewrite these repetitive blocks of code as in iterator over a set of data?

I imagine it like:

let accountant_received_payment = accountant_recording_arc.lock().unwrap();
vec![
(received_payments_1, expected_transactions_1, new_start_bock_1),
(received_payments_2, expected_transactions_2, new_start_bock_2),
(received_payments_3, expected_transactions_3, new_start_bock_3),
(received_payments_4, expected_transactions_4, new_start_bock_4),
]
.enumerate()
.for_each(|(idx, (received_payments, expected_transactions, new_start_bock))|{
        let received_payment = accountant_received_payment.get_record::<ReceivedPayments>(idx);
        check_timestamp(before, received_payments1.timestamp, after);
        assert_eq!(
            received_payments,
            &ReceivedPayments {
                timestamp: received_payments.timestamp,
                payments: expected_transactions.transactions,
                new_start_block: new_start_block,
                response_skeleton_opt: Some(ResponseSkeleton {
                    client_id: 1234,
                    context_id: idx
                }),
            }
        );
})

It's a nice way of making the test less lengthy and easier to scan over.

let received_payments2 = accountant_received_payment.get_record::<ReceivedPayments>(1);
check_timestamp(before, received_payments2.timestamp, after);
assert_eq!(
received_payments2,
&ReceivedPayments {
timestamp: received_payments2.timestamp,
payments: expected_transactions2.transactions,
new_start_block: new_start_block_2,
response_skeleton_opt: Some(ResponseSkeleton {
client_id: 1234,
context_id: 2
}),
}
);
let received_payments3 = accountant_received_payment.get_record::<ReceivedPayments>(2);
check_timestamp(before, received_payments3.timestamp, after);
assert_eq!(
received_payments3,
&ReceivedPayments {
timestamp: received_payments3.timestamp,
payments: expected_transactions3.transactions,
new_start_block: new_start_block_3,
response_skeleton_opt: Some(ResponseSkeleton {
client_id: 1234,
context_id: 3
}),
}
);
let received_payments4 = accountant_received_payment.get_record::<ReceivedPayments>(3);
check_timestamp(before, received_payments4.timestamp, after);
assert_eq!(
received_payments4,
&ReceivedPayments {
timestamp: received_payments4.timestamp,
payments: expected_transactions4.transactions,
new_start_block: new_start_block_4,
response_skeleton_opt: Some(ResponseSkeleton {
client_id: 1234,
context_id: 4
}),
}
);
}

fn success_handler(
_bcb: &mut BlockchainBridge,
_msg: RetrieveTransactions,
Expand Down
Loading
Loading