C. Restore `self` ownership in `Chain::push` method (#3826)
* Drop chain if it becomes invalid Avoid returning a chain that shouldn't be used again by having the method take ownership of `self` and dropping it on error. * Update documentation and comments Restore them to be closer to how they were on `main`.
This commit is contained in:
parent
aadc07a820
commit
b6a0fcc44c
|
@ -204,7 +204,7 @@ impl NonFinalizedState {
|
||||||
#[tracing::instrument(level = "debug", skip(self, finalized_state, new_chain))]
|
#[tracing::instrument(level = "debug", skip(self, finalized_state, new_chain))]
|
||||||
fn validate_and_commit(
|
fn validate_and_commit(
|
||||||
&self,
|
&self,
|
||||||
mut new_chain: Arc<Chain>,
|
new_chain: Arc<Chain>,
|
||||||
prepared: PreparedBlock,
|
prepared: PreparedBlock,
|
||||||
finalized_state: &FinalizedState,
|
finalized_state: &FinalizedState,
|
||||||
) -> Result<Arc<Chain>, ValidateContextError> {
|
) -> Result<Arc<Chain>, ValidateContextError> {
|
||||||
|
@ -243,9 +243,10 @@ impl NonFinalizedState {
|
||||||
|
|
||||||
// We're pretty sure the new block is valid,
|
// We're pretty sure the new block is valid,
|
||||||
// so clone the inner chain if needed, then add the new block.
|
// so clone the inner chain if needed, then add the new block.
|
||||||
Arc::make_mut(&mut new_chain).push(contextual)?;
|
Arc::try_unwrap(new_chain)
|
||||||
|
.unwrap_or_else(|shared_chain| (*shared_chain).clone())
|
||||||
Ok(new_chain)
|
.push(contextual)
|
||||||
|
.map(Arc::new)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Returns the length of the non-finalized portion of the current best chain.
|
/// Returns the length of the non-finalized portion of the current best chain.
|
||||||
|
|
|
@ -189,31 +189,19 @@ impl Chain {
|
||||||
|
|
||||||
/// Push a contextually valid non-finalized block into this chain as the new tip.
|
/// Push a contextually valid non-finalized block into this chain as the new tip.
|
||||||
///
|
///
|
||||||
/// If the block is invalid, clears the chain, and returns an error.
|
/// If the block is invalid, drops this chain, and returns an error.
|
||||||
///
|
///
|
||||||
/// Note: a [`ContextuallyValidBlock`] isn't actually contextually valid until
|
/// Note: a [`ContextuallyValidBlock`] isn't actually contextually valid until
|
||||||
/// [`update_chain_state_with`] returns success.
|
/// [`update_chain_state_with`] returns success.
|
||||||
#[instrument(level = "debug", skip(self, block), fields(block = %block.block))]
|
#[instrument(level = "debug", skip(self, block), fields(block = %block.block))]
|
||||||
pub fn push(&mut self, block: ContextuallyValidBlock) -> Result<(), ValidateContextError> {
|
pub fn push(mut self, block: ContextuallyValidBlock) -> Result<Chain, ValidateContextError> {
|
||||||
// update cumulative data members
|
// update cumulative data members
|
||||||
if let Err(error) = self.update_chain_tip_with(&block) {
|
self.update_chain_tip_with(&block)?;
|
||||||
// The chain could be in an invalid half-updated state, so clear its data.
|
|
||||||
*self = Chain::new(
|
|
||||||
self.network,
|
|
||||||
sprout::tree::NoteCommitmentTree::default(),
|
|
||||||
sapling::tree::NoteCommitmentTree::default(),
|
|
||||||
orchard::tree::NoteCommitmentTree::default(),
|
|
||||||
HistoryTree::default(),
|
|
||||||
ValueBalance::zero(),
|
|
||||||
);
|
|
||||||
|
|
||||||
return Err(error);
|
|
||||||
}
|
|
||||||
|
|
||||||
tracing::debug!(block = %block.block, "adding block to chain");
|
tracing::debug!(block = %block.block, "adding block to chain");
|
||||||
self.blocks.insert(block.height, block);
|
self.blocks.insert(block.height, block);
|
||||||
|
|
||||||
Ok(())
|
Ok(self)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Remove the lowest height block of the non-finalized portion of a chain.
|
/// Remove the lowest height block of the non-finalized portion of a chain.
|
||||||
|
|
|
@ -64,7 +64,7 @@ fn push_genesis_chain() -> Result<()> {
|
||||||
|
|
||||||
chain_values.insert(block.height.into(), (block.chain_value_pool_change.into(), None));
|
chain_values.insert(block.height.into(), (block.chain_value_pool_change.into(), None));
|
||||||
|
|
||||||
only_chain
|
only_chain = only_chain
|
||||||
.push(block.clone())
|
.push(block.clone())
|
||||||
.map_err(|e| (e, chain_values.clone()))
|
.map_err(|e| (e, chain_values.clone()))
|
||||||
.expect("invalid chain value pools");
|
.expect("invalid chain value pools");
|
||||||
|
@ -105,7 +105,7 @@ fn push_history_tree_chain() -> Result<()> {
|
||||||
.iter()
|
.iter()
|
||||||
.take(count)
|
.take(count)
|
||||||
.map(ContextuallyValidBlock::test_with_zero_chain_pool_change) {
|
.map(ContextuallyValidBlock::test_with_zero_chain_pool_change) {
|
||||||
only_chain.push(block)?;
|
only_chain = only_chain.push(block)?;
|
||||||
}
|
}
|
||||||
|
|
||||||
prop_assert_eq!(only_chain.blocks.len(), count);
|
prop_assert_eq!(only_chain.blocks.len(), count);
|
||||||
|
@ -154,7 +154,7 @@ fn forked_equals_pushed_genesis() -> Result<()> {
|
||||||
block,
|
block,
|
||||||
partial_chain.unspent_utxos(),
|
partial_chain.unspent_utxos(),
|
||||||
)?;
|
)?;
|
||||||
partial_chain
|
partial_chain = partial_chain
|
||||||
.push(block)
|
.push(block)
|
||||||
.expect("partial chain push is valid");
|
.expect("partial chain push is valid");
|
||||||
}
|
}
|
||||||
|
@ -171,7 +171,7 @@ fn forked_equals_pushed_genesis() -> Result<()> {
|
||||||
for block in chain.iter().cloned() {
|
for block in chain.iter().cloned() {
|
||||||
let block =
|
let block =
|
||||||
ContextuallyValidBlock::with_block_and_spent_utxos(block, full_chain.unspent_utxos())?;
|
ContextuallyValidBlock::with_block_and_spent_utxos(block, full_chain.unspent_utxos())?;
|
||||||
full_chain
|
full_chain = full_chain
|
||||||
.push(block.clone())
|
.push(block.clone())
|
||||||
.expect("full chain push is valid");
|
.expect("full chain push is valid");
|
||||||
|
|
||||||
|
@ -221,7 +221,7 @@ fn forked_equals_pushed_genesis() -> Result<()> {
|
||||||
for block in chain.iter().skip(fork_at_count).cloned() {
|
for block in chain.iter().skip(fork_at_count).cloned() {
|
||||||
let block =
|
let block =
|
||||||
ContextuallyValidBlock::with_block_and_spent_utxos(block, forked.unspent_utxos())?;
|
ContextuallyValidBlock::with_block_and_spent_utxos(block, forked.unspent_utxos())?;
|
||||||
forked.push(block).expect("forked chain push is valid");
|
forked = forked.push(block).expect("forked chain push is valid");
|
||||||
}
|
}
|
||||||
|
|
||||||
prop_assert_eq!(forked.blocks.len(), full_chain.blocks.len());
|
prop_assert_eq!(forked.blocks.len(), full_chain.blocks.len());
|
||||||
|
@ -261,13 +261,13 @@ fn forked_equals_pushed_history_tree() -> Result<()> {
|
||||||
.iter()
|
.iter()
|
||||||
.take(fork_at_count)
|
.take(fork_at_count)
|
||||||
.map(ContextuallyValidBlock::test_with_zero_chain_pool_change) {
|
.map(ContextuallyValidBlock::test_with_zero_chain_pool_change) {
|
||||||
partial_chain.push(block)?;
|
partial_chain = partial_chain.push(block)?;
|
||||||
}
|
}
|
||||||
|
|
||||||
for block in chain
|
for block in chain
|
||||||
.iter()
|
.iter()
|
||||||
.map(ContextuallyValidBlock::test_with_zero_chain_pool_change) {
|
.map(ContextuallyValidBlock::test_with_zero_chain_pool_change) {
|
||||||
full_chain.push(block.clone())?;
|
full_chain = full_chain.push(block.clone())?;
|
||||||
}
|
}
|
||||||
|
|
||||||
let mut forked = full_chain
|
let mut forked = full_chain
|
||||||
|
@ -291,7 +291,7 @@ fn forked_equals_pushed_history_tree() -> Result<()> {
|
||||||
.iter()
|
.iter()
|
||||||
.skip(fork_at_count)
|
.skip(fork_at_count)
|
||||||
.map(ContextuallyValidBlock::test_with_zero_chain_pool_change) {
|
.map(ContextuallyValidBlock::test_with_zero_chain_pool_change) {
|
||||||
forked.push(block)?;
|
forked = forked.push(block)?;
|
||||||
}
|
}
|
||||||
|
|
||||||
prop_assert_eq!(forked.blocks.len(), full_chain.blocks.len());
|
prop_assert_eq!(forked.blocks.len(), full_chain.blocks.len());
|
||||||
|
@ -328,7 +328,7 @@ fn finalized_equals_pushed_genesis() -> Result<()> {
|
||||||
.iter()
|
.iter()
|
||||||
.take(finalized_count)
|
.take(finalized_count)
|
||||||
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
||||||
full_chain.push(block)?;
|
full_chain = full_chain.push(block)?;
|
||||||
}
|
}
|
||||||
|
|
||||||
let mut partial_chain = Chain::new(
|
let mut partial_chain = Chain::new(
|
||||||
|
@ -343,14 +343,14 @@ fn finalized_equals_pushed_genesis() -> Result<()> {
|
||||||
.iter()
|
.iter()
|
||||||
.skip(finalized_count)
|
.skip(finalized_count)
|
||||||
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
||||||
partial_chain.push(block.clone())?;
|
partial_chain = partial_chain.push(block.clone())?;
|
||||||
}
|
}
|
||||||
|
|
||||||
for block in chain
|
for block in chain
|
||||||
.iter()
|
.iter()
|
||||||
.skip(finalized_count)
|
.skip(finalized_count)
|
||||||
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
||||||
full_chain.push(block.clone())?;
|
full_chain = full_chain.push(block.clone())?;
|
||||||
}
|
}
|
||||||
|
|
||||||
for _ in 0..finalized_count {
|
for _ in 0..finalized_count {
|
||||||
|
@ -398,7 +398,7 @@ fn finalized_equals_pushed_history_tree() -> Result<()> {
|
||||||
.iter()
|
.iter()
|
||||||
.take(finalized_count)
|
.take(finalized_count)
|
||||||
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
||||||
full_chain.push(block)?;
|
full_chain = full_chain.push(block)?;
|
||||||
}
|
}
|
||||||
|
|
||||||
let mut partial_chain = Chain::new(
|
let mut partial_chain = Chain::new(
|
||||||
|
@ -414,14 +414,14 @@ fn finalized_equals_pushed_history_tree() -> Result<()> {
|
||||||
.iter()
|
.iter()
|
||||||
.skip(finalized_count)
|
.skip(finalized_count)
|
||||||
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
||||||
partial_chain.push(block.clone())?;
|
partial_chain = partial_chain.push(block.clone())?;
|
||||||
}
|
}
|
||||||
|
|
||||||
for block in chain
|
for block in chain
|
||||||
.iter()
|
.iter()
|
||||||
.skip(finalized_count)
|
.skip(finalized_count)
|
||||||
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
.map(ContextuallyValidBlock::test_with_zero_spent_utxos) {
|
||||||
full_chain.push(block.clone())?;
|
full_chain= full_chain.push(block.clone())?;
|
||||||
}
|
}
|
||||||
|
|
||||||
for _ in 0..finalized_count {
|
for _ in 0..finalized_count {
|
||||||
|
@ -563,8 +563,8 @@ fn different_blocks_different_chains() -> Result<()> {
|
||||||
} else {
|
} else {
|
||||||
Default::default()
|
Default::default()
|
||||||
};
|
};
|
||||||
let mut chain1 = Chain::new(Network::Mainnet, Default::default(), Default::default(), Default::default(), finalized_tree1, ValueBalance::fake_populated_pool());
|
let chain1 = Chain::new(Network::Mainnet, Default::default(), Default::default(), Default::default(), finalized_tree1, ValueBalance::fake_populated_pool());
|
||||||
let mut chain2 = Chain::new(Network::Mainnet, Default::default(), Default::default(), Default::default(), finalized_tree2, ValueBalance::fake_populated_pool());
|
let chain2 = Chain::new(Network::Mainnet, Default::default(), Default::default(), Default::default(), finalized_tree2, ValueBalance::fake_populated_pool());
|
||||||
|
|
||||||
let block1 = vec1[1].clone().prepare().test_with_zero_spent_utxos();
|
let block1 = vec1[1].clone().prepare().test_with_zero_spent_utxos();
|
||||||
let block2 = vec2[1].clone().prepare().test_with_zero_spent_utxos();
|
let block2 = vec2[1].clone().prepare().test_with_zero_spent_utxos();
|
||||||
|
@ -572,8 +572,8 @@ fn different_blocks_different_chains() -> Result<()> {
|
||||||
let result1 = chain1.push(block1.clone());
|
let result1 = chain1.push(block1.clone());
|
||||||
let result2 = chain2.push(block2.clone());
|
let result2 = chain2.push(block2.clone());
|
||||||
|
|
||||||
// if there is an error, the chains come back empty
|
// if there is an error, we don't get the chains back
|
||||||
if result1.is_ok() && result2.is_ok() {
|
if let (Ok(mut chain1), Ok(chain2)) = (result1, result2) {
|
||||||
if block1 == block2 {
|
if block1 == block2 {
|
||||||
// the blocks were equal, so the chains should be equal
|
// the blocks were equal, so the chains should be equal
|
||||||
|
|
||||||
|
|
|
@ -50,7 +50,7 @@ fn construct_single() -> Result<()> {
|
||||||
ValueBalance::fake_populated_pool(),
|
ValueBalance::fake_populated_pool(),
|
||||||
);
|
);
|
||||||
|
|
||||||
chain.push(block.prepare().test_with_zero_spent_utxos())?;
|
chain = chain.push(block.prepare().test_with_zero_spent_utxos())?;
|
||||||
|
|
||||||
assert_eq!(1, chain.blocks.len());
|
assert_eq!(1, chain.blocks.len());
|
||||||
|
|
||||||
|
@ -81,7 +81,7 @@ fn construct_many() -> Result<()> {
|
||||||
);
|
);
|
||||||
|
|
||||||
for block in blocks {
|
for block in blocks {
|
||||||
chain.push(block.prepare().test_with_zero_spent_utxos())?;
|
chain = chain.push(block.prepare().test_with_zero_spent_utxos())?;
|
||||||
}
|
}
|
||||||
|
|
||||||
assert_eq!(100, chain.blocks.len());
|
assert_eq!(100, chain.blocks.len());
|
||||||
|
@ -105,7 +105,7 @@ fn ord_matches_work() -> Result<()> {
|
||||||
Default::default(),
|
Default::default(),
|
||||||
ValueBalance::fake_populated_pool(),
|
ValueBalance::fake_populated_pool(),
|
||||||
);
|
);
|
||||||
lesser_chain.push(less_block.prepare().test_with_zero_spent_utxos())?;
|
lesser_chain = lesser_chain.push(less_block.prepare().test_with_zero_spent_utxos())?;
|
||||||
|
|
||||||
let mut bigger_chain = Chain::new(
|
let mut bigger_chain = Chain::new(
|
||||||
Network::Mainnet,
|
Network::Mainnet,
|
||||||
|
@ -115,7 +115,7 @@ fn ord_matches_work() -> Result<()> {
|
||||||
Default::default(),
|
Default::default(),
|
||||||
ValueBalance::zero(),
|
ValueBalance::zero(),
|
||||||
);
|
);
|
||||||
bigger_chain.push(more_block.prepare().test_with_zero_spent_utxos())?;
|
bigger_chain = bigger_chain.push(more_block.prepare().test_with_zero_spent_utxos())?;
|
||||||
|
|
||||||
assert!(bigger_chain > lesser_chain);
|
assert!(bigger_chain > lesser_chain);
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue