-
Notifications
You must be signed in to change notification settings - Fork 578
feat: ValidatorKeystore implementation for high-availability signer #19094
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
base: next
Are you sure you want to change the base?
Conversation
ab1a508 to
662e326
Compare
662e326 to
9c10b83
Compare
d716b2c to
1882379
Compare
1882379 to
7e10af7
Compare
Fixes [A-352](https://linear.app/aztec-labs/issue/A-352/add-postgresql-integration-for-recording-signed-duties) [A-353](https://linear.app/aztec-labs/issue/A-353/implement-core-slash-protection-logic) Introduces High-Availability validator signer. The signer uses a Postgres DB that will be used by multiple sequencer nodes, running with the same set of validator keys, in order to ensure that only one payload per duty is signed.
250ec79 to
b1c57a9
Compare
| return undefined; | ||
| } | ||
| if (err instanceof SlashingProtectionError) { | ||
| this.log.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are defining this as normal, it shouldn't be a warning should it? This will scare people.
| return undefined; | ||
| } | ||
| if (err instanceof SlashingProtectionError) { | ||
| this.log.warn(`Attestations signature for slot ${this.slot} blocked by slashing protection`, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
| ? { | ||
| slot: proposal.slotNumber, | ||
| blockNumber, | ||
| blockIndexWithinCheckpoint: -1, // -1 indicates not applicable (attestation, not a block proposal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this should be optional rather than set to -1 whenevre it's not a block proposal being signed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah would actually make sense to make it optional in SigningContext and just use -1 at the bottom DB layer
| this.log.info(`Assembling checkpoint proposal for slot ${header.slotNumber}`); | ||
| return this.createBlockProposal(0 as BlockNumber, header, archive, txs, proposerAddress, options); | ||
| // Derive checkpoint number from the header's slot | ||
| const checkpointNumber = header.slotNumber as unknown as CheckpointNumber; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't right. Slot number !== Checkpoint number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, missed this one. fixing
| public readonly slot: bigint, | ||
| public readonly slot: SlotNumber, | ||
| public readonly dutyType: DutyType, | ||
| public readonly blockIndexWithinCheckpoint: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should make a new branded type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might need @spalladino input on this, I know he has more work on checkpoints in progress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a TODO for that, heh
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
Fixes A-354
Implements
ExtendedValidatorKeyStoreutilizingValidatorHASigner.Updated interfaces to return
nullwhen the signature isn't acquired & updated handling of signing fornullresults.Also some cleanup on previous HA work like consolidating config