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

Storage: remove get_range #359

Merged
merged 6 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
13 changes: 8 additions & 5 deletions storage/blockchain/src/ops/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use monero_serai::{
};

use cuprate_database::{
DbResult, RuntimeError, StorableVec, {DatabaseIter, DatabaseRo, DatabaseRw},
DbResult, RuntimeError, StorableVec, {DatabaseRo, DatabaseRw},
};
use cuprate_helper::cast::usize_to_u64;
use cuprate_helper::{
Expand Down Expand Up @@ -268,10 +268,13 @@ pub fn get_block_complete_entry(

let first_tx_idx = miner_tx_idx + 1;

let tx_blobs = tables
.tx_blobs_iter()
.get_range(first_tx_idx..(usize_to_u64(numb_non_miner_txs) + first_tx_idx))?
.map(|tx_blob| Ok(Bytes::from(tx_blob?.0)))
let tx_blobs = (first_tx_idx..(usize_to_u64(numb_non_miner_txs) + first_tx_idx))
.into_iter()
.map(|idx| {
let tx_blob = tables.tx_blobs().get(&idx)?.0;

Ok(Bytes::from(tx_blob))
})
.collect::<Result<_, RuntimeError>>()?;

Ok(BlockCompleteEntry {
Expand Down
12 changes: 5 additions & 7 deletions storage/blockchain/src/service/read.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ use rayon::{
};
use thread_local::ThreadLocal;

use cuprate_database::{
ConcreteEnv, DatabaseIter, DatabaseRo, DbResult, Env, EnvInner, RuntimeError,
};
use cuprate_database::{ConcreteEnv, DatabaseRo, DbResult, Env, EnvInner, RuntimeError};
use cuprate_database_service::{init_thread_pool, DatabaseReadService, ReaderThreads};
use cuprate_helper::map::combine_low_high_bits_to_u128;
use cuprate_types::{
Expand Down Expand Up @@ -616,10 +614,10 @@ fn next_chain_entry(
let chain_height = crate::ops::blockchain::chain_height(table_block_heights)?;
let last_height_in_chain_entry = min(first_known_height + next_entry_size, chain_height);

let (block_ids, block_weights) = table_block_infos
.get_range(first_known_height..last_height_in_chain_entry)?
.map(|block_info| {
let block_info = block_info?;
let (block_ids, block_weights) = (first_known_height..last_height_in_chain_entry)
.into_iter()
.map(|height| {
let block_info = table_block_infos.get(&height)?;
Copy link
Member

Choose a reason for hiding this comment

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

That is a temporary fix right?

Copy link
Member Author

Choose a reason for hiding this comment

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

should be, yes


Ok((block_info.block_hash, block_info.weight))
})
Expand Down
5 changes: 4 additions & 1 deletion storage/database/src/backend/heed/database.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Implementation of `trait Database` for `heed`.

//---------------------------------------------------------------------------------------------------- Import
use std::{cell::RefCell, ops::RangeBounds};
use std::cell::RefCell;

use crate::{
backend::heed::types::HeedDb,
Expand Down Expand Up @@ -90,6 +90,7 @@ fn is_empty<T: Table>(db: &HeedDb<T::Key, T::Value>, tx_ro: &heed::RoTxn<'_>) ->

//---------------------------------------------------------------------------------------------------- DatabaseIter Impl
impl<T: Table> DatabaseIter<T> for HeedTableRo<'_, T> {
/*
#[inline]
fn get_range<'a, Range>(
&'a self,
Expand All @@ -101,6 +102,8 @@ impl<T: Table> DatabaseIter<T> for HeedTableRo<'_, T> {
Ok(self.db.range(self.tx_ro, &range)?.map(|res| Ok(res?.1)))
}

*/

#[inline]
fn iter(&self) -> DbResult<impl Iterator<Item = DbResult<(T::Key, T::Value)>> + '_> {
Ok(self.db.iter(self.tx_ro)?.map(|res| Ok(res?)))
Expand Down
3 changes: 3 additions & 0 deletions storage/database/src/backend/redb/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ fn is_empty<T: Table>(

//---------------------------------------------------------------------------------------------------- DatabaseIter
impl<T: Table + 'static> DatabaseIter<T> for RedbTableRo<T::Key, T::Value> {
/*
#[inline]
fn get_range<'a, Range>(
&'a self,
Expand All @@ -79,6 +80,8 @@ impl<T: Table + 'static> DatabaseIter<T> for RedbTableRo<T::Key, T::Value> {
}))
}

*/

#[inline]
fn iter(&self) -> DbResult<impl Iterator<Item = DbResult<(T::Key, T::Value)>> + '_> {
Ok(ReadableTable::iter(self)?.map(|result| {
Expand Down
11 changes: 4 additions & 7 deletions storage/database/src/backend/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ fn db_read_write() {

// Assert the whole range is there.
{
let range = table_ro.get_range(..).unwrap();
let range = table_ro.values().unwrap();
let mut i = 0;
for result in range {
let value = result.unwrap();
Expand All @@ -241,22 +241,19 @@ fn db_read_write() {
let range = KEY..key;

// Assert count is correct.
assert_eq!(
N as usize,
table_ro.get_range(range.clone()).unwrap().count()
);
assert_eq!(N as usize, table_ro.values().unwrap().count());

// Assert each returned value from the iterator is owned.
{
let mut iter = table_ro.get_range(range.clone()).unwrap();
let mut iter = table_ro.values().unwrap();
let value = iter.next().unwrap().unwrap(); // 1. take value out
drop(iter); // 2. drop the `impl Iterator + 'a`
assert_value(value); // 3. assert even without the iterator, the value is alive
}

// Assert each value is the same.
{
let mut iter = table_ro.get_range(range).unwrap();
let mut iter = table_ro.values().unwrap();
for _ in 0..N {
let value = iter.next().unwrap().unwrap();
assert_value(value);
Expand Down
7 changes: 5 additions & 2 deletions storage/database/src/database.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
//! Abstracted database table operations; `trait DatabaseRo` & `trait DatabaseRw`.

//---------------------------------------------------------------------------------------------------- Import
use std::ops::RangeBounds;

use crate::{
error::{DbResult, RuntimeError},
table::Table,
Expand Down Expand Up @@ -35,6 +33,9 @@ Each iteration of the iterator has the potential to error as well."
/// - <https://github.com/Cuprate/cuprate/pull/102#discussion_r1548695610>
/// - <https://github.com/Cuprate/cuprate/pull/104>
pub trait DatabaseIter<T: Table> {
/*
FIXME: <https://github.com/Cuprate/cuprate/issues/348>
Comment on lines +36 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth leaving this as-is then where used in cuprate_blockchain it branches on the backend? E.g.:

let tx_blobs = if cfg!(feature = "heed") {
    (first_tx_idx..(usize_to_u64(numb_non_miner_txs) + first_tx_idx))
        .map(|idx| {
            let tx_blob = tables.tx_blobs().get(&idx)?.0;

            Ok(Bytes::from(tx_blob))
        })
        .collect::<Result<_, RuntimeError>>()?
} else {
    tables
        .tx_blobs_iter()
        .get_range(first_tx_idx..(usize_to_u64(numb_non_miner_txs) + first_tx_idx))?
        .map(|tx_blob| Ok(Bytes::from(tx_blob?.0)))
        .collect::<Result<_, RuntimeError>>()?
};

Or is it not that important for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or actually, the DatabaseIter::get_range impl for heed would just use get internally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or actually, the DatabaseIter::get_range impl for heed would just use get internally.

I tried to do this but it's harder than it sounds. In get_range we don't know how much of the range we actually have, i.e if we ask for 0..100 but we only have the key 50 stored we still need to check the other 99 keys, not impossible but defiantly a footgun IMO.

Would it be worth leaving this as-is then where used in cuprate_blockchain it branches on the backend?

I don't think the complexity is worth it. This is only used for other nodes syncing from us, which from some initial tests, is still significantly faster than monerod


/// Get an [`Iterator`] of value's corresponding to a range of keys.
///
/// For example:
Expand All @@ -55,6 +56,8 @@ pub trait DatabaseIter<T: Table> {
where
Range: RangeBounds<T::Key> + 'a;

*/

/// Get an [`Iterator`] that returns the `(key, value)` types for this database.
#[doc = doc_iter!()]
#[expect(clippy::iter_not_returning_iterator)]
Expand Down
Loading