From 25d95bdcb959fb92d944c452a6f344bcf9ca87f7 Mon Sep 17 00:00:00 2001 From: edwardmack Date: Fri, 13 Sep 2024 15:08:25 -0400 Subject: [PATCH] address PR comments, rename variables and functions --- .../backing/candidate_backing_test.go | 4 +-- dot/parachain/backing/integration_test.go | 4 +-- .../backing/per_relay_parent_state.go | 8 ++--- .../candidate_validation_test.go | 18 +++++----- dot/parachain/candidate-validation/host.go | 12 +++---- .../candidate-validation/host_test.go | 16 ++++----- dot/parachain/candidate-validation/worker.go | 19 ++++++----- .../candidate-validation/worker_pool.go | 33 ++++++++++--------- .../candidate-validation/worker_pool_test.go | 8 ++--- dot/parachain/runtime/instance.go | 5 +++ 10 files changed, 67 insertions(+), 60 deletions(-) diff --git a/dot/parachain/backing/candidate_backing_test.go b/dot/parachain/backing/candidate_backing_test.go index 4aa1c454b8..eb2c29da12 100644 --- a/dot/parachain/backing/candidate_backing_test.go +++ b/dot/parachain/backing/candidate_backing_test.go @@ -731,7 +731,7 @@ func TestValidateAndMakeAvailable(t *testing.T) { ci := candidatevalidation.ExecutionError data.Ch <- parachaintypes.OverseerFuncRes[candidatevalidation.ValidationResult]{ Data: candidatevalidation.ValidationResult{ - InvalidResult: &ci, + Invalid: &ci, }, } default: @@ -768,7 +768,7 @@ func TestValidateAndMakeAvailable(t *testing.T) { case candidatevalidation.ValidateFromExhaustive: data.Ch <- parachaintypes.OverseerFuncRes[candidatevalidation.ValidationResult]{ Data: candidatevalidation.ValidationResult{ - ValidResult: &candidatevalidation.Valid{}, + Valid: &candidatevalidation.Valid{}, }, } case availabilitystore.StoreAvailableData: diff --git a/dot/parachain/backing/integration_test.go b/dot/parachain/backing/integration_test.go index 52e84a71dd..53f3415554 100644 --- a/dot/parachain/backing/integration_test.go +++ b/dot/parachain/backing/integration_test.go @@ -227,7 +227,7 @@ func validResponseForValidateFromExhaustive( msgValidate.Ch <- parachaintypes.OverseerFuncRes[candidatevalidation.ValidationResult]{ Data: candidatevalidation.ValidationResult{ - ValidResult: &candidatevalidation.Valid{ + Valid: &candidatevalidation.Valid{ CandidateCommitments: parachaintypes.CandidateCommitments{ HeadData: headData, UpwardMessages: []parachaintypes.UpwardMessage{}, @@ -337,7 +337,7 @@ func TestSecondsValidCandidate(t *testing.T) { badReturn := candidatevalidation.BadReturn validateFromExhaustive.Ch <- parachaintypes.OverseerFuncRes[candidatevalidation.ValidationResult]{ Data: candidatevalidation.ValidationResult{ - InvalidResult: &badReturn, + Invalid: &badReturn, }, } return true diff --git a/dot/parachain/backing/per_relay_parent_state.go b/dot/parachain/backing/per_relay_parent_state.go index d9e90fb5a2..4c4b2861af 100644 --- a/dot/parachain/backing/per_relay_parent_state.go +++ b/dot/parachain/backing/per_relay_parent_state.go @@ -340,8 +340,8 @@ func (rpState *perRelayParentState) validateAndMakeAvailable( bgValidationResult = backgroundValidationResult{ outputs: &backgroundValidationOutputs{ candidateReceipt: candidateReceipt, - candidateCommitments: validationResultRes.Data.ValidResult.CandidateCommitments, - persistedValidationData: validationResultRes.Data.ValidResult.PersistedValidationData, + candidateCommitments: validationResultRes.Data.Valid.CandidateCommitments, + persistedValidationData: validationResultRes.Data.Valid.PersistedValidationData, }, candidate: nil, err: nil, @@ -358,11 +358,11 @@ func (rpState *perRelayParentState) validateAndMakeAvailable( } } else { // Invalid - logger.Error(validationResultRes.Data.InvalidResult.Error()) + logger.Error(validationResultRes.Data.Invalid.Error()) bgValidationResult = backgroundValidationResult{ outputs: nil, candidate: &candidateReceipt, - err: fmt.Errorf(validationResultRes.Data.InvalidResult.Error()), + err: fmt.Errorf(validationResultRes.Data.Invalid.Error()), } } diff --git a/dot/parachain/candidate-validation/candidate_validation_test.go b/dot/parachain/candidate-validation/candidate_validation_test.go index d1cfa93115..9e2050ddcc 100644 --- a/dot/parachain/candidate-validation/candidate_validation_test.go +++ b/dot/parachain/candidate-validation/candidate_validation_test.go @@ -164,7 +164,7 @@ func TestCandidateValidation_processMessageValidateFromExhaustive(t *testing.T) }, want: parachaintypes.OverseerFuncRes[ValidationResult]{ Data: ValidationResult{ - InvalidResult: &povHashMismatch, + Invalid: &povHashMismatch, }, Err: nil, }, @@ -184,7 +184,7 @@ func TestCandidateValidation_processMessageValidateFromExhaustive(t *testing.T) }, want: parachaintypes.OverseerFuncRes[ValidationResult]{ Data: ValidationResult{ - InvalidResult: ¶msTooLarge, + Invalid: ¶msTooLarge, }, }, }, @@ -203,7 +203,7 @@ func TestCandidateValidation_processMessageValidateFromExhaustive(t *testing.T) }, want: parachaintypes.OverseerFuncRes[ValidationResult]{ Data: ValidationResult{ - InvalidResult: &codeHashMismatch, + Invalid: &codeHashMismatch, }, }, }, @@ -222,7 +222,7 @@ func TestCandidateValidation_processMessageValidateFromExhaustive(t *testing.T) }, want: parachaintypes.OverseerFuncRes[ValidationResult]{ Data: ValidationResult{ - ValidResult: &Valid{ + Valid: &Valid{ CandidateCommitments: parachaintypes.CandidateCommitments{ HeadData: parachaintypes.HeadData{Data: []byte{2, 0, 0, 0, 0, 0, 0, 0, 123, 207, 206, 8, 219, 227, 136, 82, 236, 169, 14, 100, 45, 100, 31, 177, 154, 160, 220, 245, @@ -388,7 +388,7 @@ func TestCandidateValidation_processMessageValidateFromChainState(t *testing.T) Pov: pov, }, want: &ValidationResult{ - InvalidResult: &povHashMismatch, + Invalid: &povHashMismatch, }, }, "invalid_pov_size": { @@ -397,7 +397,7 @@ func TestCandidateValidation_processMessageValidateFromChainState(t *testing.T) Pov: pov, }, want: &ValidationResult{ - InvalidResult: ¶msTooLarge, + Invalid: ¶msTooLarge, }, }, "code_mismatch": { @@ -406,7 +406,7 @@ func TestCandidateValidation_processMessageValidateFromChainState(t *testing.T) Pov: pov, }, want: &ValidationResult{ - InvalidResult: &codeHashMismatch, + Invalid: &codeHashMismatch, }, }, "bad_signature": { @@ -415,7 +415,7 @@ func TestCandidateValidation_processMessageValidateFromChainState(t *testing.T) Pov: pov, }, want: &ValidationResult{ - InvalidResult: &badSignature, + Invalid: &badSignature, }, }, "happy_path": { @@ -424,7 +424,7 @@ func TestCandidateValidation_processMessageValidateFromChainState(t *testing.T) Pov: pov, }, want: &ValidationResult{ - ValidResult: &Valid{ + Valid: &Valid{ CandidateCommitments: parachaintypes.CandidateCommitments{ HeadData: parachaintypes.HeadData{Data: []byte{2, 0, 0, 0, 0, 0, 0, 0, 123, 207, 206, 8, 219, 227, 136, 82, 236, 169, 14, 100, 45, 100, 31, 177, 154, 160, 220, 245, 59, 106, 76, 168, 122, 109, diff --git a/dot/parachain/candidate-validation/host.go b/dot/parachain/candidate-validation/host.go index 7b0e832273..8b192d82e4 100644 --- a/dot/parachain/candidate-validation/host.go +++ b/dot/parachain/candidate-validation/host.go @@ -8,7 +8,7 @@ import ( "github.com/ChainSafe/gossamer/pkg/scale" ) -var logger = log.NewFromGlobal(log.AddContext("pkg", "pvf"), log.SetLevel(log.Debug)) +var logger = log.NewFromGlobal(log.AddContext("pkg", "candidatevalidation"), log.SetLevel(log.Debug)) // host is the struct that holds the workerPool which is responsible for executing the validation tasks type host struct { @@ -17,13 +17,12 @@ type host struct { func newValidationHost() *host { return &host{ - workerPool: newValidationWorkerPool(), + workerPool: newWorkerPool(), } } func (v *host) validate(msg *ValidationTask) (*ValidationResult, error) { validationCodeHash := msg.ValidationCode.Hash() - // performBasicChecks validationErr, internalErr := performBasicChecks(&msg.CandidateReceipt.Descriptor, msg.PersistedValidationData.MaxPovSize, msg.PoV, @@ -33,15 +32,15 @@ func (v *host) validate(msg *ValidationTask) (*ValidationResult, error) { return nil, internalErr } if validationErr != nil { - return &ValidationResult{InvalidResult: validationErr}, nil //nolint + return &ValidationResult{Invalid: validationErr}, nil //nolint } // submit request return v.workerPool.executeRequest(msg) } -// performBasicChecks Does basic checks of a candidate. Provide the encoded PoV-block. -// Returns ReasonForInvalidity and internal error if any. +// performBasicChecks does basic checks of a candidate. Provided the encoded PoV-block it returns ReasonForInvalidity +// and internal error if any. func performBasicChecks(candidate *parachaintypes.CandidateDescriptor, maxPoVSize uint32, pov parachaintypes.PoV, validationCodeHash parachaintypes.ValidationCodeHash) ( validationError *ReasonForInvalidity, internalError error) { @@ -76,5 +75,6 @@ func performBasicChecks(candidate *parachaintypes.CandidateDescriptor, maxPoVSiz ci := BadSignature return &ci, nil } + return nil, nil } diff --git a/dot/parachain/candidate-validation/host_test.go b/dot/parachain/candidate-validation/host_test.go index e48f45c460..5491dfd4b4 100644 --- a/dot/parachain/candidate-validation/host_test.go +++ b/dot/parachain/candidate-validation/host_test.go @@ -74,7 +74,7 @@ func TestHost_validate(t *testing.T) { ValidationCode: &validationCode, }, want: &ValidationResult{ - InvalidResult: &povHashMismatch, + Invalid: &povHashMismatch, }, isValid: false, }, @@ -91,7 +91,7 @@ func TestHost_validate(t *testing.T) { PoV: pov, }, want: &ValidationResult{ - InvalidResult: ¶msTooLarge, + Invalid: ¶msTooLarge, }, }, "code_mismatch": { @@ -107,7 +107,7 @@ func TestHost_validate(t *testing.T) { PoV: pov, }, want: &ValidationResult{ - InvalidResult: &codeHashMismatch, + Invalid: &codeHashMismatch, }, isValid: false, }, @@ -121,7 +121,7 @@ func TestHost_validate(t *testing.T) { PoV: pov, }, want: &ValidationResult{ - InvalidResult: &executionError, + Invalid: &executionError, }, }, "para_head_hash_mismatch": { @@ -137,7 +137,7 @@ func TestHost_validate(t *testing.T) { PoV: pov, }, want: &ValidationResult{ - InvalidResult: ¶HedHashMismatch, + Invalid: ¶HedHashMismatch, }, isValid: false, }, @@ -154,7 +154,7 @@ func TestHost_validate(t *testing.T) { PoV: pov, }, want: &ValidationResult{ - InvalidResult: &commitmentsHashMismatch, + Invalid: &commitmentsHashMismatch, }, isValid: false, }, @@ -171,7 +171,7 @@ func TestHost_validate(t *testing.T) { PoV: pov, }, want: &ValidationResult{ - ValidResult: &Valid{ + Valid: &Valid{ CandidateCommitments: parachaintypes.CandidateCommitments{ UpwardMessages: nil, HorizontalMessages: nil, @@ -323,7 +323,7 @@ func TestHost_performBasicChecks(t *testing.T) { if tt.expectedError != nil { require.EqualError(t, validationError, tt.expectedError.Error()) } else { - require.Nil(t, validationError) + require.NoError(t, validationError) } }) } diff --git a/dot/parachain/candidate-validation/worker.go b/dot/parachain/candidate-validation/worker.go index 5f45e8e55c..4a38bdb9a5 100644 --- a/dot/parachain/candidate-validation/worker.go +++ b/dot/parachain/candidate-validation/worker.go @@ -5,9 +5,10 @@ import ( parachaintypes "github.com/ChainSafe/gossamer/dot/parachain/types" ) +// worker is the thing that can execute a validation request type worker struct { workerID parachaintypes.ValidationCodeHash - instance *parachainruntime.Instance + instance parachainruntime.ValidatorInstance isProcessed map[parachaintypes.CandidateHash]*ValidationResult } @@ -18,14 +19,15 @@ type workerTask struct { } func newWorker(validationCode parachaintypes.ValidationCode) (*worker, error) { - validationRuntime, err := parachainruntime.SetupVM(validationCode) + parachainRuntime, err := parachainruntime.SetupVM(validationCode) if err != nil { return nil, err } + return &worker{ workerID: validationCode.Hash(), - instance: validationRuntime, + instance: parachainRuntime, isProcessed: make(map[parachaintypes.CandidateHash]*ValidationResult), }, nil } @@ -37,7 +39,6 @@ func (w *worker) executeRequest(task *workerTask) (*ValidationResult, error) { return nil, err } - // do isProcessed check here if processed, ok := w.isProcessed[candidateHash]; ok { logger.Debugf("candidate %x already processed", candidateHash) return processed, nil @@ -46,19 +47,19 @@ func (w *worker) executeRequest(task *workerTask) (*ValidationResult, error) { if err != nil { logger.Errorf("executing validate_block: %w", err) reasonForInvalidity := ExecutionError - return &ValidationResult{InvalidResult: &reasonForInvalidity}, nil + return &ValidationResult{Invalid: &reasonForInvalidity}, nil } headDataHash, err := validationResult.HeadData.Hash() if err != nil { logger.Errorf("hashing head data: %w", err) reasonForInvalidity := ExecutionError - return &ValidationResult{InvalidResult: &reasonForInvalidity}, nil + return &ValidationResult{Invalid: &reasonForInvalidity}, nil } if headDataHash != task.candidateReceipt.Descriptor.ParaHead { reasonForInvalidity := ParaHeadHashMismatch - return &ValidationResult{InvalidResult: &reasonForInvalidity}, nil + return &ValidationResult{Invalid: &reasonForInvalidity}, nil } candidateCommitments := parachaintypes.CandidateCommitments{ UpwardMessages: validationResult.UpwardMessages, @@ -72,7 +73,7 @@ func (w *worker) executeRequest(task *workerTask) (*ValidationResult, error) { // if validation produced a new set of commitments, we treat the candidate as invalid if task.candidateReceipt.CommitmentsHash != candidateCommitments.Hash() { reasonForInvalidity := CommitmentsHashMismatch - return &ValidationResult{InvalidResult: &reasonForInvalidity}, nil + return &ValidationResult{Invalid: &reasonForInvalidity}, nil } pvd := parachaintypes.PersistedValidationData{ ParentHead: task.work.ParentHeadData, @@ -81,7 +82,7 @@ func (w *worker) executeRequest(task *workerTask) (*ValidationResult, error) { MaxPovSize: task.maxPoVSize, } result := &ValidationResult{ - ValidResult: &Valid{ + Valid: &Valid{ CandidateCommitments: candidateCommitments, PersistedValidationData: pvd, }, diff --git a/dot/parachain/candidate-validation/worker_pool.go b/dot/parachain/candidate-validation/worker_pool.go index 2bff748fbd..46f8dad514 100644 --- a/dot/parachain/candidate-validation/worker_pool.go +++ b/dot/parachain/candidate-validation/worker_pool.go @@ -22,20 +22,18 @@ type ValidationTask struct { } // ValidationResult represents the result coming from the candidate validation subsystem. -// Validation results can be either a Valid or InvalidValidationResult. +// Validation results can be either valid or invalid. // -// If the result is invalid, -// store the reason for invalidity in the InvalidResult field of ValidationResult. +// If the result is invalid, store the reason for invalidity. // -// If the result is valid, -// set the values of the ValidResult field of Valid. +// If the result is valid, store persisted validation data and candidate commitments. type ValidationResult struct { - ValidResult *Valid - InvalidResult *ReasonForInvalidity + Valid *Valid + Invalid *ReasonForInvalidity } func (vr ValidationResult) IsValid() bool { - return vr.ValidResult != nil + return vr.Valid != nil } type Valid struct { @@ -107,20 +105,23 @@ func (ci ReasonForInvalidity) Error() string { } } -func newValidationWorkerPool() *workerPool { +func newWorkerPool() *workerPool { return &workerPool{ workers: make(map[parachaintypes.ValidationCodeHash]*worker), } } -func (v *workerPool) newValidationWorker(validationCode parachaintypes.ValidationCode) error { +func (v *workerPool) addNewWorker(validationCode parachaintypes.ValidationCode) error { + workerID := validationCode.Hash() + if !v.containsWorker(workerID) { + worker, err := newWorker(validationCode) + if err != nil { + return fmt.Errorf("failed to create a new worker: %w", err) + } - worker, err := newWorker(validationCode) - if err != nil { - return fmt.Errorf("failed to create a new worker: %w", err) - } + v.workers[workerID] = worker - v.workers[worker.workerID] = worker + } return nil } @@ -133,7 +134,7 @@ func (v *workerPool) executeRequest(msg *ValidationTask) (*ValidationResult, err // create worker if not in pool if !v.containsWorker(validationCodeHash) { - err := v.newValidationWorker(*msg.ValidationCode) + err := v.addNewWorker(*msg.ValidationCode) if err != nil { return nil, err } diff --git a/dot/parachain/candidate-validation/worker_pool_test.go b/dot/parachain/candidate-validation/worker_pool_test.go index bf76e51c69..edf30412c3 100644 --- a/dot/parachain/candidate-validation/worker_pool_test.go +++ b/dot/parachain/candidate-validation/worker_pool_test.go @@ -29,8 +29,8 @@ func TestValidationWorkerPool_newValidationWorker(t *testing.T) { }{ "add_one_invalid_worker": { setupWorkerPool: func(t *testing.T) *workerPool { - pool := newValidationWorkerPool() - err := pool.newValidationWorker(parachaintypes.ValidationCode{1, 2, 3, 4}) + pool := newWorkerPool() + err := pool.addNewWorker(parachaintypes.ValidationCode{1, 2, 3, 4}) require.Error(t, err) return pool }, @@ -38,8 +38,8 @@ func TestValidationWorkerPool_newValidationWorker(t *testing.T) { }, "add_one_valid_worker": { setupWorkerPool: func(t *testing.T) *workerPool { - pool := newValidationWorkerPool() - err := pool.newValidationWorker(testValidationCode) + pool := newWorkerPool() + err := pool.addNewWorker(testValidationCode) require.NoError(t, err) return pool }, diff --git a/dot/parachain/runtime/instance.go b/dot/parachain/runtime/instance.go index 41b01bfff5..acc3829e76 100644 --- a/dot/parachain/runtime/instance.go +++ b/dot/parachain/runtime/instance.go @@ -89,6 +89,11 @@ func (in *Instance) ValidateBlock(params ValidationParameters) ( return &validationResult, nil } +// ValidatorInstance for candidate validation methods +type ValidatorInstance interface { + ValidateBlock(params ValidationParameters) (*ValidationResult, error) +} + // RuntimeInstance for runtime methods type RuntimeInstance interface { ParachainHostPersistedValidationData(parachaidID uint32, assumption parachaintypes.OccupiedCoreAssumption,