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

merge queue: embarking main (0e39c9d) and [#7578 + #7566] together #7584

Closed
wants to merge 59 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
40ac61d
Avoid manual handling of previous sapling trees by using iterator win…
teor2345 Sep 7, 2023
5a11f00
Avoid manual sapling subtree index handling by comparing prev and cur…
teor2345 Sep 7, 2023
effff63
Simplify adding notes by using the exact number of remaining notes
teor2345 Sep 7, 2023
c0e61ed
Simplify by skipping the first block, because it can't complete a sub…
teor2345 Sep 7, 2023
3c9eb58
Re-use existing tree update code
teor2345 Sep 7, 2023
bc0c674
Apply the sapling changes to orchard subtree updates
teor2345 Sep 7, 2023
387f8bd
add a reverse database column family iterator function
teor2345 Sep 7, 2023
c15d734
Make skipping the lowest tree independent of iteration order
teor2345 Sep 7, 2023
0695da8
Move new subtree checks into the iterator, rename to end_height
teor2345 Sep 7, 2023
f0be95c
Split subtree calculation into a new method
teor2345 Sep 7, 2023
8864370
Split the calculate and write methods
teor2345 Sep 12, 2023
538653d
Quickly check the first subtree before running the full upgrade
teor2345 Sep 12, 2023
a678989
Do the quick checks every time Zebra runs, and refactor slow check er…
teor2345 Sep 12, 2023
bebb226
Do quick checks for orchard as well
teor2345 Sep 12, 2023
8b54c09
Make orchard tree upgrade match sapling upgrade code
teor2345 Sep 12, 2023
a63d109
Upgrade subtrees in reverse height order
teor2345 Sep 12, 2023
fa11314
Bump the database patch version so the upgrade runs again
teor2345 Sep 12, 2023
392f699
Reset previous subtree upgrade data before doing this one
teor2345 Sep 12, 2023
b3f4b80
Add extra checks to subtree calculation to diagnose errors
teor2345 Sep 12, 2023
294c9a9
Use correct heights for subtrees completed at the end of a block
teor2345 Sep 12, 2023
f018c5d
Add even more checks to diagnose issues
teor2345 Sep 12, 2023
2a47cc0
Instrument upgrade methods to improve diagnostics
teor2345 Sep 12, 2023
5799da8
Prevent modification of re-used trees
teor2345 Sep 12, 2023
fd72b1f
Debug with subtree positions as well
teor2345 Sep 12, 2023
5e6bcd7
Fix an off-by-one error with completed subtrees
teor2345 Sep 12, 2023
cd1ecc5
Fix typos and confusing comments
teor2345 Sep 14, 2023
6c6d157
Restore the panic on invalid subtrees
teor2345 Sep 14, 2023
540933f
Fix mistaken previous tree handling and end tree comments
teor2345 Sep 14, 2023
c4382bd
Remove unnecessary subtraction in remaining leaves calc
teor2345 Sep 14, 2023
1f5f58d
Log heights when assertions fail
teor2345 Sep 15, 2023
bc259c3
Fix new subtree detection filter
teor2345 Sep 15, 2023
a86891d
Move new subtree check into a method, cleanup unused code
teor2345 Sep 15, 2023
58ee6f8
Remove redundant assertions
teor2345 Sep 15, 2023
0146f9f
Wait for subtree upgrade before testing RPCs
teor2345 Sep 15, 2023
1d889aa
Fix subtree search in quick check
teor2345 Sep 15, 2023
a9558be
Temporarily upgrade subtrees in forward height order
teor2345 Sep 15, 2023
74db923
Clarify some comments
teor2345 Sep 15, 2023
2f88858
Fix missing test imports
teor2345 Sep 15, 2023
d457eaf
Fix subtree logging
teor2345 Sep 15, 2023
1d8f458
Add a comment about a potential hang with future upgrades
teor2345 Sep 15, 2023
6d1e494
Fix zebrad var ownership
teor2345 Sep 15, 2023
c6b8aa7
Log more info when add_subtrees.rs fails
teor2345 Sep 17, 2023
5ee803a
cargo fmt --all
teor2345 Sep 17, 2023
96ace90
Fix unrelated clippy::unnecessary_unwrap
teor2345 Sep 17, 2023
d33f497
cargo clippy --fix --all-features --all-targets; cargo fmt --all
teor2345 Sep 17, 2023
7b0baf8
Stop the quick check depending on tree de-duplication
teor2345 Sep 17, 2023
e3b014a
Refactor waiting for the upgrade into functions
teor2345 Sep 17, 2023
be6a33e
Wait for state upgrades whenever the cached state is updated
teor2345 Sep 17, 2023
171998b
Wait for the testnet upgrade in the right place
teor2345 Sep 18, 2023
82bf50a
Fix unused variable
teor2345 Sep 18, 2023
2e69e84
build(deps): bump the test group with 2 updates
dependabot[bot] Sep 18, 2023
82ca619
Merge branch 'main' into fix-flsync-subt
teor2345 Sep 18, 2023
5a1deb1
Fix a subtree detection bug and comments
teor2345 Sep 18, 2023
c206404
Remove an early reference to reverse direction
teor2345 Sep 18, 2023
41eef09
Stop skipping subtrees completed at the end of blocks
teor2345 Sep 19, 2023
54f2bba
Actually fix new subtree code
teor2345 Sep 19, 2023
f85068b
Merge branch 'main' into fix-flsync-subt
upbqdn Sep 19, 2023
138a689
Merge of #7578
mergify[bot] Sep 19, 2023
71541ea
Merge of #7566
mergify[bot] Sep 19, 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
23 changes: 12 additions & 11 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2052,9 +2052,9 @@ dependencies = [

[[package]]
name = "inferno"
version = "0.11.16"
version = "0.11.17"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "73c0fefcb6d409a6587c07515951495d482006f89a21daa0f2f783aa4fd5e027"
checksum = "c50453ec3a6555fad17b1cd1a80d16af5bc7cb35094f64e429fd46549018c6a3"
dependencies = [
"ahash",
"is-terminal",
Expand Down Expand Up @@ -3146,12 +3146,12 @@ dependencies = [

[[package]]
name = "prost"
version = "0.12.0"
version = "0.12.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "aa8473a65b88506c106c28ae905ca4a2b83a2993640467a41bb3080627ddfd2c"
checksum = "f4fdd22f3b9c31b53c060df4a0613a1c7f062d4115a2b984dd15b1858f7e340d"
dependencies = [
"bytes",
"prost-derive 0.12.0",
"prost-derive 0.12.1",
]

[[package]]
Expand All @@ -3168,7 +3168,7 @@ dependencies = [
"once_cell",
"petgraph",
"prettyplease",
"prost 0.12.0",
"prost 0.12.1",
"prost-types 0.12.0",
"regex",
"syn 2.0.29",
Expand All @@ -3191,9 +3191,9 @@ dependencies = [

[[package]]
name = "prost-derive"
version = "0.12.0"
version = "0.12.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "56075c27b20ae524d00f247b8a4dc333e5784f889fe63099f8e626bc8d73486c"
checksum = "265baba7fabd416cf5078179f7d2cbeca4ce7a9041111900675ea7c4cb8a4c32"
dependencies = [
"anyhow",
"itertools 0.11.0",
Expand All @@ -3217,7 +3217,7 @@ version = "0.12.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "cebe0a918c97f86c217b0f76fd754e966f8b9f41595095cf7d74cb4e59d730f6"
dependencies = [
"prost 0.12.0",
"prost 0.12.1",
]

[[package]]
Expand Down Expand Up @@ -4556,7 +4556,7 @@ dependencies = [
"hyper-timeout",
"percent-encoding",
"pin-project",
"prost 0.12.0",
"prost 0.12.1",
"tokio",
"tokio-stream",
"tower",
Expand Down Expand Up @@ -5740,6 +5740,7 @@ dependencies = [
"futures",
"halo2_proofs",
"hex",
"hex-literal",
"howudoin",
"indexmap 2.0.0",
"insta",
Expand Down Expand Up @@ -5848,7 +5849,7 @@ dependencies = [
"pin-project",
"proptest",
"proptest-derive",
"prost 0.12.0",
"prost 0.12.1",
"rand 0.8.5",
"rayon",
"regex",
Expand Down
22 changes: 10 additions & 12 deletions zebra-chain/src/block/arbitrary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,16 @@ impl Block {
}
}
// update history tree for the next block
if history_tree.is_none() {
if let Some(history_tree) = history_tree.as_mut() {
history_tree
.push(
current.network,
Arc::new(block.clone()),
sapling_tree.root(),
orchard_tree.root(),
)
.unwrap();
} else {
history_tree = Some(
HistoryTree::from_block(
current.network,
Expand All @@ -527,17 +536,6 @@ impl Block {
)
.unwrap(),
);
} else {
history_tree
.as_mut()
.unwrap()
.push(
current.network,
Arc::new(block.clone()),
sapling_tree.root(),
orchard_tree.root(),
)
.unwrap();
}
}

Expand Down
106 changes: 106 additions & 0 deletions zebra-chain/src/orchard/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,72 @@ impl NoteCommitmentTree {
self.inner.value()
}

/// Returns the position of the most recently appended leaf in the tree.
///
/// This method is used for debugging, use `incrementalmerkletree::Address` for tree operations.
pub fn position(&self) -> Option<u64> {
let Some(tree) = self.frontier() else {
// An empty tree doesn't have a previous leaf.
return None;
};

Some(tree.position().into())
}

/// Returns true if this tree has at least one new subtree, when compared with `prev_tree`.
pub fn contains_new_subtree(&self, prev_tree: &Self) -> bool {
// Use -1 for the index of the subtree with no notes, so the comparisons are valid.
let index = self.subtree_index().map_or(-1, |index| i32::from(index.0));
let prev_index = prev_tree
.subtree_index()
.map_or(-1, |index| i32::from(index.0));

// This calculation can't overflow, because we're using i32 for u16 values.
let index_difference = index - prev_index;

// There are 4 cases we need to handle:
// - lower index: never a new subtree
// - equal index: sometimes a new subtree
// - next index: sometimes a new subtree
// - greater than the next index: always a new subtree
//
// To simplify the function, we deal with the simple cases first.

// There can't be any new subtrees if the current index is strictly lower.
if index < prev_index {
return false;
}

// There is at least one new subtree, even if there is a spurious index difference.
if index_difference > 1 {
return true;
}

// If the indexes are equal, there can only be a new subtree if `self` just completed it.
if index == prev_index {
return self.is_complete_subtree();
}

// If `self` is the next index, check for spurious index differences.
//
// There is one new subtree somewhere in the trees. It is either:
// - a new subtree at the end of the previous tree, or
// - a new subtree in this tree (but not at the end).
//
// Spurious index differences happen because the subtree index only increases when the
// first note is added to the new subtree. So we need to exclude subtrees completed by the
// last note commitment in the previous tree.
//
// We also need to exclude empty previous subtrees, because the index changes to zero when
// the first note is added, but a subtree wasn't completed.
if prev_tree.is_complete_subtree() || prev_index == -1 {
return false;
}

// A new subtree was completed by a note commitment that isn't in the previous tree.
true
}

/// Returns true if the most recently appended leaf completes the subtree
pub fn is_complete_subtree(&self) -> bool {
let Some(tree) = self.frontier() else {
Expand Down Expand Up @@ -423,6 +489,46 @@ impl NoteCommitmentTree {
Some(index)
}

/// Returns the number of leaf nodes required to complete the subtree at
/// [`TRACKED_SUBTREE_HEIGHT`].
///
/// Returns `2^TRACKED_SUBTREE_HEIGHT` if the tree is empty.
#[allow(clippy::unwrap_in_result)]
pub fn remaining_subtree_leaf_nodes(&self) -> usize {
let remaining = match self.frontier() {
// If the subtree has at least one leaf node, the remaining number of nodes can be
// calculated using the maximum subtree position and the current position.
Some(tree) => {
let max_position = incrementalmerkletree::Address::above_position(
TRACKED_SUBTREE_HEIGHT.into(),
tree.position(),
)
.max_position();

max_position - tree.position().into()
}
// If the subtree has no nodes, the remaining number of nodes is the number of nodes in
// a subtree.
None => {
let subtree_address = incrementalmerkletree::Address::above_position(
TRACKED_SUBTREE_HEIGHT.into(),
// This position is guaranteed to be in the first subtree.
0.into(),
);

assert_eq!(
subtree_address.position_range_start(),
0.into(),
"address is not in the first subtree"
);

subtree_address.position_range_end()
}
};

u64::from(remaining).try_into().expect("fits in usize")
}

/// Returns subtree index and root if the most recently appended leaf completes the subtree
pub fn completed_subtree_index_and_root(&self) -> Option<(NoteCommitmentSubtreeIndex, Node)> {
if !self.is_complete_subtree() {
Expand Down
21 changes: 15 additions & 6 deletions zebra-chain/src/parallel/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ impl NoteCommitmentTrees {
}

/// Update the sprout note commitment tree.
/// This method modifies the tree inside the `Arc`, if the `Arc` only has one reference.
fn update_sprout_note_commitment_tree(
mut sprout: Arc<sprout::tree::NoteCommitmentTree>,
sprout_note_commitments: Vec<sprout::NoteCommitment>,
Expand All @@ -145,8 +146,9 @@ impl NoteCommitmentTrees {
}

/// Update the sapling note commitment tree.
/// This method modifies the tree inside the `Arc`, if the `Arc` only has one reference.
#[allow(clippy::unwrap_in_result)]
fn update_sapling_note_commitment_tree(
pub fn update_sapling_note_commitment_tree(
mut sapling: Arc<sapling::tree::NoteCommitmentTree>,
sapling_note_commitments: Vec<sapling::tree::NoteCommitmentUpdate>,
) -> Result<
Expand All @@ -170,11 +172,14 @@ impl NoteCommitmentTrees {
let mut subtree_root = None;

for sapling_note_commitment in sapling_note_commitments {
sapling_nct.append(sapling_note_commitment)?;

// Subtrees end heights come from the blocks they are completed in,
// so we check for new subtrees after appending the note.
// (If we check before, subtrees at the end of blocks have the wrong heights.)
if let Some(index_and_node) = sapling_nct.completed_subtree_index_and_root() {
subtree_root = Some(index_and_node);
}

sapling_nct.append(sapling_note_commitment)?;
}

// Re-calculate and cache the tree root.
Expand All @@ -184,8 +189,9 @@ impl NoteCommitmentTrees {
}

/// Update the orchard note commitment tree.
/// This method modifies the tree inside the `Arc`, if the `Arc` only has one reference.
#[allow(clippy::unwrap_in_result)]
fn update_orchard_note_commitment_tree(
pub fn update_orchard_note_commitment_tree(
mut orchard: Arc<orchard::tree::NoteCommitmentTree>,
orchard_note_commitments: Vec<orchard::tree::NoteCommitmentUpdate>,
) -> Result<
Expand All @@ -203,11 +209,14 @@ impl NoteCommitmentTrees {
let mut subtree_root = None;

for orchard_note_commitment in orchard_note_commitments {
orchard_nct.append(orchard_note_commitment)?;

// Subtrees end heights come from the blocks they are completed in,
// so we check for new subtrees after appending the note.
// (If we check before, subtrees at the end of blocks have the wrong heights.)
if let Some(index_and_node) = orchard_nct.completed_subtree_index_and_root() {
subtree_root = Some(index_and_node);
}

orchard_nct.append(orchard_note_commitment)?;
}

// Re-calculate and cache the tree root.
Expand Down
Loading