Operator attestation policy #10721
Operator attestation policy #10721
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
cd2a40c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8ea94c46.agoric-sdk.pages.dev |
| Branch Preview URL: | https://ta-attestation-policy.agoric-sdk.pages.dev |
turadg
force-pushed
the
ta/attestation-policy
branch
from
186dd94 to
cd2a40c
Compare
| @@ -79,6 +79,21 @@ test('happy aggregation', async t => { | |||
| }); | |||
| }); | |||
|
|
|||
| test('disabled operator', async t => { | |||
There was a problem hiding this comment.
good; the "unknown transaction" test mixed in testing this, and I wondered about reducing coverage.
| @@ -118,10 +118,12 @@ export const prepareTransactionFeedKit = (zone, zcf) => { | |||
| /** | |||
| * Add evidence from an operator. | |||
| * | |||
| * NB: the operatorKit is responsible for | |||
There was a problem hiding this comment.
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.
| minAttestations, | ||
| 'necessary attestations', | ||
| ); | ||
| if (found.length < minAttestations) { |
| for (const pendingStore of pending.values()) { | ||
| pendingStore.delete(txHash); | ||
| // sufficient agreement, so remove from pending and publish | ||
| for (const store of found) { |
There was a problem hiding this comment.
the change from pending.values() to found... I had to think a bit to convince myself that make no difference.
| // but this time the third is different | ||
| op3.operator.submitEvidence(MockCctpTxEvidences.AGORIC_PLUS_DYDX()); | ||
|
|
||
| // Now third operator catches up with same evidence already published |
There was a problem hiding this comment.
I haven't thought carefully about whether this is sufficient test coverage.
| lastEvidence = next; | ||
| } else { | ||
| trace( | ||
| '🚨 conflicting evidence for', |
There was a problem hiding this comment.
is this a red-alert situation? It seems like we recover: we don't act on inconsistent evidence.
There was a problem hiding this comment.
I think it deserves immediate operational attention if two oracles are disagreeing. At least one is unreliable and may require immediate action.
| lastEvidence, | ||
| '!=', | ||
| next, |
There was a problem hiding this comment.
if we have to use this in anger, I expect we'll want to see which operator submitted which evidence.
is that already traced?
|
|
||
| // it's simply ignored | ||
| t.notThrows(() => op3.operator.submitEvidence(e1bad)); | ||
| t.like(await evidenceSubscriber.getUpdateSince(0), { |
There was a problem hiding this comment.
why ask for updates since 0 again? isn't the n+1th call supposed to pass in the updateCount from the nth call?
There was a problem hiding this comment.
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. |
|
|
||
| // Constraints on the configurations | ||
| const MAINNET_EXPECTED_ORACLES = 3; | ||
| assert( |
TODO in code
Description
Update
TransactionFeedto publish when a majority of operators to attest (previous required unanimous). Conflicts log a loud error and don't publish.Also updates the contract test to have 3 operators (per Mainnet policy) and rely on unit tests for the other values.
Security Considerations
Adds assertions to the deploy-config to help ensure that Mainnet has three unique operators, per security policy.
Scaling Considerations
Each new attestation iterates over three stores for matching values.
Documentation Considerations
Nothing end-user. OCW is documented elsewhere.
Testing Considerations
CI
Upgrade Considerations
not yet deployed.
contract is upgradable but changing the settlementAccountAddress will require creating a new TransactionFeed (as is the case for the Settler object.