Skip to content

Commit

Permalink
fix: remove unbounded vec allocations from base node grpc/p2p messagi…
Browse files Browse the repository at this point in the history
…ng (#3467)

Description
---
- replaces unbounded height lists with bound height ranges in internal base node service
  interfaces and p2p messaging
- removes unused and deprecated base node p2p messaging requests/responses
- load chain headers when requesting headers, this simplifies and
  reduces the database load when calling grpc get_blocks
- use iterator with almost no footprint for paging in grpc calls
- implement `DoubleSidedIterator` for `NonOverlappingIntegerPairIter`
- add overflow protection to `NonOverlappingIntegerPairIter` and
  additional tests

This PR does not contain any breaking changes, as no nodes using the base node p2p messaging interface except for block propagation (which is unchanged). GRPC protobuf contract is preserved.

**New dependency**
`either = "1.6.1"` added to `tari_base_node` crate and used for its `Either` iterator impl

Motivation and Context
---
- removing unused "outbound comms interface" request/responses reduces attack surface (TODO: remove this interface entirely - specifically the coupling for internal service requests and external p2p comms is troublesome, but mostly removed in the PR, but removing it entirely will make making changes to the base node service much easier)
- remote peers could request any number of heights allowing an external party to allocate memory without bounds 

Closes #3310 

How Has This Been Tested?
---
Existing GRPC tests
Manually tested block explorer api connected to local base node
Additional unit tests
  • Loading branch information
sdbondi committed Oct 19, 2021
1 parent a05ac5e commit 5d7fb20
Show file tree
Hide file tree
Showing 25 changed files with 429 additions and 974 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions applications/tari_base_node/Cargo.toml
Expand Up @@ -25,6 +25,7 @@ anyhow = "1.0.32"
bincode = "1.3.1"
chrono = "0.4"
config = { version = "0.9.3" }
either = "1.6.1"
futures = { version = "^0.3.16", default-features = false, features = ["alloc"] }
log = { version = "0.4.8", features = ["std"] }
num_cpus = "1"
Expand Down
70 changes: 30 additions & 40 deletions applications/tari_base_node/src/command_handler.rs
Expand Up @@ -131,14 +131,9 @@ impl CommandHandler {
status_line.add_field("State", state_info.borrow().state_info.short_desc());

let metadata = node.get_metadata().await.unwrap();

let last_header = node
.get_headers(vec![metadata.height_of_longest_chain()])
.await
.unwrap()
.pop()
.unwrap();
let last_block_time = DateTime::<Utc>::from(last_header.timestamp);
let height = metadata.height_of_longest_chain();
let last_header = node.get_header(height).await.unwrap().unwrap();
let last_block_time = DateTime::<Utc>::from(last_header.header().timestamp);
status_line.add_field(
"Tip",
format!(
Expand Down Expand Up @@ -870,21 +865,20 @@ impl CommandHandler {
io::stdout().flush().unwrap();
// we can only check till the pruning horizon, 0 is archive node so it needs to check every block.
if height > horizon_height {
match node.get_blocks(vec![height]).await {
Err(_err) => {
missing_blocks.push(height);
},
Ok(mut data) => match data.pop() {
// We need to check the data it self, as FetchMatchingBlocks will suppress any error, only
match node.get_block(height).await {
Err(err) => {
// We need to check the data itself, as FetchMatchingBlocks will suppress any error, only
// logging it.
Some(_historical_block) => {},
None => missing_blocks.push(height),
error!(target: LOG_TARGET, "{}", err);
missing_blocks.push(height);
},
Ok(Some(_)) => {},
Ok(None) => missing_blocks.push(height),
};
}
height -= 1;
let next_header = node.get_headers(vec![height]).await;
if next_header.is_err() {
let next_header = node.get_header(height).await.ok().flatten();
if next_header.is_none() {
// this header is missing, so we stop here and need to ask for this header
missing_headers.push(height);
};
Expand Down Expand Up @@ -921,34 +915,30 @@ impl CommandHandler {
print!("{}", height);
io::stdout().flush().unwrap();

let block = match node.get_blocks(vec![height]).await {
Err(_err) => {
println!("Error in db, could not get block");
let block = match node.get_block(height).await {
Err(err) => {
println!("Error in db, could not get block: {}", err);
break;
},
Ok(mut data) => match data.pop() {
// We need to check the data it self, as FetchMatchingBlocks will suppress any error, only
// logging it.
Some(historical_block) => historical_block,
None => {
println!("Error in db, could not get block");
break;
},
// We need to check the data it self, as FetchMatchingBlocks will suppress any error, only
// logging it.
Ok(Some(historical_block)) => historical_block,
Ok(None) => {
println!("Error in db, block not found at height {}", height);
break;
},
};
let prev_block = match node.get_blocks(vec![height - 1]).await {
Err(_err) => {
println!("Error in db, could not get block");
let prev_block = match node.get_block(height - 1).await {
Err(err) => {
println!("Error in db, could not get block: {}", err);
break;
},
Ok(mut data) => match data.pop() {
// We need to check the data it self, as FetchMatchingBlocks will suppress any error, only
// logging it.
Some(historical_block) => historical_block,
None => {
println!("Error in db, could not get block");
break;
},
// We need to check the data it self, as FetchMatchingBlocks will suppress any error, only
// logging it.
Ok(Some(historical_block)) => historical_block,
Ok(None) => {
println!("Error in db, block not found at height {}", height - 1);
break;
},
};
height -= 1;
Expand Down

0 comments on commit 5d7fb20

Please sign in to comment.