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

Operator attestation policy #10721

Merged
merged 7 commits into from
Dec 18, 2024
Merged

Operator attestation policy #10721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
caf6d7a
chore: constrain fast-usdc config for Mainnet
turadg Dec 17, 2024
621e22f
test: disabled operators
turadg Dec 17, 2024
0b57a65
refactor: TransactionFeed attest
turadg Dec 17, 2024
1e52a41
refactor: sync submitEvidence
turadg Dec 17, 2024
fb4ff30
refactor: attest by operatorId
turadg Dec 17, 2024
bc28201
feat: operator majority logic
turadg Dec 17, 2024
cd2a40c
feat: error on conflicting evidence
turadg Dec 17, 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
12 changes: 6 additions & 6 deletions packages/fast-usdc/src/exos/operator-kit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const trace = makeTracer('TxOperator');

/**
* @typedef {object} OperatorPowers
* @property {(evidence: CctpTxEvidence, operatorKit: OperatorKit) => void} submitEvidence
* @property {(evidence: CctpTxEvidence, operatorId: string) => void} attest
*/

/**
Expand All @@ -35,7 +35,7 @@ const OperatorKitI = {
}),

operator: M.interface('Operator', {
submitEvidence: M.call(CctpTxEvidenceShape).returns(M.promise()),
submitEvidence: M.call(CctpTxEvidenceShape).returns(),
getStatus: M.call().returns(M.record()),
}),
};
Expand Down Expand Up @@ -87,7 +87,7 @@ export const prepareOperatorKit = (zone, staticPowers) =>
const { operator } = this.facets;
// TODO(bootstrap integration): cause this call to throw and confirm that it
// shows up in the the smart-wallet UpdateRecord `error` property
await operator.submitEvidence(evidence);
operator.submitEvidence(evidence);
return staticPowers.makeInertInvitation(
'evidence was pushed in the invitation maker call',
);
Expand All @@ -98,12 +98,12 @@ export const prepareOperatorKit = (zone, staticPowers) =>
* submit evidence from this operator
*
* @param {CctpTxEvidence} evidence
* @returns {void}
*/
async submitEvidence(evidence) {
submitEvidence(evidence) {
const { state } = this;
!state.disabled || Fail`submitEvidence for disabled operator`;
const result = state.powers.submitEvidence(evidence, this.facets);
return result;
state.powers.attest(evidence, state.operatorId);
},
/** @returns {OperatorStatus} */
getStatus() {
Expand Down
68 changes: 45 additions & 23 deletions packages/fast-usdc/src/exos/transaction-feed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { makeTracer } from '@agoric/internal';
import { prepareDurablePublishKit } from '@agoric/notifier';
import { M } from '@endo/patterns';
import { keyEQ, M } from '@endo/patterns';
import { Fail } from '@endo/errors';
import { CctpTxEvidenceShape } from '../type-guards.js';
import { defineInertInvitation } from '../utils/zoe.js';
import { prepareOperatorKit } from './operator-kit.js';
Expand All @@ -18,7 +19,7 @@ export const INVITATION_MAKERS_DESC = 'oracle operator invitation';

const TransactionFeedKitI = harden({
operatorPowers: M.interface('Transaction Feed Admin', {
submitEvidence: M.call(CctpTxEvidenceShape, M.any()).returns(),
attest: M.call(CctpTxEvidenceShape, M.string()).returns(),
}),
creator: M.interface('Transaction Feed Creator', {
// TODO narrow the return shape to OperatorKit
Expand Down Expand Up @@ -118,23 +119,16 @@ export const prepareTransactionFeedKit = (zone, zcf) => {
/**
* Add evidence from an operator.
*
* NB: the operatorKit is responsible for
Copy link
Member

Choose a reason for hiding this comment

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

for... what?

Copy link
Member Author

Choose a reason for hiding this comment

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

I must have gotten a Slack notification!

The operatorKit is responsible for preventing a disabled kit from attesting.

I'll get that in before merge. I'll probably wait for another reviewer's comments before pushing again.

*
* @param {CctpTxEvidence} evidence
* @param {OperatorKit} operatorKit
* @param {string} operatorId
*/
submitEvidence(evidence, operatorKit) {
const { pending } = this.state;
trace(
'submitEvidence',
operatorKit.operator.getStatus().operatorId,
evidence,
);
const { operatorId } = operatorKit.operator.getStatus();

// TODO should this verify that the operator is one made by this exo?
// This doesn't work...
// operatorKit === operators.get(operatorId) ||
// Fail`operatorKit mismatch`;
attest(evidence, operatorId) {
const { operators, pending } = this.state;
trace('submitEvidence', operatorId, evidence);

// TODO https://github.com/Agoric/agoric-sdk/pull/10720
// TODO validate that it's a valid for Fast USDC before accepting
// E.g. that the `recipientAddress` is the FU settlement account and that
// the EUD is a chain supported by FU.
Expand All @@ -154,18 +148,46 @@ export const prepareTransactionFeedKit = (zone, zcf) => {
const found = [...pending.values()].filter(store =>
store.has(txHash),
);
// TODO determine the real policy for checking agreement
if (found.length < pending.getSize()) {
// not all have seen it
const minAttestations = Math.ceil(operators.getSize() / 2);
trace(
'transaction',
txHash,
'has',
found.length,
'of',
minAttestations,
'necessary attestations',
);
if (found.length < minAttestations) {
Copy link
Member

Choose a reason for hiding this comment

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

this seems to be the gist of it.

return;
}

// TODO verify that all found deep equal
let lastEvidence;
for (const store of found) {
const next = store.get(txHash);
if (lastEvidence) {
if (keyEQ(lastEvidence, next)) {
lastEvidence = next;
} else {
trace(
'🚨 conflicting evidence for',
Copy link
Member

Choose a reason for hiding this comment

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

is this a red-alert situation? It seems like we recover: we don't act on inconsistent evidence.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it deserves immediate operational attention if two oracles are disagreeing. At least one is unreliable and may require immediate action.

txHash,
':',
lastEvidence,
'!=',
next,
Comment on lines +176 to +178
Copy link
Member

Choose a reason for hiding this comment

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

if we have to use this in anger, I expect we'll want to see which operator submitted which evidence.

is that already traced?

);
Fail`conflicting evidence for ${txHash}`;
}
}
lastEvidence = next;
}

// all agree, so remove from pending and publish
for (const pendingStore of pending.values()) {
pendingStore.delete(txHash);
// sufficient agreement, so remove from pending and publish
for (const store of found) {
Comment on lines -166 to +187
Copy link
Member

Choose a reason for hiding this comment

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

the change from pending.values() to found... I had to think a bit to convince myself that make no difference.

store.delete(txHash);
}
trace('publishing evidence', evidence);
publisher.publish(evidence);
},
},
Expand Down
8 changes: 8 additions & 0 deletions packages/fast-usdc/src/utils/deploy-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,11 @@ export const configurations = {
},
};
harden(configurations);

// Constraints on the configurations
const MAINNET_EXPECTED_ORACLES = 3;
assert(
Copy link
Member

Choose a reason for hiding this comment

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

👍

new Set(Object.values(configurations.MAINNET.oracles)).size ===
MAINNET_EXPECTED_ORACLES,
`Mainnet must have exactly ${MAINNET_EXPECTED_ORACLES} oracles`,
);
93 changes: 65 additions & 28 deletions packages/fast-usdc/test/exos/transaction-feed.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,50 +47,87 @@ test('happy aggregation', async t => {
const evidenceSubscriber = feedKit.public.getEvidenceSubscriber();

const { op1, op2, op3 } = await makeOperators(feedKit);
const evidence = MockCctpTxEvidences.AGORIC_PLUS_OSMO();
const results = await Promise.all([
op1.operator.submitEvidence(evidence),
op2.operator.submitEvidence(evidence),
op3.operator.submitEvidence(evidence),
]);
t.deepEqual(results, [undefined, undefined, undefined]);

const e1 = MockCctpTxEvidences.AGORIC_PLUS_OSMO();
op1.operator.submitEvidence(e1);
op2.operator.submitEvidence(e1);

// Publishes with 2 of 3
const accepted = await evidenceSubscriber.getUpdateSince(0);
t.deepEqual(accepted, {
value: evidence,
value: e1,
updateCount: 1n,
});

// verify that it doesn't publish until three match
await Promise.all([
// once it publishes, it doesn't remember that it already saw these
op1.operator.submitEvidence(evidence),
op2.operator.submitEvidence(evidence),
// but this time the third is different
op3.operator.submitEvidence(MockCctpTxEvidences.AGORIC_PLUS_DYDX()),
]);
// Now third operator catches up with same evidence already published
Copy link
Member

Choose a reason for hiding this comment

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

I haven't thought carefully about whether this is sufficient test coverage.

op3.operator.submitEvidence(e1);
t.like(await evidenceSubscriber.getUpdateSince(0), {
// Update count is still 1
// The confirming evidence doesn't change anything
updateCount: 1n,
});
await op3.operator.submitEvidence(evidence);

const e2 = MockCctpTxEvidences.AGORIC_PLUS_DYDX();
assert(e1.txHash !== e2.txHash);
op1.operator.submitEvidence(e2);
t.like(await evidenceSubscriber.getUpdateSince(0), {
// op1 attestation insufficient
updateCount: 1n,
});
});

test('disagreement', async t => {
const feedKit = makeFeedKit();
const { op1, op2 } = await makeOperators(feedKit);
const e1 = MockCctpTxEvidences.AGORIC_PLUS_OSMO();
const e1bad = { ...e1, tx: { ...e1.tx, amount: 999_999_999n } };
assert(e1.txHash === e1bad.txHash);
op1.operator.submitEvidence(e1);

t.throws(() => op2.operator.submitEvidence(e1bad), {
message:
'conflicting evidence for "0xc81bc6105b60a234c7c50ac17816ebcd5561d366df8bf3be59ff387552761702"',
});
});

test('disagreement after publishing', async t => {
const feedKit = makeFeedKit();
const evidenceSubscriber = feedKit.public.getEvidenceSubscriber();
const { op1, op2, op3 } = await makeOperators(feedKit);
const e1 = MockCctpTxEvidences.AGORIC_PLUS_OSMO();
const e1bad = { ...e1, tx: { ...e1.tx, amount: 999_999_999n } };
assert(e1.txHash === e1bad.txHash);
op1.operator.submitEvidence(e1);
op2.operator.submitEvidence(e1);

t.like(await evidenceSubscriber.getUpdateSince(0), {
updateCount: 1n,
});

// it's simply ignored
t.notThrows(() => op3.operator.submitEvidence(e1bad));
t.like(await evidenceSubscriber.getUpdateSince(0), {
Copy link
Member

Choose a reason for hiding this comment

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

why ask for updates since 0 again? isn't the n+1th call supposed to pass in the updateCount from the nth call?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a handy way to ask for the latest without risk of a hanging await while writing tests

updateCount: 1n,
});

// now another op repeats the bad evidence, so it's published to the stream.
Copy link
Member

Choose a reason for hiding this comment

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

ooh. tricky.

// It's the responsibility of the Advancer to fail because it has already processed that tx hash.
op1.operator.submitEvidence(e1bad);
t.like(await evidenceSubscriber.getUpdateSince(0), {
updateCount: 2n,
});
});

// TODO: find a way to get this working
test.skip('forged source', async t => {
test('disabled operator', async t => {
Copy link
Member

Choose a reason for hiding this comment

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

good; the "unknown transaction" test mixed in testing this, and I wondered about reducing coverage.

const feedKit = makeFeedKit();
const { op1 } = await makeOperators(feedKit);
const evidence = MockCctpTxEvidences.AGORIC_PLUS_OSMO();

// op1 is different than the facets object the evidence must come from
t.throws(() =>
feedKit.operatorPowers.submitEvidence(
evidence,
// @ts-expect-error XXX Types of property '[GET_INTERFACE_GUARD]' are incompatible.
op1,
),
);
// works before disabling
op1.operator.submitEvidence(evidence);

op1.admin.disable();

t.throws(() => op1.operator.submitEvidence(evidence), {
message: 'submitEvidence for disabled operator',
});
});
41 changes: 25 additions & 16 deletions packages/fast-usdc/test/fast-usdc.contract.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { test as anyTest } from '@agoric/zoe/tools/prepare-test-env-ava.js';
import type { ExecutionContext, TestFn } from 'ava';

import {
decodeAddressHook,
encodeAddressHook,
} from '@agoric/cosmic-proto/address-hooks.js';
import { AmountMath } from '@agoric/ertp/src/amountMath.js';
import { deeplyFulfilledObject } from '@agoric/internal';
import {
eventLoopIteration,
inspectMapStore,
Expand All @@ -24,13 +27,9 @@ import {
import type { Instance } from '@agoric/zoe/src/zoeService/utils.js';
import { setUpZoeForTest } from '@agoric/zoe/tools/setup-zoe.js';
import { E } from '@endo/far';
import { matches, objectMap } from '@endo/patterns';
import { matches } from '@endo/patterns';
import { makePromiseKit } from '@endo/promise-kit';
import path from 'path';
import {
decodeAddressHook,
encodeAddressHook,
} from '@agoric/cosmic-proto/address-hooks.js';
import type { OperatorKit } from '../src/exos/operator-kit.js';
import type { FastUsdcSF } from '../src/fast-usdc.contract.js';
import { PoolMetricsShape } from '../src/type-guards.js';
Expand All @@ -56,10 +55,12 @@ const getInvitationProperties = async (
return amount.value[0];
};

// Spec for Mainnet. Other values are covered in unit tests of TransactionFeed.
const operatorQty = 3;

type CommonSetup = Awaited<ReturnType<typeof commonSetup>>;
const startContract = async (
common: Pick<CommonSetup, 'brands' | 'commonPrivateArgs' | 'utils'>,
operatorQty = 1,
) => {
const {
brands: { usdc },
Expand Down Expand Up @@ -104,7 +105,7 @@ const makeTestContext = async (t: ExecutionContext) => {
const common = await commonSetup(t);
await E(common.mocks.ibcBridge).setAddressPrefix('noble');

const startKit = await startContract(common, 2);
const startKit = await startContract(common);

const { transferBridge } = common.mocks;
const evm = makeEVM();
Expand Down Expand Up @@ -227,12 +228,17 @@ const makeOracleOperator = async (
]);
const { invitationMakers } = operatorKit;

let active = true;

return harden({
watch: () => {
void observeIteration(subscribeEach(txSubscriber), {
updateState: tx =>
updateState: tx => {
if (!active) {
return;
}
// KLUDGE: tx wouldn't include aux. OCW looks it up
E.when(
return E.when(
E(invitationMakers).SubmitEvidence(tx),
inv =>
E.when(E(E(zoe).offer(inv)).getOfferResult(), res => {
Expand All @@ -242,13 +248,17 @@ const makeOracleOperator = async (
reason => {
failures.push(reason.message);
},
),
);
},
});
},
getDone: () => done,
getFailures: () => harden([...failures]),
// operator only gets .invitationMakers
getKit: () => operatorKit,
setActive: flag => {
active = flag;
},
});
};

Expand Down Expand Up @@ -760,10 +770,12 @@ test.serial('Settlement for unknown transaction (operator down)', async t => {
} = t.context;
const operators = await sync.ocw.promise;

// Simulate 2 of 3 operators being unavailable
operators[0].setActive(false);
operators[1].setActive(false);

const opDown = makeCustomer('Otto', cctp, txPub.publisher, feeConfig);

// what removeOperator will do
await E(E.get(E(operators[1]).getKit()).admin).disable();
const bridgePos = snapshot();
const sent = await opDown.sendFast(t, 20_000_000n, 'osmo12345');
await mint(sent);
Expand All @@ -788,9 +800,6 @@ test.serial('Settlement for unknown transaction (operator down)', async t => {
t.deepEqual(bridgeTraffic.local, [], 'no IBC transfers');

await transmitTransferAck();
t.deepEqual(await E(operators[1]).getFailures(), [
'submitEvidence for disabled operator',
]);
});

test.todo(
Expand Down
Loading