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

Crash resistant create() API #40

Closed
6 tasks done
staltz opened this issue Dec 7, 2022 · 16 comments
Closed
6 tasks done

Crash resistant create() API #40

staltz opened this issue Dec 7, 2022 · 16 comments
Labels
must Needed to complete grant milestone

Comments

@staltz
Copy link
Member

staltz commented Dec 7, 2022

We should "restart" any create() that was partially done but interrupted by a crash.

Plus tests.


@staltz staltz added the must Needed to complete grant milestone label Dec 7, 2022
@Powersource
Copy link
Collaborator

@staltz @mixmix @arj03 is there any prior art on this?

@Powersource
Copy link
Collaborator

if this crashes on line 89, then we've got a new group feed that won't be used. that doesn't break anything since we'll just create a new one the next time we try to create a group, but it's slightly inefficient. does that need to be fixed?

ssb-tribes2/index.js

Lines 83 to 95 in 206138e

const secret = new SecretKey()
ssb.metafeeds.findOrCreate(function gotRoot(err, root) {
if (err) return cb(err)
findOrCreateGroupFeed(secret, function gotGroupFeed(err, groupFeed) {
if (err) return cb(err)
const recps = [
{ key: secret.toBuffer(), scheme: keySchemes.private_group },
]
ssb.db.create(

@Powersource
Copy link
Collaborator

i guess it's a similar problem to, and maybe has the same solution as, when you crash inbetween creating the root msg and inviting yourself to the group. which similarly should mostly just be suboptimal

@Powersource
Copy link
Collaborator

i was gonna ask if this was redundant

// TODO: add self to recps, for crash-resistance

since we later add ourselves to the group anyway. but maybe encrypting to ourselves here as well could be a way of saving the group early

or wait, when recovering from scratch, will you need the feed announcement encrypted to yourself to be able to find the feed again? hmm maybe not, since we do invite ourselves on our invitations feed, and we know how to find the invitations feed

@staltz
Copy link
Member Author

staltz commented Dec 13, 2022

if this crashes on line 89, then we've got a new group feed that won't be used. that doesn't break anything since we'll just create a new one the next time we try to create a group

I guess it's fine, but it depends on what kind of crash it is. If it's a totally random crash, then the issue of leaving empty groups behind is definitely not a problem because it's very rare. But let's say that this is a deterministic crash, e.g. every time the user creates a group, it always crashes on line 89. Then crash-resistance is important, otherwise we're creating a lot of group feeds in vain.

There is also another issue here in that the group/init msg doesn't say anything at all about what the group is in terms of name, purpose, description, etc. It's completely naked. I think we should have some kind of descriptive data there, but it doesn't need to be done in this issue.

So here is what I suggest:

  • Crash-resistance for line 89
    • On start(), traverse the user's MF tree and detect all group feeds that have no msgs posted on them, and then post the group/init msg. However I think this is not quite possible to do because the ssb-box2 API hasn't been called yet, so the announcement of the group feed (on the shard feed) cannot be decrypted. So:
    • Add a new API to ssb-keyring keyring.group.addTemp where the key is like Date.now() instead of cloaked msg ID
    • Add a new API addTempGroupInfo to ssb-box2 that calls keyring.group.addTemp
    • Use box2.addTempGroupInfo in tribes2 before line 89
    • Optional: remove the temp group info after the official one is added. This is just about performance
  • Crash-resistance for line 103
    • Re-calculate const data to get the groupId, and call box2.getGroupKeyInfo to see if we have something. If we don't, then add the group info
  • Crash-resistance for line 120
    • Read all of the user's groups, and check if there is a group/add-member msg for that group on the additions feed, if not then call addMembers()

@Powersource
Copy link
Collaborator

thanks for the elaborate suggestions

Add a new API to ssb-keyring keyring.group.addTemp where the key is like Date.now() instead of cloaked msg ID

I don't understand what we'd do with the date.now?


I think the underlying problem is that we only really save the secret for ourselves in the last step, when we add ourselves. The suggestion on line 56 linked above might be the solution to this problem, then we save the key in a way recoverable by ourselves in the first step (first published message by create() ). Then from that we should be able to go through the rest of the steps based on the secret we store there, even if we crash.

@Powersource
Copy link
Collaborator

Now how do I actually test this lmao

@Powersource
Copy link
Collaborator

@staltz not done! several more potential crashes i think

@Powersource Powersource reopened this Feb 6, 2023
@Powersource
Copy link
Collaborator

Right now I see two remaining issues:

  1. ssb-tribes2/index.js

    Lines 171 to 195 in 7710a3b

    ssb.db.create(
    {
    keys: groupFeed.keys,
    content,
    recps,
    encryptionFormat: 'box2',
    },
    (err, groupInitMsg) => {
    // prettier-ignore
    if (err) return cb(clarify(err, 'Failed to create group init message when creating a group'))
    const data = {
    id: buildGroupId({
    groupInitMsg,
    groupKey: secret.toBuffer(),
    }),
    secret: secret.toBuffer(),
    root: fromMessageSigil(groupInitMsg.key),
    subfeed: groupFeed.keys,
    }
    ssb.box2.addGroupInfo(data.id, {
    key: data.secret,
    root: data.root,
    })

    Crash after creating the root message but before saving it in box2 or through addMembers
    me on signal right now:

if you crash between creating the root (init) message and saving the group key somewhere then you've 'lost' that root message. especially since the spec says

This means this initial message and it's content will need to be manually boxed,
with the only recipient_key being the symmetric group_key for this new group.

so we can't according to that add ourselves as a recipient 🤔 should i just create a pr adding ourselves as a recipient and hope in review that no one remembers some big reasons we can't encrypt it to ourselves
hmm maybe one reason could be that right now every message on the group feed only has [groupId] as the recps, but we said we wanted to put re-invites on those feeds anyway

  1. Hmm maybe there isn't a second one. It feels like addGroupInfo and addMembers will be sort-of atomic? Since we can't addMembers without having added the key.

@Powersource
Copy link
Collaborator

That part of the spec got added in the commit "first pass at all READMEs" v0.0.1 so maybe not that important ssbc/private-group-spec@260a381 The file btw https://github.com/ssbc/private-group-spec/blob/master/group/init/README.md

@mixmix
Copy link
Member

mixmix commented Feb 13, 2023

@Powersource I think in the group/init we can :

  • add ourselves as a recps (so we can later decrypt using own_key)
  • include a copy of the group_key/secret in the content, so that we can recover it that way (which is nice if we don't get around to persisting it in the keyring yet)

@Powersource
Copy link
Collaborator

oh yeah that sounds smart

@Powersource
Copy link
Collaborator

for the purposes of tribes2 code i don't think having the secret in the init will save us lines of code here

ssb-tribes2/index.js

Lines 230 to 241 in ea5e29a

ssb.metafeeds.advanced.findById(
rootMsg.value.author,
(err, groupFeed) => {
// prettier-ignore
if (err) return cb(clarify(err, "failed finding details of the memberless group feed"))
rootMsg.value.content = rootMsg.meta.originalContent
return cb(null, {
groupInitMsg: rootMsg,
groupFeed,
myRoot,
})
since we're gonna need the feed key anyway here

ssb-tribes2/index.js

Lines 260 to 275 in ea5e29a

findOrCreateGroupWithoutMembers(
(err, { groupInitMsg, groupFeed, myRoot }) => {
// prettier-ignore
if (err) return cb(clarify(err, 'Failed to create group init message when creating a group'))
const secret = secretKeyFromPurpose(groupFeed.purpose)
const data = {
id: buildGroupId({
groupInitMsg,
groupKey: secret.toBuffer(),
}),
secret: secret.toBuffer(),
root: fromMessageSigil(groupInitMsg.key),
subfeed: groupFeed.keys,
}
but still feels like a good change for redundancy

@Powersource
Copy link
Collaborator

@mixmix made a pr for that ssbc/private-group-spec#25

@Powersource
Copy link
Collaborator

also in tribes2 #62

@Powersource
Copy link
Collaborator

Booya

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
must Needed to complete grant milestone
Projects
None yet
Development

No branches or pull requests

3 participants