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.

Sign up for GitHub

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

Jump to bottom

Prevent duplicate event when anchoring reg or cred in multisigs #271

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Prevent duplicate event when anchoring reg or cred in multisigs #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
97607aa
avoid duplicating an anchor
rodolfomiranda Jul 27, 2024
c65ee57
fix date iso format
rodolfomiranda Jul 27, 2024
60ce790
conditionals
rodolfomiranda Jul 27, 2024
be95cd2
pretty
rodolfomiranda Jul 27, 2024
83e15e0
more conditionals
rodolfomiranda Jul 27, 2024
bceda2c
fix test
rodolfomiranda Jul 27, 2024
fd73bde
pretty again
rodolfomiranda Jul 27, 2024
6dcf772
+ tests
rodolfomiranda Jul 29, 2024
8261cd9
pretty
rodolfomiranda Jul 29, 2024
a3e2b21
test coverage
rodolfomiranda Jul 30, 2024
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
63 changes: 54 additions & 9 deletions src/keri/app/credentialing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ export class Credentials {
args: CredentialData
): Promise<IssueCredentialResult> {
const hab = await this.client.identifiers().get(name);
const events = await this.client.keyEvents().get(hab.prefix);
const estOnly = hab.state.c !== undefined && hab.state.c.includes('EO');
if (estOnly) {
// TODO implement rotation event
Expand Down Expand Up @@ -230,18 +231,33 @@ export class Credentials {
dt: subject.dt,
});

const sn = parseInt(hab.state.s, 16);
let sn = parseInt(hab.state.s, 16);
sn = sn + 1;
let dig = hab.state.d;

// check if last event already has the anchor in it
// and avoid creating a new event if it does
const lastEvent = events[events.length - 1];
Copy link
Contributor

@iFergal iFergal Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with registry events but I'm also curious if there's a way to generally prevent duplicate anchoring events in a KEL directly in KERIA, at least with the same signing keys.

The trouble here is if there are many concurrent things happening, and the duplicate event is e.g. not actually the lastEvent but the event before that (events.length - 2 or so on)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think this should be handled server side and in a way that avoids any race conditions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might actually just be something we can add to keripy directly, maybe Sam or Phil have some ideas

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that something can also be done in the keripy/keria side. Those types of race conditions are always tricky and recover from them is not trivial. There's some code on keripy that assigns one of the members as the lead. And I remember Phil mentioning that they cover for some race conditions, but probably not all cases.

Anyway, the goal of this PR is to solve a specific use case that is that one member of a multisig initiates the creation of a new event and the others join; however one of them joins late, after the event was already completed (thresholds satisfied) with the signatures of the other members. We want this tardy member to create the correct event, not a new one. And we can catch the error on the client side because we know that the same anchor is already on the KEL. There are no race conditions in this use case. One member start the event, and the rest of the members join. This require an extra call, but I think it worth it since prevent other problems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would still be cleaner if the client side did the redundant signing while late joining and KERIA just accepts it and doesn't add it to the KEL (as it's already a duplicate).

Doing on the client side here doesn't cover the case I mentioned for events.length - 2 and also doesn't cover the case of the final threshold signature appearing at the same time as the client is signing but after they did this check (which is a race condition).

Albeit we could have it as a stop gap solution perhaps

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, then again, that would leave an inconsistent KEL locally since ultimately the controller of the KEL is on the client side, hmm

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right in both, this PR only covers the use case of events.lenght - 1. And for the race condition, we need something on keripy that I was trying to avoid, or postpone it for later, for simplicity and urgent need.
We do also need a way to recover from the duplication in case it happens.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't reviewed the nitty gritty details of it but if it's urgent, perhaps it's OK so long as we open the appropriate issues now and tackle it soon (and not let it fall into the pile of issues :P)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In light of this discussion, I'm interested to see what @rodolfomiranda and @iFergal think about @lenkan PR solution #286

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lenkan's PR makes sense to me and avoids my race condition concerns here

if (
lastEvent?.a?.length == 1 &&
lastEvent?.a[0]?.i == iss.i &&
lastEvent?.a[0]?.s == iss.s &&
lastEvent?.a[0]?.d == iss.d
) {
sn = sn - 1; // revert sn
dig = hab.state.p!;
}
const anc = interact({
pre: hab.prefix,
sn: sn + 1,
sn: sn,
data: [
{
i: iss.i,
s: iss.s,
d: iss.d,
},
],
dig: hab.state.d,
dig: dig,
version: undefined,
kind: undefined,
});
Expand Down Expand Up @@ -287,6 +303,7 @@ export class Credentials {
datetime?: string
): Promise<RevokeCredentialResult> {
const hab = await this.client.identifiers().get(name);
const events = await this.client.keyEvents().get(hab.prefix);
const pre: string = hab.prefix;

const vs = versify(Ident.KERI, undefined, Serials.JSON, 0);
Expand Down Expand Up @@ -320,8 +337,9 @@ export class Credentials {
var estOnly = false;
}

const sn = parseInt(state.s, 16);
const dig = state.d;
let sn = parseInt(state.s, 16);
sn = sn + 1;
let dig = state.d;

const data: any = [
{
Expand All @@ -332,14 +350,26 @@ export class Credentials {
];

const keeper = this.client!.manager!.get(hab);
// check if last event already has the anchor in it
// and avoid creating a new event if it does
const lastEvent = events[events.length - 1];
if (
lastEvent?.a?.length == 1 &&
lastEvent?.a[0]?.i == rev.i &&
lastEvent?.a[0]?.s == rev.s &&
lastEvent?.a[0]?.d == rev.d
) {
sn = sn - 1; // revert sn
dig = state.p!;
}

if (estOnly) {
// TODO implement rotation event
throw new Error('Establishment only not implemented');
} else {
const serder = interact({
pre: pre,
sn: sn + 1,
sn: sn,
data: data,
dig: dig,
version: undefined,
Expand Down Expand Up @@ -585,6 +615,7 @@ export class Registries {
nonce,
}: CreateRegistryArgs): Promise<RegistryResult> {
const hab = await this.client.identifiers().get(name);
const events = await this.client.keyEvents().get(hab.prefix);
const pre: string = hab.prefix;

const cnfg: string[] = [];
Expand All @@ -604,8 +635,9 @@ export class Registries {
throw new Error('establishment only not implemented');
} else {
const state = hab.state;
const sn = parseInt(state.s, 16);
const dig = state.d;
let sn = parseInt(state.s, 16);
sn = sn + 1;
let dig = state.d;

const data: any = [
{
Expand All @@ -615,9 +647,22 @@ export class Registries {
},
];

// check if last event already has the anchor in it
// and avoid creating a new event if it does
const lastEvent = events[events.length - 1];
if (
lastEvent?.a?.length == 1 &&
lastEvent?.a[0]?.i == regser.pre &&
lastEvent?.a[0]?.s == '0' &&
lastEvent?.a[0]?.d == regser.pre
) {
sn = sn - 1; // revert sn
dig = state.p!;
}

const serder = interact({
pre: pre,
sn: sn + 1,
sn: sn,
data: data,
dig: dig,
version: Versionage,
Expand Down
4 changes: 2 additions & 2 deletions src/keri/core/manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ export class Manager {
pp.stem = creator.stem;
pp.tier = creator.tier;

const dt = new Date().toString();
const dt = new Date().toISOString().replace('Z', '000+00:00');
const nw = new PubLot();
nw.pubs = Array.from(verfers, (verfer: Verfer) => verfer.qb64);
nw.ridx = ridx;
Expand Down Expand Up @@ -793,7 +793,7 @@ export class Manager {
(signer: Signer) => new Diger({ code: dcode }, signer.verfer.qb64b)
);

const dt = new Date().toString();
const dt = new Date().toISOString().replace('Z', '000+00:00');
ps.nxt = new PubLot();
ps.nxt.pubs = Array.from(
keys.signers,
Expand Down
111 changes: 108 additions & 3 deletions test/app/credentialing.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,67 @@ const mockGetAID = {
windexes: [],
};

const mockGetAID2 = {
name: 'aid2',
prefix: 'ELUvZ8aJEHAQE-0nsevyYTP98rBbGJUrTj5an-pCmwrK',
salty: {
sxlt: '1AAHnNQTkD0yxOC9tSz_ukbB2e-qhDTStH18uCsi5PCwOyXLONDR3MeKwWv_AVJKGKGi6xiBQH25_R1RXLS2OuK3TN3ovoUKH7-A',
pidx: 0,
kidx: 0,
stem: 'signify:aid',
tier: 'low',
dcode: 'E',
icodes: ['A'],
ncodes: ['A'],
transferable: true,
},
transferable: true,
state: {
vn: [1, 0],
i: 'ELUvZ8aJEHAQE-0nsevyYTP98rBbGJUrTj5an-pCmwrK',
s: '1',
p: '',
d: 'ELUvZ8aJEHAQE-0nsevyYTP98rBbGJUrTj5an-pCmwrK',
f: '0',
dt: '2023-08-21T22:30:46.473545+00:00',
et: 'ixn',
kt: '1',
k: ['DPmhSfdhCPxr3EqjxzEtF8TVy0YX7ATo0Uc8oo2cnmY9'],
nt: '1',
n: ['EAORnRtObOgNiOlMolji-KijC_isa3lRDpHCsol79cOc'],
bt: '0',
b: [],
c: [],
ee: {
s: '0',
d: 'ELUvZ8aJEHAQE-0nsevyYTP98rBbGJUrTj5an-pCmwrK',
br: [],
ba: [],
},
di: '',
},
windexes: [],
};

const mockEvents = [
{
i: 'a prefix',
s: '0',
},
{
i: 'a prefix',
s: '1',
d: 'a 2nd digest',
a: [
{
d: 'EGB8Q3UgcsLftRsffimiRS0pqpVgRNr4Di8qorKm0u1_',
i: 'EK_6Rlxmdl6Ieyd5oz81HF3Kvv2E8nCG1rYRHA7CZPRF',
s: '0',
},
],
},
];

const mockCredential = {
sad: {
v: 'ACDC10JSON000197_',
Expand Down Expand Up @@ -210,9 +271,19 @@ fetchMock.mockResponse((req) => {
req.method,
requrl.pathname.split('?')[0]
);
const body = req.url.startsWith(url + '/credentials')
? mockCredential
: mockGetAID;

let body = {};
if (req.url.startsWith(url + '/credentials')) {
body = mockCredential;
} else if (req.url === url + '/identifiers/aid1') {
body = mockGetAID;
} else if (req.url === url + '/identifiers/aid2') {
body = mockGetAID2;
} else if (req.url.startsWith(url + '/events')) {
body = mockEvents;
} else if (req.url.startsWith(url + '/identifiers')) {
body = mockGetAID;
}

return Promise.resolve({
body: JSON.stringify(body),
Expand Down Expand Up @@ -369,6 +440,40 @@ describe('Credentialing', () => {
'EP10ooRj0DJF0HWZePEYMLPl-arMV-MAoTKK-o3DXbgX'
);
});
it('Issue credentials when anchor is already in the KEL', async () => {
await libsodium.ready;
const bran = '0123456789abcdefghijk';

const client = new SignifyClient(url, bran, Tier.low, boot_url);

await client.boot();
await client.connect();

const credentials = client.credentials();

const registry = 'EP10ooRj0DJF0HWZePEYMLPl-arMV-MAoTKK-o3DXbgX';
const schema = 'EBfdlu8R27Fbx-ehrqwImnK-8Cm79sqbAQ4MmvEAYqao';
const isuee = 'EG2XjQN-3jPN5rcR4spLjaJyM4zA6Lgg-Hd5vSMymu5p';
await credentials.issue('aid2', {
ri: registry,
s: schema,
a: {
i: isuee,
LEI: '1234',
dt: '2023-08-23T15:16:07.553000+00:00',
},
});
const lastCall = fetchMock.mock.calls[fetchMock.mock.calls.length - 1]!;
const lastBody = JSON.parse(lastCall[1]!.body!.toString());
assert.equal(lastCall[0]!, url + '/identifiers/aid2/credentials');
assert.equal(lastCall[1]!.method, 'POST');
assert.equal(lastBody.iss.s, '0');
assert.equal(lastBody.iss.t, 'iss');
assert.equal(lastBody.iss.ri, registry);
assert.equal(lastBody.iss.i, lastBody.acdc.d);
assert.equal(lastBody.ixn.t, 'ixn');
assert.equal(lastBody.ixn.s, '1');
});
});

describe('Ipex', () => {
Expand Down
Loading
Loading