[v2.0] Introduce Topics #236

DyrellC · 2022-05-19T20:00:15Z

Description of change

Introduce Topic type and split KeyStore into BranchStore.
Changes:

Cursor and Key management are mapped to Topic's in BranchStore
Topic's will be sent inside the HDF of messages
Each branch will have an anchor message to attach Keyload messages to
Each branch will keep track of the latest link sent to it for iterative attachment purposes
Messages can be sent without a link_to address
Introduce new_branch function to allow users to announce (and retrieve) branches and store them before Keyload messages are parsed

Type of change

Enhancement (a non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

How the change has been tested

Examples updated to include branch announcements, removal of link_to from send functions
Existing cargo tests have been updated and tested

lets/src/message/topic.rs

DyrellC · 2022-05-20T18:36:02Z

I need to investigate something that's happening here: The example is not completing consistently, for some reason it looks like a user that is not supposed to be able to read a signed packet is retrieving, need to look into it to see where the bug is

arnauorriols

First feedback, that focuses on 2 aspects that I think need to be discussed before continuing with the review:

Root branch provided as input in User::create_stream() instead of having a BASE_BRANCH constant
- [v2.0] Introduce Topics #236 (comment)
Topic being backed by a String instead of a byte array
- [v2.0] Introduce Topics #236 (comment)

streams/src/lib.rs

streams/examples/full-example/scenarios/basic.rs

streams/src/api/mod.rs

lets/src/message/topic.rs

streams/src/api/key_store.rs

…hStore::remove_from_all()`

arnauorriols

Reviewed key_store.rs. I've found an important bug in remove_from_all() (and similar methods that use the fold pattern). I've written a test that replicates the bug, give it a look!

streams/src/api/key_store.rs

lets/src/message/topic.rs

DyrellC

The main issues that needs resolving first are:

Topic internal and the Copy in HDF
The fold bug that may be responsible for intermittent false removal of subscribers

lets/src/message/topic.rs

streams/src/lib.rs

lets/src/message/topic.rs

streams/src/api/key_store.rs

lets/src/message/topic.rs

streams/examples/full-example/scenarios/basic.rs

streams/src/api/mod.rs

lets/src/message/topic.rs

…into v2.0/test-topics

… v2.0/test-topics

…nto v2.0/test-topics

… v2.0/test-topics

lets/src/address.rs

lets/src/message/preparsed.rs

lets/src/message/message.rs

lets/src/message/topic.rs

streams/src/api/cursor_store.rs

streams/src/api/user.rs

DyrellC

Fresh round of changes in response to the reviews. Still need to do the Option/upserting for BranchStore but most of the rest has been addressed.

DyrellC · 2022-06-08T19:20:34Z

spongos/src/ddml/types/bytes.rs

-        self.0.resize(new_size, 0)
+    // Return an error if size exceeds u32 max size to avoid panic
+    // TODO: Remove anyhow error and replace with no_std compatible error handling
+    pub(crate) fn resize(&mut self, new_size: usize) -> anyhow::Result<()> {


When recovering a user we end up unmasking the stream of wrapped topics/cursors, but if you use the wrong password (as we do in the examples) it mixes up the unmasked Size value for number of topics, and the Size within the topic Byte unwrap gets messed up as well. This results in an attempt to create a Vec with a size that exceeds its max allocation availability (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.reserve), resulting in a panic that we have no way to catch.

One thing that threw me off was I tried to use isize::MAX and it still resulted in panics on occassion when the new size exceeded u32::MAX, so I made that the default max length. From a Streams perspective, we have no instance where Bytes should exceed 32Kb let alone u32::MAX so I thought that was an appropriate value to use.

lets/src/address.rs

lets/src/message/message.rs

lets/src/message/preparsed.rs

lets/src/message/topic.rs

streams/src/api/user_builder.rs

streams/src/api/user.rs

streams/src/api/cursor_store.rs

arnauorriols

Ok, here goes some more comments. I did not had time to finish reviewing user.rs, but I'm confident the big chunk is done. There are some clone() here and there that I think can be spared but didn't comment on it. We'll clean them up along the way.

The most important discussions I'm opening are:

branch announcement as a separate message type
keyloads linked to tip of branch, and joining the stream announcement spongos

streams/src/api/cursor_store.rs

streams/src/api/user.rs

streams/src/api/cursor_store.rs

arnauorriols · 2022-06-09T13:33:57Z

streams/src/api/user.rs

        // Prepare HDF and PCF
-        let header = HDF::new(message_types::ANNOUNCEMENT, ANN_MESSAGE_NUM, self.identifier())?;
+        let header = HDF::new(
+            message_types::ANNOUNCEMENT,


(I have the feeling to had already raised this concern somewhere, but can't remember where nor if it got any answer, so I'm raising it again from scratch 😆 )

Are we sure we want to reuse the announcement message for both stream announcement and branch announcement? In my opinion that's not a very good idea, mainly for 2 reasons:

the announcement message is a fundamentally different entity than the branch announcement, as proven by the fact that it includes the author's identifier, which is not necessary for the branch announcement. Instead, if we want to support a tree structure branching from other branches, branch announcements will need the topic they are branching from. Also haven't really thought about it, but are we sure it's ok that branch announcements are not linked to any message?

the previous fact forces us to introduce the is_base_branch if-branching in the announcement-related methods. Now it seems harmless, but it will become uglier once we introduce admin permissions (admin subscribers can send branch announcements, but not stream announcements)

We can agree on an iterative approach as long as we are aligned on the design.

I wouldn't be opposed to having branch announcement be its own thing, although I'd prefer to make that a focused PR of its own. This one is already undergoing a pretty significant number of changes/reviews as is

more than fair.

arnauorriols · 2022-06-09T14:18:04Z

lets/src/address.rs

@@ -229,10 +231,11 @@ impl MsgId {
        Self(bytes)
    }

-    pub fn gen(appaddr: AppAddr, identifier: Identifier, seq_num: usize) -> MsgId {
+    pub fn gen(appaddr: AppAddr, identifier: Identifier, topic: Topic, seq_num: usize) -> MsgId {


Topic does not need to be taken by value, and it is not Copy now so it should be taken by ref

Should we not do this for Identifier as well despite the Copy? It's an extra allocation without it being necessary.

As long as something is Copy, you can be certain it does not incur in a heap allocation (basically because that's a hard condition for types to be able to implement Copy). Generally speaking, Copy is a marker that says that "cloning" the type is essentially the same as moving the type (a memcpy). For this reason, Copy types can keep using the old variable, even after a move to another variable (aka the copy semantics). Types that don't implement Copy have a more complex cloning (for example, because they handle resources, like the pointer to some heap allocated memory that tneeds to be freed when dropping the value), therefore move != clone anymore.

For the types that don't implement Copy, it's important not to take ownership in function parameters if it's not needed, cause you are forcing a potentially costly clone unnecessarily. For Copy types, this clone is as cheap as a move, and the extra cost of the move vs reference, even if arguable, I'd say is under the ream of "profile before assuming anything" and definitely negligible in our case. So in this case ergonomics prevail.

TL;DR: performance is not a factor in this decision. But still passing Identifier by reference would be valid if you prefer it (for coherence, or any other reason)

lets/src/address.rs

streams/src/api/user.rs

streams/src/api/messages.rs

streams/src/api/user.rs

Co-authored-by: Arnau Orriols <[email protected]>

…into v2.0/test-topics

arnauorriols

From my side, this can be merged. I'll create the issue to discuss and track the comments that have been postponed (keyload linking, branch announcement message type)

DyrellC · 2022-06-10T15:09:18Z

@kwek20 any further comments/change requests before we merge?

kwek20 · 2022-06-13T14:59:28Z

Hmm only thing is maybe the name of cursor_store. SHouldnt it be branch_store now? Or maybe just state_store? (Im assuming you guys dont want to call it just store :D)

DyrellC · 2022-06-13T17:20:54Z

Probalby reasonable to change the module name, but I don't think it matters all that much, the only purpose of BranchStore is to map CursorStore's. So we either we pull cursor_store::{BranchStore, CursorStore} or branch_store::{BranchStore, CursorStore}. Seems rather negligible which is used since it's only ever pulled into User anyways. @arnauorriols any feelings one way or the other?

arnauorriols · 2022-06-14T14:29:24Z

Seems rather negligible which is used since it's only ever pulled into User anyways

Even if it's not part of the public API, we should view this naming decision from a new contributor perspective, and what name would be most intuitive for her when trying to make sense of the code structure. In my opinion, the only meaningful name is cursor_store, cause at the end of the day what we are storing are cursors (that happen to be grouped by branch, but the ultimate item stored is cursors). I didn't want to open this topic, but now that you have mention it, ideally I'd rename BranchStore to CursorStore, and come up with any other name for what is currently called CursorStore (InnerStore for all I care 😄).

But I really don't think it's that important, you both have equally valid arguments, I'm fine with any decision as long as this beast gets merged!

DyrellC added 8 commits May 18, 2022 19:54

First pass branching

15eb1dc

AsRef for topic

8886b36

mask topic directly, include in address gen, eq failing for user

d79e56d

Fix examples/merge overwrites

125f314

Add anchor/latest link to key store, remove link_to from sending

c6eda71

clippy/fmt/warnings

3fbf7ea

fix tests

bd41e2b

fmt

8515d9d

DyrellC changed the title ~~V2.0/test topics~~ [v2.0] Introduce Topics May 19, 2022

fmt in nightly

6d904c5

kwek20 reviewed May 20, 2022

View reviewed changes

remove unused functions

c5c104c

arnauorriols reviewed May 23, 2022

View reviewed changes

arnauorriols reviewed May 24, 2022

View reviewed changes

streams/src/api/key_store.rs Outdated Show resolved Hide resolved

Write test to expose BranchStore not removing all cursors in `Branc…

cd30f54

…hStore::remove_from_all()`

arnauorriols reviewed May 24, 2022

View reviewed changes

DyrellC commented May 24, 2022

View reviewed changes

DyrellC added 3 commits May 24, 2022 12:04

remove fold and use for_each in cursor removal

27fe5ca

Merge branch 'v2.0/test-topics' of https://github.com/DyrellC/streams …

ea855fb

…into v2.0/test-topics

Insert own base branch topic

2b00e82

arnauorriols mentioned this pull request May 25, 2022

[v2.0] Move psk store to user #237

Merged

DyrellC added 8 commits May 26, 2022 08:32

Make Topic String

8aa1fa6

Merge branch 'v2.0-dev' of https://github.com/iotaledger/streams into…

0e12d43

… v2.0/test-topics

Check size before resizing bytes

53cb461

fmt/clippy

05c7d84

tests update

d257605

Merge branch 'v2.0/move-keys' of https://github.com/DyrellC/streams i…

3990b43

…nto v2.0/test-topics

fmt

21afe44

fix cursor store test

79bd3ea

Merge branch 'v2.0-dev' of https://github.com/iotaledger/streams into…

0e222b4

… v2.0/test-topics

arnauorriols reviewed Jun 8, 2022

View reviewed changes

DyrellC added 2 commits June 8, 2022 22:36

first round review changes

4c26b18

round two review updates

b068917

DyrellC commented Jun 9, 2022

View reviewed changes

update tests

10f923f

arnauorriols reviewed Jun 9, 2022

View reviewed changes

DyrellC and others added 10 commits June 9, 2022 10:06

Mac check for backup/restore, Address::gen takes topic by ref

f14f065

Update streams/src/api/cursor_store.rs

1194c8e

Co-authored-by: Arnau Orriols <[email protected]>

Update cursor_store for anchor and latest link set/get

ef728fd

fmt, clippy

d301431

Merge branch 'v2.0/test-topics' of https://github.com/DyrellC/streams …

158b3f3

…into v2.0/test-topics

typo

705daad

base_topic -> base_branch

e514d50

clean up user from review suggestions

5b837d5

revert byte changes

91c0b37

remove resize result

a5f0ae1

arnauorriols approved these changes Jun 10, 2022

View reviewed changes

arnauorriols mentioned this pull request Jun 14, 2022

branch announcement message type #258

Open

rename CursorStore and BranchStore

d191de1

DyrellC merged commit 35ea66f into iotaledger:v2.0-dev Jun 14, 2022

DyrellC deleted the v2.0/test-topics branch March 13, 2023 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2.0] Introduce Topics #236

[v2.0] Introduce Topics #236

DyrellC commented May 19, 2022

DyrellC commented May 20, 2022

arnauorriols left a comment •

edited

Loading

arnauorriols left a comment

DyrellC left a comment

DyrellC left a comment

DyrellC Jun 8, 2022

arnauorriols left a comment

arnauorriols Jun 9, 2022

DyrellC Jun 9, 2022

arnauorriols Jun 10, 2022

arnauorriols Jun 9, 2022

DyrellC Jun 9, 2022

arnauorriols Jun 10, 2022 •

edited

Loading

arnauorriols left a comment

DyrellC commented Jun 10, 2022

kwek20 commented Jun 13, 2022

DyrellC commented Jun 13, 2022

arnauorriols commented Jun 14, 2022 •

edited

Loading

[v2.0] Introduce Topics #236

[v2.0] Introduce Topics #236

Conversation

DyrellC commented May 19, 2022

Description of change

Type of change

How the change has been tested

DyrellC commented May 20, 2022

arnauorriols left a comment • edited Loading

Choose a reason for hiding this comment

arnauorriols left a comment

Choose a reason for hiding this comment

DyrellC left a comment

Choose a reason for hiding this comment

DyrellC left a comment

Choose a reason for hiding this comment

DyrellC Jun 8, 2022

Choose a reason for hiding this comment

arnauorriols left a comment

Choose a reason for hiding this comment

arnauorriols Jun 9, 2022

Choose a reason for hiding this comment

DyrellC Jun 9, 2022

Choose a reason for hiding this comment

arnauorriols Jun 10, 2022

Choose a reason for hiding this comment

arnauorriols Jun 9, 2022

Choose a reason for hiding this comment

DyrellC Jun 9, 2022

Choose a reason for hiding this comment

arnauorriols Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

arnauorriols left a comment

Choose a reason for hiding this comment

DyrellC commented Jun 10, 2022

kwek20 commented Jun 13, 2022

DyrellC commented Jun 13, 2022

arnauorriols commented Jun 14, 2022 • edited Loading

arnauorriols left a comment •

edited

Loading

arnauorriols Jun 10, 2022 •

edited

Loading

arnauorriols commented Jun 14, 2022 •

edited

Loading