feat: single-node implementation of slash-protection signer#20894
feat: single-node implementation of slash-protection signer#20894AztecBot merged 1 commit intomerge-train/spartanfrom
Conversation
PhilWindle
left a comment
There was a problem hiding this comment.
Generally looks ok. Just a couple of questions, things I can't fathom from the PR.
| * Cleanup own stuck duties (SIGNING status older than maxAgeMs). | ||
| */ | ||
| public cleanupOwnStuckDuties(nodeId: string, maxAgeMs: number): Promise<number> { | ||
| const cutoffMs = Date.now() - maxAgeMs; |
There was a problem hiding this comment.
Can we use a DateProvider here?
| * Cleanup old signed duties older than maxAgeMs. | ||
| */ | ||
| public cleanupOldDuties(maxAgeMs: number): Promise<number> { | ||
| const cutoffMs = Date.now() - maxAgeMs; |
| lockToken, | ||
| startedAtMs: now, | ||
| }; | ||
| await this.duties.set(key, newRecord); |
There was a problem hiding this comment.
@alexghr should we serialise this to a Buffer before writing to the DB?
There was a problem hiding this comment.
I'll let Alex answer that but I think not because all properties are primitives
There was a problem hiding this comment.
It's fine considering all the values are strings/numbers. It would've have been a problem if it contained bigints.
| const db = new LmdbSlashingProtectionDatabase(kvStore); | ||
|
|
||
| // haSigningEnabled must be true for ValidatorHASigner; override it since in local mode | ||
| // the flag is false (it refers to the distributed Postgres-backed HA mode). |
There was a problem hiding this comment.
Not sure I follow, what is this override for?
There was a problem hiding this comment.
for high-availability signer, I added this config haSigningEnabled which must be true for the signer to run.
We use the same signer implementation here but with a different underlying DB so it still requires the config to be true, but for local protected signing, it won't be.
I'm thinking now that may be better to remove the requirement for haSigningEnabled to be true
| lockToken, | ||
| startedAtMs: now, | ||
| }; | ||
| await this.duties.set(key, newRecord); |
There was a problem hiding this comment.
It's fine considering all the values are strings/numbers. It would've have been a problem if it contained bigints.
| const result = await this.store.transactionAsync(async () => { | ||
| const existing = await this.duties.getAsync(key); | ||
| if (existing) { | ||
| return { isNew: false as const, record: existing }; |
There was a problem hiding this comment.
should this delete the lockToken before returning?
| * Cleanup duties with outdated rollup address. | ||
| */ | ||
| public cleanupOutdatedRollupDuties(currentRollupAddress: EthAddress): Promise<number> { | ||
| const currentAddr = currentRollupAddress.toString(); |
There was a problem hiding this comment.
I was thinking. Instead of this, all other database instances just delete the database and start again whenever there is a change in the rollup address. Surely this would simplify things here wouldn't it?
There was a problem hiding this comment.
ah yes, this actually already happens at the store creation level. when passing l1Contracts to createStore, it resets the DB for older contracts.
So this function is actually a no-op, the db we're managing is only valid for the current rollupContract
Fixes [A-477](https://linear.app/aztec-labs/issue/A-477/lightweight-ha-like-signer-to-prevent-duplicate-proposals) - Introduces `LocalSignerWithProtection` which will be used by default if no high-availability config has been provided - Moves around a bunch of things bc `DataStoreConfig` had to be moved into `stdlib` in order to be required by `validator-ha-signer` Co-authored-by: Alex Gherghisan <alexghr@users.noreply.github.com>
fc02322 to
bea8dd7
Compare
|
❌ Failed to cherry-pick to |
…20894) Cherry-pick of c5cc991 with conflict resolution for v4. Key adaptations for v4: - Replaced @aztec/stdlib/ha-signing imports with local imports within validator-ha-signer (types already moved to this package on v4) - Added HA_SIGNER_* metric definitions and HA_DUTY_TYPE/HA_NODE_ID attributes to telemetry-client (these were added in a prior PR on next) - Updated SlashingProtectionService and ValidatorHASigner constructors to accept deps parameter (metrics + dateProvider) - Added new package exports: ./local-config, ./metrics - Added LMDB-backed local signing protection database - Updated all test files to use new constructor signatures
BEGIN_COMMIT_OVERRIDE test: update proving-real test to mbps (#20991) chore: epoch proving log analyzer (#21033) chore: update pause script to allow resume (#21032) feat: price bump for RPC transaction replacement (#20806) refactor: remove update checker, retain version checks (#20898) fix: (A-592) p2p client proposal tx collector test (#20998) refactor: use publishers-per-pod in deployments (#21039) chore: web3signer refreshes keystore (#21045) feat(sequencer): set block building limits from checkpoint limits (#20974) chore(e2e): fix e2e bot L1 tx nonce reuse (#21052) feat: Update L1 to L2 message APIs (#20913) fix: (A-589) epochs l1 reorgs test (#20999) feat(sequencer): add SEQ_MAX_TX_PER_CHECKPOINT config (#21016) fix: drop --pid=host from docker_isolate (#21081) feat: standby mode for prover broker (#21098) fix(p2p): remove default block handler in favor of block handler (#21105) feat(validator): add VALIDATOR_ env vars for independent block limits (#21060) refactor(p2p): decouple proposal validators from base class via composition (#21075) feat: additional validation in public setup allowlist (onlySelf + null msg sender) (#21122) fix: (A-591) aztecProofSubmissionEpochs incorrectly named as aztecProofSubmissionWindow (#21108) refactor(sequencer): rename SEQ_GAS_PER_BLOCK_ALLOCATION_MULTIPLIER to SEQ_PER_BLOCK_ALLOCATION_MULTIPLIER (#21125) fix: unbound variable in check_doc_references.sh with set -u (#21126) feat: calldata length validation of public setup function allowlist (#21139) fix: include mismatched values in tx metadata validation errors (#21147) feat: single-node implementation of slash-protection signer (#20894) feat: Remove non-protocol contracts from public setup allowlist (#21154) chore: More updated Alpha configuration (#21155) chore: tally slashing pruning improvements (#21161) fix: update dependencies (#20997) fix: omit bigint priceBumpPercentage from IPC config in testbench worker (#21169) refactor(p2p): (A-588) maintain sorted array in tx pool instead of sorting on read (#21079) fix(p2p): report most severe failure in runValidations (#21185) fix: use dedicated L1 account for bot bridge resume tests to avoid nonce race (#21148) fix: parse error.message in formatViemError (#21163) fix: bump lighthouse consensus client v7.1.0 -> v8.0.1 (#21170) chore: code decuplication + refactor (public setup allowlist) (#21200) END_COMMIT_OVERRIDE
Fixes A-477
LocalSignerWithProtectionwhich will be used by default if no high-availability config has been providedDataStoreConfighad to be moved intostdlibin order to be required byvalidator-ha-signer