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 the trimming of the record of the first block height of each epoch #1898

Closed
wants to merge 5 commits into from
Closed

Fix the trimming of the record of the first block height of each epoch #1898

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35a77bb
remove trimming of `pred_epochs`
brentstone Sep 18, 2023
04cb536
fix tests
brentstone Sep 18, 2023
7113b8b
remove unnecessary `Epochs` methods
brentstone Sep 19, 2023
49411db
changelog: #1898
brentstone Sep 19, 2023
4a83bbe
fixup! remove unnecessary `Epochs` methods
brentstone Sep 21, 2023
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
3 changes: 3 additions & 0 deletions .changelog/unreleased/bug-fixes/1898-fix-epoch-trimming.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- Keep a record of the first block heights of every epoch in the chain's history
instead of trimming to only keep this data for a certain number of epochs in
the past. ([\#1898](https://github.com/anoma/namada/pull/1898))
2 changes: 1 addition & 1 deletion apps/src/lib/node/ledger/shell/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2075,7 +2075,7 @@ mod test_utils {
.storage
.block
.pred_epochs
.new_epoch(BlockHeight(1), 1000);
.new_epoch(BlockHeight(1));
update_allowed_conversions(&mut shell.wl_storage)
.expect("update conversions failed");
shell.wl_storage.commit_block().expect("commit failed");
Expand Down
13 changes: 5 additions & 8 deletions apps/src/lib/node/ledger/storage/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ mod tests {
.write(&key, value_bytes.clone())
.expect("write failed");
storage.block.epoch = storage.block.epoch.next();
storage.block.pred_epochs.new_epoch(BlockHeight(100), 1000);
storage.block.pred_epochs.new_epoch(BlockHeight(100));
// make wl_storage to update conversion for a new epoch
let mut wl_storage = WlStorage::new(WriteLog::default(), storage);
update_allowed_conversions(&mut wl_storage)
Expand Down Expand Up @@ -420,10 +420,7 @@ mod tests {
if storage.block.height.0 % 5 == 0 {
// new epoch every 5 heights
storage.block.epoch = storage.block.epoch.next();
storage
.block
.pred_epochs
.new_epoch(storage.block.height, 1000);
storage.block.pred_epochs.new_epoch(storage.block.height);
}
storage.commit_block(batch)?;
let hash = BlockHash::default();
Expand Down Expand Up @@ -512,7 +509,7 @@ mod tests {
.expect("write failed");

storage.block.epoch = storage.block.epoch.next();
storage.block.pred_epochs.new_epoch(BlockHeight(1), 1000);
storage.block.pred_epochs.new_epoch(BlockHeight(1));
let batch = PersistentStorage::batch();
storage.commit_block(batch).expect("commit failed");

Expand All @@ -527,7 +524,7 @@ mod tests {
.expect("write failed");

storage.block.epoch = storage.block.epoch.next();
storage.block.pred_epochs.new_epoch(BlockHeight(6), 1000);
storage.block.pred_epochs.new_epoch(BlockHeight(6));
let batch = PersistentStorage::batch();
storage.commit_block(batch).expect("commit failed");

Expand All @@ -538,7 +535,7 @@ mod tests {
.begin_block(BlockHash::default(), BlockHeight(11))
.expect("begin_block failed");
storage.block.epoch = storage.block.epoch.next();
storage.block.pred_epochs.new_epoch(BlockHeight(11), 1000);
storage.block.pred_epochs.new_epoch(BlockHeight(11));
let batch = PersistentStorage::batch();
storage.commit_block(batch).expect("commit failed");

Expand Down
9 changes: 2 additions & 7 deletions core/src/ledger/storage/wl_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,8 @@ where
self.storage.next_epoch_min_start_height =
height + min_num_of_blocks;
self.storage.next_epoch_min_start_time = time + min_duration;
// TODO put this into PoS parameters and pass it to tendermint
// `consensus_params` on `InitChain` and `EndBlock`
let evidence_max_age_num_blocks: u64 = 100000;
self.storage
.block
.pred_epochs
.new_epoch(height, evidence_max_age_num_blocks);

self.storage.block.pred_epochs.new_epoch(height);
tracing::info!("Began a new epoch {}", self.storage.block.epoch);
}
Ok(new_epoch)
Expand Down
174 changes: 79 additions & 95 deletions core/src/types/storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1149,8 +1149,6 @@ impl Mul for Epoch {
BorshDeserialize,
)]
pub struct Epochs {
/// The oldest epoch we can look-up.
first_known_epoch: Epoch,
/// The block heights of the first block of each known epoch.
/// Invariant: the values must be sorted in ascending order.
pub first_block_heights: Vec<BlockHeight>,
Expand All @@ -1161,48 +1159,25 @@ impl Default for Epochs {
/// block height 0.
fn default() -> Self {
Self {
first_known_epoch: Epoch::default(),
first_block_heights: vec![BlockHeight::default()],
}
}
}

impl Epochs {
/// Record start of a new epoch at the given block height and trim any
/// epochs that ended more than `max_age_num_blocks` ago.
pub fn new_epoch(
&mut self,
block_height: BlockHeight,
max_age_num_blocks: u64,
) {
let min_block_height_to_keep = (block_height.0 + 1)
.checked_sub(max_age_num_blocks)
.unwrap_or_default();
// trim off any epochs whose last block is before the limit
while let Some((_first_known_epoch_height, rest)) =
self.first_block_heights.split_first()
{
if let Some(second_known_epoch_height) = rest.first() {
if second_known_epoch_height.0 < min_block_height_to_keep {
self.first_known_epoch = self.first_known_epoch.next();
self.first_block_heights = rest.to_vec();
continue;
}
}
break;
}
/// Record start of a new epoch at the given block height
pub fn new_epoch(&mut self, block_height: BlockHeight) {
self.first_block_heights.push(block_height);
}

/// Look-up the epoch of a given block height.
/// Look up the epoch of a given block height.
/// TODO: handle passing of a future block height? (only valid for current
/// or past)
Copy link
Member

Choose a reason for hiding this comment

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

We can't determine future epochs because epoch progression depends on 2 params - min. num of blocks and min duration. I think if the given block_height > current_height we can return None

Copy link
Member

Choose a reason for hiding this comment

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

ah, nvm, we don't have enough context here to check current height. The current behavior is that we return the current epoch, which is probably fine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I put this here because it doesn't rly make sense to pass a future block height into this, but figured if this function were to eventually also take in the current block height, we could return None perhaps

brentstone marked this conversation as resolved.
Show resolved Hide resolved
pub fn get_epoch(&self, block_height: BlockHeight) -> Option<Epoch> {
if let Some((first_known_epoch_height, rest)) =
if let Some((_first_known_epoch_height, rest)) =
self.first_block_heights.split_first()
{
if block_height < *first_known_epoch_height {
return None;
}
let mut epoch = self.first_known_epoch;
let mut epoch = Epoch::default();
for next_block_height in rest {
if block_height < *next_block_height {
return Some(epoch);
Expand All @@ -1215,7 +1190,7 @@ impl Epochs {
None
}

/// Look-up the starting block height of an epoch at or before a given
/// Look up the starting block height of an epoch at or before a given
/// height.
pub fn get_epoch_start_height(
&self,
Expand All @@ -1229,32 +1204,15 @@ impl Epochs {
None
}

/// Look-up the starting block height of the given epoch
/// Look up the starting block height of the given epoch
pub fn get_start_height_of_epoch(
&self,
epoch: Epoch,
) -> Option<BlockHeight> {
if epoch < self.first_known_epoch {
if epoch.0 > self.first_block_heights.len() as u64 {
return None;
}

let mut cur_epoch = self.first_known_epoch;
for height in &self.first_block_heights {
if epoch == cur_epoch {
return Some(*height);
} else {
cur_epoch = cur_epoch.next();
}
}
None
}

/// Look-up the block height of a given epoch.
pub fn get_height(&self, epoch: Epoch) -> Option<BlockHeight> {
// the given epoch should be greater than or equal to the
// first known epoch
let index = epoch.0.checked_sub(self.first_known_epoch.0)? as usize;
self.first_block_heights.get(index).copied()
self.first_block_heights.get(epoch.0 as usize).copied()
}

/// Return all starting block heights for each successive Epoch.
Expand Down Expand Up @@ -1657,14 +1615,19 @@ mod tests {
fn test_predecessor_epochs_and_heights() {
let mut epochs = Epochs::default();
println!("epochs {:#?}", epochs);
assert_eq!(epochs.get_height(Epoch(0)), Some(BlockHeight(0)));
assert_eq!(
epochs.get_start_height_of_epoch(Epoch(0)),
Some(BlockHeight(0))
);
assert_eq!(epochs.get_epoch(BlockHeight(0)), Some(Epoch(0)));
let mut max_age_num_blocks = 100;

// epoch 1
epochs.new_epoch(BlockHeight(10), max_age_num_blocks);
epochs.new_epoch(BlockHeight(10));
println!("epochs {:#?}", epochs);
assert_eq!(epochs.get_height(Epoch(1)), Some(BlockHeight(10)));
assert_eq!(
epochs.get_start_height_of_epoch(Epoch(1)),
Some(BlockHeight(10))
);
assert_eq!(epochs.get_epoch(BlockHeight(0)), Some(Epoch(0)));
assert_eq!(
epochs.get_epoch_start_height(BlockHeight(0)),
Expand Down Expand Up @@ -1692,9 +1655,12 @@ mod tests {
);

// epoch 2
epochs.new_epoch(BlockHeight(20), max_age_num_blocks);
epochs.new_epoch(BlockHeight(20));
println!("epochs {:#?}", epochs);
assert_eq!(epochs.get_height(Epoch(2)), Some(BlockHeight(20)));
assert_eq!(
epochs.get_start_height_of_epoch(Epoch(2)),
Some(BlockHeight(20))
);
assert_eq!(epochs.get_epoch(BlockHeight(0)), Some(Epoch(0)));
assert_eq!(epochs.get_epoch(BlockHeight(9)), Some(Epoch(0)));
assert_eq!(epochs.get_epoch(BlockHeight(10)), Some(Epoch(1)));
Expand All @@ -1714,14 +1680,17 @@ mod tests {
Some(BlockHeight(20))
);

// epoch 3, epoch 0 and 1 should be trimmed
epochs.new_epoch(BlockHeight(200), max_age_num_blocks);
// epoch 3
epochs.new_epoch(BlockHeight(200));
println!("epochs {:#?}", epochs);
assert_eq!(epochs.get_height(Epoch(3)), Some(BlockHeight(200)));
assert_eq!(epochs.get_epoch(BlockHeight(0)), None);
assert_eq!(epochs.get_epoch(BlockHeight(9)), None);
assert_eq!(epochs.get_epoch(BlockHeight(10)), None);
assert_eq!(epochs.get_epoch(BlockHeight(11)), None);
assert_eq!(
epochs.get_start_height_of_epoch(Epoch(3)),
Some(BlockHeight(200))
);
assert_eq!(epochs.get_epoch(BlockHeight(0)), Some(Epoch(0)));
assert_eq!(epochs.get_epoch(BlockHeight(9)), Some(Epoch(0)));
assert_eq!(epochs.get_epoch(BlockHeight(10)), Some(Epoch(1)));
assert_eq!(epochs.get_epoch(BlockHeight(11)), Some(Epoch(1)));
assert_eq!(epochs.get_epoch(BlockHeight(20)), Some(Epoch(2)));
assert_eq!(epochs.get_epoch(BlockHeight(100)), Some(Epoch(2)));
assert_eq!(
Expand All @@ -1734,62 +1703,77 @@ mod tests {
Some(BlockHeight(200))
);

// increase the limit
max_age_num_blocks = 200;

// epoch 4
epochs.new_epoch(BlockHeight(300), max_age_num_blocks);
epochs.new_epoch(BlockHeight(300));
println!("epochs {:#?}", epochs);
assert_eq!(epochs.get_height(Epoch(4)), Some(BlockHeight(300)));
assert_eq!(
epochs.get_start_height_of_epoch(Epoch(4)),
Some(BlockHeight(300))
);
assert_eq!(epochs.get_epoch(BlockHeight(20)), Some(Epoch(2)));
assert_eq!(epochs.get_epoch(BlockHeight(100)), Some(Epoch(2)));
assert_eq!(epochs.get_epoch(BlockHeight(200)), Some(Epoch(3)));
assert_eq!(epochs.get_epoch(BlockHeight(300)), Some(Epoch(4)));

// epoch 5, epoch 2 should be trimmed
epochs.new_epoch(BlockHeight(499), max_age_num_blocks);
// epoch 5
epochs.new_epoch(BlockHeight(499));
println!("epochs {:#?}", epochs);
assert_eq!(epochs.get_height(Epoch(5)), Some(BlockHeight(499)));
assert_eq!(epochs.get_epoch(BlockHeight(20)), None);
assert_eq!(epochs.get_epoch(BlockHeight(100)), None);
assert_eq!(
epochs.get_start_height_of_epoch(Epoch(5)),
Some(BlockHeight(499))
);
assert_eq!(epochs.get_epoch(BlockHeight(20)), Some(Epoch(2)));
assert_eq!(epochs.get_epoch(BlockHeight(100)), Some(Epoch(2)));
assert_eq!(epochs.get_epoch(BlockHeight(200)), Some(Epoch(3)));
assert_eq!(epochs.get_epoch(BlockHeight(300)), Some(Epoch(4)));
assert_eq!(epochs.get_epoch(BlockHeight(499)), Some(Epoch(5)));

// epoch 6, epoch 3 should be trimmed
epochs.new_epoch(BlockHeight(500), max_age_num_blocks);
// epoch 6
epochs.new_epoch(BlockHeight(500));
println!("epochs {:#?}", epochs);
assert_eq!(epochs.get_height(Epoch(6)), Some(BlockHeight(500)));
assert_eq!(epochs.get_epoch(BlockHeight(200)), None);
assert_eq!(
epochs.get_start_height_of_epoch(Epoch(6)),
Some(BlockHeight(500))
);
assert_eq!(epochs.get_epoch(BlockHeight(200)), Some(Epoch(3)));
assert_eq!(epochs.get_epoch(BlockHeight(300)), Some(Epoch(4)));
assert_eq!(epochs.get_epoch(BlockHeight(499)), Some(Epoch(5)));
assert_eq!(epochs.get_epoch(BlockHeight(500)), Some(Epoch(6)));

// decrease the limit
max_age_num_blocks = 50;

// epoch 7, epoch 4 and 5 should be trimmed
epochs.new_epoch(BlockHeight(550), max_age_num_blocks);
// epoch 7
epochs.new_epoch(BlockHeight(550));
println!("epochs {:#?}", epochs);
assert_eq!(epochs.get_height(Epoch(7)), Some(BlockHeight(550)));
assert_eq!(epochs.get_epoch(BlockHeight(300)), None);
assert_eq!(epochs.get_epoch(BlockHeight(499)), None);
assert_eq!(
epochs.get_start_height_of_epoch(Epoch(7)),
Some(BlockHeight(550))
);
assert_eq!(epochs.get_epoch(BlockHeight(300)), Some(Epoch(4)));
assert_eq!(epochs.get_epoch(BlockHeight(499)), Some(Epoch(5)));
assert_eq!(epochs.get_epoch(BlockHeight(500)), Some(Epoch(6)));
assert_eq!(epochs.get_epoch(BlockHeight(550)), Some(Epoch(7)));

// epoch 8, epoch 6 should be trimmed
epochs.new_epoch(BlockHeight(600), max_age_num_blocks);
// epoch 8
epochs.new_epoch(BlockHeight(600));
println!("epochs {:#?}", epochs);
assert_eq!(epochs.get_height(Epoch(7)), Some(BlockHeight(550)));
assert_eq!(epochs.get_height(Epoch(8)), Some(BlockHeight(600)));
assert_eq!(epochs.get_epoch(BlockHeight(500)), None);
assert_eq!(
epochs.get_start_height_of_epoch(Epoch(7)),
Some(BlockHeight(550))
);
assert_eq!(
epochs.get_start_height_of_epoch(Epoch(8)),
Some(BlockHeight(600))
);
assert_eq!(epochs.get_epoch(BlockHeight(500)), Some(Epoch(6)));
assert_eq!(epochs.get_epoch(BlockHeight(550)), Some(Epoch(7)));
assert_eq!(epochs.get_epoch(BlockHeight(600)), Some(Epoch(8)));

// try to fetch height values out of range
// at this point, the min known epoch is 7
for e in [1, 2, 3, 4, 5, 6, 9, 10, 11, 12] {
assert!(epochs.get_height(Epoch(e)).is_none(), "Epoch: {e}");
for e in [9, 10, 11, 12] {
assert!(
epochs.get_start_height_of_epoch(Epoch(e)).is_none(),
"Epoch: {e}"
);
}
}

Expand Down
2 changes: 1 addition & 1 deletion ethereum_bridge/src/protocol/transactions/validator_set_update/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ where
.storage
.block
.pred_epochs
.get_height(signing_epoch)
.get_start_height_of_epoch(signing_epoch)
// NOTE: The only way this can fail is if validator set updates do not
// reach a `seen` state before the relevant epoch data is purged from
// Namada. In most scenarios, we should reach a complete proof before
Expand Down
6 changes: 5 additions & 1 deletion proof_of_stake/src/pos_queries.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,11 @@ where
/// been purged from Namada, or if it is not available yet.
#[inline]
pub fn get_height(self, epoch: Epoch) -> Option<BlockHeight> {
self.wl_storage.storage.block.pred_epochs.get_height(epoch)
self.wl_storage
.storage
.block
.pred_epochs
.get_start_height_of_epoch(epoch)
}

/// Retrieves the [`BlockHeight`] that is currently being decided.
Expand Down