Keystone: Forwarder contract updates #12962
Conversation
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(s_signers[i].mockPrivateKey, hash); | ||
| // rs[i] = r; | ||
| // ss[i] = s; | ||
| // vs[i] = bytes1(v - 27); |
There was a problem hiding this comment.
OCR2Base seems to do this - 27 subtraction but the signature doesn't validate for me then
|
Go solidity wrappers are out-of-date, regenerate them via the |
|
Go solidity wrappers are out-of-date, regenerate them via the |
|
Go solidity wrappers are out-of-date, regenerate them via the |
1 similar comment
|
Go solidity wrappers are out-of-date, regenerate them via the |
| // TODO: we probably need some type of f value config? | ||
| (/* bytes32 workflowId */, bytes4 donId, bytes32 workflowExecutionId) = Utils._splitReport(rawReport); | ||
|
|
||
| if (signatures.length != s_configs[donId].f + 1) { |
There was a problem hiding this comment.
| if (signatures.length != s_configs[donId].f + 1) { | |
| if (signatures.length < s_configs[donId].f + 1) { |
We could get more than f+1 signatures, no?
There was a problem hiding this comment.
libocr's version is strict in the number of signatures expected, though mostly so we don't reimburse gas costs for redundant signature verifications
| if (signers.length > MAX_ORACLES) revert ExcessSigners(signers.length, MAX_ORACLES); | ||
| if (signers.length <= 3 * f) revert InsufficientSigners(signers.length, 3 * f + 1); | ||
|
|
||
| // TODO: how does setConfig handle expiration? e.g. if the signer set changes |
There was a problem hiding this comment.
Nit: I typically expect to see a link to the Jira ticket if you leave the TODO in the code. Otherwise, these aren't visible in Jira.
There was a problem hiding this comment.
@archseer what do you mean by expiration?
There was a problem hiding this comment.
In OCR2Aggregator changing the config also expires any past in-flight transmissions since the config digest will change (and needs to match on transmit).
There was a problem hiding this comment.
We're not sending the digest alongside our reports are we? I thought we will only rely purely on signature validation.
There was a problem hiding this comment.
We don't but the question is do we want something equivalent for expiration?
There was a problem hiding this comment.
Created a ticket to investigate: https://smartcontract-it.atlassian.net/browse/KS-202?atlOrigin=eyJpIjoiMGM1NGQ4ZTFmMDg1NDZlZjgxOWIyNDA4MjUzZGJjOTYiLCJwIjoiaiJ9
|
|
||
| // NOTE: we don't inherit OCR2Base since unlike aggregator we only care about signers, not transmitters | ||
| // and the signers don't fetch their config from the forwarder | ||
| function setConfig(bytes4 donId, uint8 f, address[] calldata signers) external nonReentrant { |
There was a problem hiding this comment.
Given that we control this and will need to build tools anyways, wouldn't it be cheaper for us to expose a getSigners function so we can calculate the diff off-chain and then provide address[] calldata signersToRemove, address[] calldata signersToAdd arguments? This way, we wouldn't need to iterate through all the signers 🤷♂️ just a thought
There was a problem hiding this comment.
Let's do this as part of the code golf optimizations later, it requires slightly more work on the tooling side than a single idempotent call
DeividasK
changed the title
DeividasK
force-pushed
the
keystone-validate-signers
branch
from
800894a to
a431ed3
Compare
…signers # Conflicts: # core/gethwrappers/keystone/generation/generated-wrapper-dependency-versions-do-not-edit.txt
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
| protoc-gen-go | ||
| protoc-gen-go-grpc | ||
|
|
||
| foundry-bin |
There was a problem hiding this comment.
These appear to have broken the nix shell for me:
error: builder for '/nix/store/g9g5kmnv6hs4bv0hgzf0jv9nnr4k0n3l-source.drv' failed with exit code 1;
last 7 log lines:
>
> trying https://github.com/foundry-rs/foundry/releases/download/nightly-bfc6549f0d50fe31cd2fae875c2c7233db98bde9/foundry_nightly_darwin_amd64.tar.gz
> % Total % Received % Xferd Average Speed Time Time Time Current
> Dload Upload Total Spent Left Speed
> 0 9 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
> curl: (22) The requested URL returned error: 404
> error: cannot download source from any mirror
For full logs, run 'nix-store -l /nix/store/g9g5kmnv6hs4bv0hgzf0jv9nnr4k0n3l-source.drv'.
error: 1 dependencies of derivation '/nix/store/yvwz6v83vxqi2razmmdd4lrd0552fsf9-foundry-0.0.0.drv' failed to build
error: 1 dependencies of derivation '/nix/store/g4c68z4l24vpgcsgrhqkjx1ki7vb0ii2-nix-shell-env.drv' failed to build
https://github.com/foundry-rs/foundry/releases/tag/nightly-bfc6549f0d50fe31cd2fae875c2c7233db98bde9 does not exist. There is no tag for this so it can't download it.
No description provided.