Replace hash-based validation of migrated Cadence values to use Equal()
#5204
Conversation
This change replaces hash-based validation of Cadence values in order to prepare for atree inlining to be incorporated into the migration program. Also added validate flag to enable validation during migration since enabling validation can increase duration of migration.
fxamacker
changed the title
Equal()
fxamacker
requested review from
janezpodhostnik,
zhangchiqing,
ramtinms,
turbolent and
SupunS
| } | ||
|
|
||
| // NoopRuntimeInterface is a runtime interface that can be used in migrations. | ||
| type NoopRuntimeInterface struct { |
There was a problem hiding this comment.
Can we reuse MigrationRuntimeInterface so we dont have to create a new one?
There was a problem hiding this comment.
I didn't reuse MigrationRuntimeInterface code because it contains many fields that are not needed and I wanted to avoid complications if we need to make modifications.
On the other hand, we might be able to reuse NoopRuntimeInterface because it is bare.
| newValue := newStorageMap.ReadValue(nopMemoryGauge, interpreter.StringStorageMapKey(stringKey)) | ||
|
|
||
| if !cadenceValueEqual(oldRuntime.Interpreter, oldValue, newRuntime.Interpreter, newValue) { | ||
| return fmt.Errorf("failed to validate domain %s, key %s: old value %v (%T), new value %v (%T)", domain, key, oldValue, oldValue, newValue, newValue) |
There was a problem hiding this comment.
What will happen here if the value is a 100GB cadence value...
Would it be better to only log the deepest most difference instead of the outermost?
i.e. if difference is in
- composite A:
- array B:
- element 2:
- Composite C:
- field D
- Composite C:
- element 2:
- array B:
we would log something like: "A.B[2].C.D (which is 'asdf') is different from A'.B'[2].C'.D' (which is 'asdfg')"
instead of entire A and entire A'
But then again, this is only relevant if we actually find a difference, so it might be premature to think about this.
There was a problem hiding this comment.
Good point! Done in 706b5a5:
- default: modified error message to include trace and only include values at the deepest level.
- non-default flag:
--log-verbose-validation-errorenables logging of entire Cadence values on validation error during atree migration.
For example, this is a test snippet for non-verbose error msg and verbose error log:
wantErrorMsg := "failed to validate value for address 0000000000000001, domain storage, key 0: failed to validate ([AnyStruct][0]).([UInt64][1]): values differ: 1 (interpreter.UInt64Value) != 2 (interpreter.UInt64Value)"
wantVerboseMsg := "{\"level\":\"info\",\"address\":\"0000000000000001\",\"domain\":\"storage\",\"key\":\"0\",\"trace\":\"failed to validate ([AnyStruct][0]).([UInt64][1]): values differ: 1 (interpreter.UInt64Value) != 2 (interpreter.UInt64Value)\",\"old value\":\"[[0, 1]]\",\"new value\":\"[[0, 2]]\",\"message\":\"failed to validate value\"}\n"
|
Great work. I'm a bit concerned with the. validation output, but thats not actually a problem if the validation passes (which it should in the end) but it might be a bit problematic while we are still debugging. |
| } | ||
|
|
||
| // Iterate through all domains and compare cadence values. | ||
| for _, domain := range domains { |
There was a problem hiding this comment.
Can we let the caller pass the domains as an argument?
There was a problem hiding this comment.
Good idea but in this case, it's not needed because we only deal with a hard coded set of domains defined in migration PR 4633.
| return false | ||
| } | ||
|
|
||
| if !v.StaticType(vInterpreter).Equal(otherComposite.StaticType(otherInterpreter)) || |
There was a problem hiding this comment.
Just to confirm, passing this check means v and other have the same fields, right? In other words, no need to check if other has additional fields that v doesn't have?
There was a problem hiding this comment.
Great catch! 👍
StaticType doesn't check fields, so I added a temporary workaround for this.
I'll open PR to improve this workaround when Cadence PR 3002 is merged and the function I requested becomes available.
Added flag --log-verbose-validation-error to enable logging of entire Cadence values on validation error during atree migration.
This is a temporary (and inefficient) workaround to check number of fields in Cadence CompositeValue. In the future, CompositeValue.FieldCount() will be added to Cadence, so that function can be used to improve this workaround.
Good point! I improved this and described it at #5204 (comment) to have non-verbose default and a flag for more verbose logging:
Although I'm merging this PR to unblock others, it would be great if you can take a look at this change to logging/tracing. |
fxamacker
deleted the
fxamacker/atree-migration-cadence-value-validation
branch
Updates PR #4633
Updates #4576
Updates onflow/cadence#2983
This change replaces hash-based validation of Cadence values in PR #4633 in order to prepare for atree inlining to be incorporated into the migration program.
Also added
--validateflag to enable validation during migration since enabling validation can increase duration of migration.Enabling validation will use
cadence.Equal()to compare every Cadence value before/after migration.