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.

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

fix: u64->i64->u64 conversion; chain split height as u64 #3442

Merged
merged 8 commits into from
Oct 11, 2021
Merged

fix: u64->i64->u64 conversion; chain split height as u64 #3442

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f479204
fix auto update tests, reduced timeout
Impala123 Oct 5, 2021
bc659b4
Merge branch 'development' into development
aviator-app[bot] Oct 5, 2021
f1247e6
Merge branch 'tari-project:development' into development
Impala123 Oct 5, 2021
963a1d3
Merge branch 'tari-project:development' into development
Impala123 Oct 9, 2021
7185d64
fix: avoid cast to i64 and back to u64
Impala123 Oct 9, 2021
5b11048
fix: use u64 for FindChainSplitResponse and related errors
Impala123 Oct 9, 2021
30a1688
check under & overflows
Impala123 Oct 11, 2021
6cf061f
Merge branch 'development' into development
aviator-app[bot] Oct 11, 2021
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: 2 additions & 2 deletions applications/tari_base_node/src/grpc/base_node_grpc_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,15 +326,15 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer {

let headers: Vec<u64> = if request.from_height != 0 {
match sorting {
Sorting::Desc => ((cmp::max(0, request.from_height as i64 - num_headers as i64 + 1) as u64)..=
Sorting::Desc => (cmp::max(0, request.from_height - num_headers + 1)..=
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there not a chance for an underflow here?

Copy link
Contributor

@delta1 delta1 Oct 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This needs a covering test, even if this logic is moved into a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this then be

let headers: Vec<u64> = if request.from_height >= num_headers {

num_headers is forced to be between 10 and 10_000
Means that from_height being 0 would go to the else

Or is it more desirable to check the underflow flag with overflowing_sub() ?

request.from_height)
.rev()
.collect(),
Sorting::Asc => (request.from_height..(request.from_height + num_headers)).collect(),
}
} else {
match sorting {
Sorting::Desc => ((cmp::max(0, tip as i64 - num_headers as i64 + 1) as u64)..=tip)
Sorting::Desc => (cmp::max(0, tip - num_headers + 1)..=tip)
.rev()
.collect(),
Sorting::Asc => (0..num_headers).collect(),
Expand Down
2 changes: 1 addition & 1 deletion base_layer/core/src/base_node/proto/rpc.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ message FindChainSplitResponse {
// An ordered list of headers starting from next header after the matching hash, up until `FindChainSplitRequest::count`
repeated tari.core.BlockHeader headers = 1;
// The index of the hash that matched from `FindChainSplitRequest::block_hashes`. This value could also be used to know how far back a split occurs.
uint32 fork_hash_index = 2;
uint64 fork_hash_index = 2;
/// The current header height of this node
uint64 tip_height = 3;
}
Expand Down
2 changes: 1 addition & 1 deletion base_layer/core/src/base_node/sync/header_sync/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub enum BlockHeaderSyncError {
#[error("Sync failed for all peers")]
SyncFailedAllPeers,
#[error("Peer sent a found hash index that was out of range (Expected less than {0}, Found: {1})")]
FoundHashIndexOutOfRange(u32, u32),
FoundHashIndexOutOfRange(u64, u64),
#[error("Failed to ban peer: {0}")]
FailedToBan(ConnectivityError),
#[error("Connectivity Error: {0}")]
Expand Down
6 changes: 3 additions & 3 deletions base_layer/core/src/base_node/sync/header_sync/synchronizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,15 +374,15 @@ impl<'a, B: BlockchainBackend + 'static> HeaderSynchronizer<'a, B> {
peer
);

if fork_hash_index >= block_hashes.len() as u32 {
if fork_hash_index >= block_hashes.len() as u64 {
let _ = self
.ban_peer_long(peer.clone(), BanReason::SplitHashGreaterThanHashes {
fork_hash_index,
num_block_hashes: block_hashes.len(),
})
.await;
return Err(BlockHeaderSyncError::FoundHashIndexOutOfRange(
block_hashes.len() as u32,
block_hashes.len() as u64,
fork_hash_index,
));
}
Expand Down Expand Up @@ -658,7 +658,7 @@ enum BanReason {
({num_block_hashes})"
)]
SplitHashGreaterThanHashes {
fork_hash_index: u32,
fork_hash_index: u64,
num_block_hashes: usize,
},
#[error("Peer sent invalid header: {0}")]
Expand Down
2 changes: 1 addition & 1 deletion base_layer/core/src/base_node/sync/rpc/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ impl<B: BlockchainBackend + 'static> BaseNodeSyncService for BaseNodeSyncRpcServ
.map_err(RpcStatus::log_internal_error(LOG_TARGET))?;

Ok(Response::new(FindChainSplitResponse {
fork_hash_index: idx as u32,
fork_hash_index: idx as u64,
headers: headers.into_iter().map(Into::into).collect(),
tip_height: metadata.height_of_longest_chain(),
}))
Expand Down