vidpf: Implements a binary tree for caching VIPDF multiple evaluations. #978

armfazh · 2024-03-21T09:50:45Z

Implements a binary tree for caching multiple VIDPF evaluations.

This is a generic append-only binary tree. Its API is minimal to support caching VIDPF evaluations. Tree is serializable.

Related #925

divergentdave · 2024-03-21T18:48:17Z

src/bt.rs

+                match node.left {
+                    None => {
+                        node.left = new_node;
+                        break;
+                    }
+                    Some(_) => iter = node.left.as_deref_mut(),
+                };


This would misbehave if we were to try inserting a value at some node before inserting all of its parents. (since we don't check that this is the last bit in the path) Granted, this shouldn't happen during normal VIDPF evaluation, but it could be an issue with some level skipping strategies. Also, the Poplar1 implementation doesn't currently store anything in its cache with the empty bit string as the key, which would be a problem for using this data structure there.

We could either return an error or panic if we find an empty left/right pointer too early, or make node.value an option, and keep building the rest of the path's nodes before trying to store the value (this may affect the serialization format).

We could either return an error or panic if we find an empty left/right pointer too early, or make node.value an option, and keep building the rest of the path's nodes before trying to store the value (this may affect the serialization format).

I opted by the former option. However, if the latter is preferred, let me know so I can make the changes.

src/bt.rs

divergentdave · 2024-03-21T20:02:35Z

src/vidpf.rs

+    /// Evaluates multiple user reports.
+    pub fn multi_eval(


I'm not sure of the use of this function, and whether it should be part of the public API or test-only. Generally, I don't think there's any computation that can be shared across multiple client reports, so it doesn't make much difference whether we loop over reports in the library or in the application. On the other hand, if this is anticipating batched verification, that would be useful to have in the library (though we wouldn't be able to make use of it within the VDAF abstraction).

this is true, it's not actually used as is.
I was trying to exemplify the use of the tree for caching.
The tree will be used when mastic is in fact implemented.

divergentdave · 2024-03-21T20:10:04Z

src/vidpf.rs

+/// Clients use this structure to send reports to each of the Servers.
+pub struct ClientReport<V: VidpfValue> {


This probably doesn't need to be public, I figure it will just be used for tests here. Higher-level protocols such as DAP will be responsible for bundling the public share, input share, and nonce together in their own way, and that will include additional fields from Mastic in the shares as well.

divergentdave · 2024-03-21T20:11:26Z

src/vidpf.rs

+                    let mut state = VidpfEvalState::init_from_key(key);
+                    // Creates a tree with root equal to the first state.
+                    let mut tree = BinaryTree::new(state.clone());
+
+                    let input = VidpfInput::from(prefix.clone());
+                    let nonce_array = &nonce[..NONCE_SIZE].try_into().unwrap(); // todo: make this conversion reliable
+
+                    (state, _) =
+                        self.eval_next(key.id, public, &input, k - 1, &state, nonce_array)?;
+
+                    // State is cached in the path indicated by the prefix.
+                    tree.insert(&prefix, state);


Note that this is only evaluating the VIDPF at the first level, because it starts afresh with the initial state each time. The BinaryTree data structure only has one state inserted, and it isn't read from. To get the benefit of caching, we'd want to create a tree per client report, and use it to look up previously computed states while evaluating the same report at multiple prefixes. (and at multiple levels, though the multiple levels wouldn't all happen in one invocation in a realistic distributed trust setting)

agree, I will let the high-level protocol to use the binary tree.

src/lib.rs

cjpatton

The binary tree bits look great. Mostly minor comments, plus one feature request.

src/bt.rs

cjpatton · 2024-04-09T19:13:37Z

src/bt.rs

+
+impl<V: Display> Display for BinaryTree<V> {


Holy crap this is pretty!! However the output will contain secret shares when used with VIDPF. We are treating secret shared data as something you don't want to inadvertently log. Thus implementation of Debug or Display should either not contain any secret shares or should be fenced behind the "test-util" feature such that it's less likely to be compiled in production.

We should also decide whether we're OK with special characters or would rather stick to ascii. I'm agnostic, but @tgeoghegan may have thoughts here for example.

cc/ @divergentdave: Should Debug implementations have unicode characters in them?

I have changed the unicode symbols for ascii chars.
Coincidentally, printing, for example a BinaryTree<u32>, produces a valid YAML output.

src/bt.rs

cjpatton · 2024-04-11T22:52:23Z

src/bt.rs

+pub struct Node<V> {
+    value: V,
+    left: SubTree<V>,
+    right: SubTree<V>,
+}


This seems like the most natural way to implement a binary tree in Rust, using Box to emulate pointers in C/Go/etc. However, I wonder if it's the most efficient thing we could do.

Alternatively, we could keep the tree in a a vector of nodes and use indices into the vector for the children:

pub struct Node<V> { value: V, left: usize, right: usize, } pub struct Tree<V>(Vec<Node<V>>);

This would make the API a bit messier (when traversing the tree from one node to another, you need to have a reference to the tree), but this messiness would be crate-private. The benefit, I suspect, would be fewer allocations. I have no idea if this would be worth it however.

Thoughts, @divergentdave?

That's a good idea, however, I would do it in a different PR, so one can contrast timings.

Drive-by:

If the tree is unlikely to be sparse (or if the tree is sparse, but an acceptable sparse-vector implementation is available, or perhaps even a map from usize to V), the representation could be:

pub struct Tree<V>(Vec<Option<V>>); /* or */ pub struct Tree<V>(SomeSpareVec<Option<V>>); /* or */ pub struct Tree<V>(HashMap<usize, Option<V>>);

... using an implicit rule to determine the children of a node: the value for the root node is stored at index 0; the children of the node whose value is stored at index i have their values stored at indices 2*i + 1 and 2*i + 2.

This maintains the likely cache-locality/lowered-allocation wins of the node-referenced-by-index tree, while permitting fewer bad tree representations (i.e. it would not be possible to construct a "tree" with a cycle in it), and slightly simplifying tree traversal.

Insertion could almost be reduced to a simple index-lookup-and-set based on an index that ~~the same bitwise representation as the path being inserted to~~ can be quickly computed from the bitstring representing the path, except that this tree implementation currently requires all intermediate nodes to have a value. So insertion would need to check all of the intermediate nodes as well; if that requirement was relaxed, the need to check intermediate nodes could be dropped.

edit: the index doesn't have the same bitwise representation as the path, and can't since (0) must map to a different index than (0, 0).

Yes, I expect that using one big allocation as an offset-indexed arena of nodes could improve performance by improving cache locality, and amortizing malloc calls. It would be helpful if we could use heuristics from the application to size the Vec, to avoid unnecessary copying when resizing, but Decode doesn't lend itself to this.

For practical reasons, this tree will need to be sparse, as we could easily have a tree depth of over 100.

@armfazh proposes to land this PR as-is and consider this change later on. I'm cool with that if you all are.

divergentdave · 2024-04-13T01:26:43Z

src/bt.rs

+    /// Inserts the value at the end of the path.
+    ///
+    /// # Returns
+    /// - `Ok(())`, the node was inserted at the end of the path.
+    /// - `Err(InsertNonEmptyNode(value))`, the node already contains a value.
+    /// - `Err(UnreachablePath(value))`, the end of the path is unreachable.
+    ///
+    /// In the error cases, no insertion occurs and the value is returned to its owner.
+    pub fn insert(&mut self, path: &Path, value: V) -> Result<(), BinaryTreeError<V>> {


I think this could use some more explanation, particularly that Node::insert() expects only part of a node's index, and particularly just enough bits to navigate from self to another node. (as opposed to BinaryTree::insert(), which takes the entire node index, since it is starting from the root) So, this would be a suffix of a prefix of a Mastic input ultimately.

(the other Node methods could benefit from the same clarification)

updated, hope is clearer now.

src/bt.rs

cjpatton · 2024-04-16T20:39:12Z

src/bt.rs

+pub struct Node<V> {
+    value: V,
+    left: SubTree<V>,
+    right: SubTree<V>,
+}


@armfazh proposes to land this PR as-is and consider this change later on. I'm cool with that if you all are.

src/bt.rs

divergentdave · 2024-04-16T22:27:32Z

src/bt.rs

+impl<V: Encode> Encode for BinaryTree<V> {
+    fn encode(&self, bytes: &mut Vec<u8>) -> Result<(), CodecError> {
+        match &self.root {
+            None => CodecMarker::Leaf.encode(bytes),
+            Some(node) => node.encode(bytes),
+        }
+    }
+}
+
+impl<P, V: ParameterizedDecode<P>> ParameterizedDecode<P> for BinaryTree<V> {
+    fn decode_with_param(param: &P, bytes: &mut Cursor<&[u8]>) -> Result<Self, CodecError> {
+        match CodecMarker::decode(bytes)? {
+            CodecMarker::Leaf => Ok(Self::default()),
+            CodecMarker::Inner => {
+                bytes.seek(SeekFrom::Start(0))?;
+                let root = Some(Box::new(Node::decode_with_param(param, bytes)?));
+                Ok(Self { root })
+            }
+        }
+    }
+}


The asymmetry between encoding and decoding, and the seek, make me a bit uneasy. This is ultimately because the encoding of a Node must start with a 1 byte, so BinaryTree uses that byte to signal a non-empty root node, and writes/reads its own 0 byte otherwise for an empty node. But, in the 1 case, that first byte has to be read by the BinaryTree impl once, then read again by the Node impl after rewinding.

I'd like to get rid of the seeking to make future maintenance easier, particularly in case we make further big changes to the codec traits. Moreover, if a BinaryTree<V> were included as a field in a larger message, seeking to the start of the buffer would be incorrect -- we'd instead need to save bytes.stream_position() before reading the CodecMarker, and rewind by seeking to that position.

I suggest either eliminating the codec impls on Node<V>, combining them with these, and then initializing the BinaryTree impl's encoding stack with vec![self.root.as_ref()], or moving common codec logic into private functions that are used by impls for both Node<V> and BinaryTree<V>. If we keep codec impls on Node<V>, I'd suggest redefining the serialization format such that a node doesn't start with a CodecMarker, but a CodecMarker still appears before each child slot. This might require changing what enum is stored in the stack, but maybe there's a more clever way to handle this.

it wasn't so difficult to remove the seek call after all while keeping serialization of Node<V>.

This is a generic append-only binary tree, only insertion and queries are allowed. Specially useful for storing sparse trees. Tree supports serialization.

armfazh marked this pull request as ready for review March 21, 2024 15:16

armfazh requested a review from a team as a code owner March 21, 2024 15:16

armfazh mentioned this pull request Mar 21, 2024

IdpfCache implementation using a binary tree #925

Open

divergentdave reviewed Mar 21, 2024

View reviewed changes

armfazh requested a review from divergentdave April 9, 2024 07:55

armfazh force-pushed the impl_binary_tree branch from a16e5fb to 8e293ce Compare April 9, 2024 07:59

cjpatton reviewed Apr 9, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

cjpatton reviewed Apr 9, 2024

View reviewed changes

cjpatton reviewed Apr 11, 2024

View reviewed changes

divergentdave reviewed Apr 13, 2024

View reviewed changes

src/bt.rs Outdated Show resolved Hide resolved

divergentdave reviewed Apr 13, 2024

View reviewed changes

src/bt.rs Outdated Show resolved Hide resolved

armfazh requested a review from cjpatton April 16, 2024 15:58

cjpatton approved these changes Apr 16, 2024

View reviewed changes

armfazh force-pushed the impl_binary_tree branch from f39c1fc to 4c1aeeb Compare April 16, 2024 21:50

divergentdave reviewed Apr 16, 2024

View reviewed changes

Implementing binary tree for caching VIPDF evaluations.

ad5cf24

This is a generic append-only binary tree, only insertion and queries are allowed. Specially useful for storing sparse trees. Tree supports serialization.

armfazh force-pushed the impl_binary_tree branch from 4c1aeeb to ad5cf24 Compare April 16, 2024 23:36

divergentdave approved these changes Apr 16, 2024

View reviewed changes

divergentdave merged commit 3cadae5 into divviup:main Apr 22, 2024
6 checks passed

divergentdave mentioned this pull request May 10, 2024

Benchmark Poplar1 preparation with different IDPF evaluation caches #1038

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vidpf: Implements a binary tree for caching VIPDF multiple evaluations. #978

vidpf: Implements a binary tree for caching VIPDF multiple evaluations. #978

armfazh commented Mar 21, 2024 •

edited

Loading

divergentdave Mar 21, 2024

armfazh Apr 9, 2024 •

edited

Loading

divergentdave Mar 21, 2024

armfazh Apr 9, 2024

divergentdave Mar 21, 2024

divergentdave Mar 21, 2024

armfazh Apr 9, 2024

cjpatton left a comment

cjpatton Apr 9, 2024

cjpatton Apr 11, 2024

armfazh Apr 12, 2024

cjpatton Apr 11, 2024

armfazh Apr 12, 2024

branlwyd Apr 12, 2024 •

edited

Loading

divergentdave Apr 13, 2024

cjpatton Apr 16, 2024

divergentdave Apr 13, 2024 •

edited

Loading

armfazh Apr 16, 2024

cjpatton Apr 16, 2024

divergentdave Apr 16, 2024

armfazh Apr 16, 2024

		/// Clients use this structure to send reports to each of the Servers.
		pub struct ClientReport<V: VidpfValue> {

vidpf: Implements a binary tree for caching VIPDF multiple evaluations. #978

vidpf: Implements a binary tree for caching VIPDF multiple evaluations. #978

Conversation

armfazh commented Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

armfazh Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjpatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branlwyd Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divergentdave Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armfazh commented Mar 21, 2024 •

edited

Loading

armfazh Apr 9, 2024 •

edited

Loading

branlwyd Apr 12, 2024 •

edited

Loading

divergentdave Apr 13, 2024 •

edited

Loading