Vault: include-invalid pending items and relaxed GetSecrets consensus - #22736
Conversation
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
| return gateAllows(ctx, r.lggr, r.cfg.VaultOptimizationsEnabled, "VaultOptimizationsEnabled") | ||
| } | ||
|
|
||
| func (r *ReportingPlugin) ciphertextlessObservationsEnabled(ctx context.Context) bool { |
There was a problem hiding this comment.
Nit: worth inlining IMO -- I don't think you gain much by abstracting this out
There was a problem hiding this comment.
We have this practice with all our gates, the one liner is long so this make it easier to read when you put the gate directly into an if
| Result: &vaultcommon.SecretResponse_Data{ | ||
| Data: &vaultcommon.SecretData{ | ||
| EncryptedValue: hex.EncodeToString(secret.EncryptedSecret), | ||
| EncryptedValue: encryptedValueForGetSecretsObservation(r.ciphertextlessObservationsEnabled(ctx), secret.EncryptedSecret), |
There was a problem hiding this comment.
I do wonder if this has some weird error modes and I think we should ask research to take a look.
One possibility that springs to mind: we generate the shares during Observation, but then a request in the same batch updates the secret. If this is processed before the get request in StateTransition, the shares will effectively be for an outdated version of the ciphertext.
It might be safer to hash the ciphertext so that we can effectively "pin" the shares to a particular ciphertext to avoid issues like this.
There was a problem hiding this comment.
There's a caching system in StateTransition now so you always get the ciphertext that was present at the start of the round even if that same secret is updated before you get it
| } | ||
| continue | ||
| } | ||
| resp.GetData().EncryptedValue = hex.EncodeToString(stored.EncryptedSecret) |
There was a problem hiding this comment.
You shouldn't use a getter when writing; this will initialize a nil Data if it doesn't exist which may lead to unexpected results. You're guarding against that above, so you can just use resp.Data.EncryptedValue = foo
There was a problem hiding this comment.
It doesn't allow me to access resp.Data directly
e9091b8 to
3192138
Compare
… into validate_observations
… into validate_observations
… into validate_observations
59d3763 to
97fde95
Compare
|
/vault-audit |
|
✅ Vault audit complete for 📋 View the full report and findings — private tracking issue, visible to chainlink team members only. Resolve blocking findings there with |
… into validate_observations
| // StateTransition-only and are intentionally not checked here. | ||
| func (r *ReportingPlugin) validateContribution( | ||
| ctx context.Context, | ||
| _ ReadKVStore, |
There was a problem hiding this comment.
Nit: I would remove this if it's not needed
| @@ -0,0 +1,297 @@ | |||
| package vault | |||
There was a problem hiding this comment.
General style nit that I don't think we need to address at this stage: there are a lot of small functions that we're calling into and the implementation of each is pretty trivial and could be inlined.
| return false | ||
| } | ||
| // The pending queue must never be purged with fewer than 2f+1 stall | ||
| // signals, regardless of the configured stall threshold. |
There was a problem hiding this comment.
@russell-stern I'm not sure I understand this -- it looks like the pendingQueueStallTracker simply counts the amount of time the current node has seen the same sequence number, and we are applying a minimum of 2F+1 as a threshold to it. That threshold is usually applied by counting the number of observations across the nodes, which we're not doing here 🤔
There was a problem hiding this comment.
This threshold is for how many times we can see the same sequence number before the node says "we're stuck". There's an attack vector if the threshold is ever set <= 2f so this is guaranteeing that we can never configure the vault with a small threshold
… into validate_observations
… into validate_observations
|
4 participants
Summary
Updates the vault to require a prefix of the pending queue to validate observations. We also guarantee that at least 1 request can be processed before reaching quorum for state transition. Part of this is relaxing get secrets consensus in state transition to be 2f+1 observations seeing the request but only at least f+1 shares need to be contributed.
All changes are gated behind feature flags