dkg: add --no-verify for hash verifications #1090
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1090 +/- ##
==========================================
- Coverage 53.67% 53.52% -0.15%
==========================================
Files 127 127
Lines 14309 14246 -63
==========================================
- Hits 7680 7625 -55
+ Misses 5546 5544 -2
+ Partials 1083 1077 -6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
dkg/disk.go
Outdated
| @@ -59,6 +60,18 @@ func loadDefinition(ctx context.Context, conf Config) (cluster.Definition, error | |||
| return cluster.Definition{}, errors.Wrap(err, "unmarshal definition") | |||
| } | |||
|
|
|||
| // Verify config hash and definition hash from json string and resultant cluster.Definition. | |||
| if !conf.NoVerify { | |||
| expected, err := json.MarshalIndent(res, "", " ") | |||
There was a problem hiding this comment.
requiring a fixed json indentation as part of verification isn't correct. The json indentation shouldn't matter.
There was a problem hiding this comment.
yeah right, corrected 👍
cluster/definition.go
Outdated
| @@ -169,6 +169,58 @@ func (d Definition) Verify() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (d Definition) VerifyHashesJSON(data []byte) error { | |||
There was a problem hiding this comment.
this API doesn't feel good. Needing to pass the raw bytes again doesn't feel intuitive.
I would go for something like:
var d Definition
err = json.Unmarshal(rawBytes, &d)
err = d.VerifyHashes()
For this to work, you need to unmarshal the hashes into private unexported fields so you can calculate the actual hashes and compare.
There was a problem hiding this comment.
i was also thinking the same to include this as private fields but was hesitant to do any changes in Definition type
There was a problem hiding this comment.
for this to work we would need to do json marshal every time when new Definition is called, this doesn't feel right
There was a problem hiding this comment.
suggest: keep VerifyHashesJSON(data []byte) error but as stateless function which would introduce two function parameters as VerifyHashesJSON(rawBytes, configHash, defHash []byte) error ?
| @@ -83,7 +83,6 @@ func TestEncode(t *testing.T) { | |||
| require.NoError(t, err) | |||
|
|
|||
| require.Equal(t, b1, b2) | |||
| require.Equal(t, definition, definition2) | |||
There was a problem hiding this comment.
if raw bytes are equal then it doesn't make sense for this require
There was a problem hiding this comment.
not sure about this, why not just leave it here, it might catch some weird edge case, especially since we now have fields that are not marshalled?
There was a problem hiding this comment.
rather add an ``Equal` method to Definition
| @@ -83,7 +83,6 @@ func TestEncode(t *testing.T) { | |||
| require.NoError(t, err) | |||
|
|
|||
| require.Equal(t, b1, b2) | |||
| require.Equal(t, definition, definition2) | |||
There was a problem hiding this comment.
not sure about this, why not just leave it here, it might catch some weird edge case, especially since we now have fields that are not marshalled?
| @@ -169,6 +175,30 @@ func (d Definition) Verify() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (d Definition) VerifyHashes() error { | |||
| require.NoError(t, err) | ||
|
|
||
| // Invalid definition without definition_hash and config_hash | ||
| invalidFile2 := "invalid-cluster-definition2.json" | ||
| defJSON := []byte(`{"name":"test cluster","operators":[{"address":"0x9A4C8145c7457b0BDC84Ba46729c3c9e15b56106","enr":"enr:-Iu4QFTSWOu_OplK-CYUv29EqIoMGQGtuHjTxLohMxOEMSxYFqraJdtWfMiwzS9wiGH-gB32IrYdyXSH-i5nJbLTm4yAgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQJ9hgLS0tOo-w3eLfoHVnENCJuN5QXDgtCX_cHQo5FpDoN0Y3CC52eDdWRwgudo","config_signature":"0x84b5e48201deebc59ac09c2aaec57870cad357d5a0b65a1954ee301b2760597d479a8ee2b6037963554b5323fb7944a389b9e86948ad89da08d6fffcc4ba5c5a01","enr_signature":"0x1530ab4bf5267f88c76850f9750e328698e0206f70a29bf2a6136cec3ffc365e620bddcd46ecfe667718dd6770b38c7c4c7bdf1e6c8ecaf0bc098d3959d9ae0c00"},{"address":"0x79AB788F445d5A689C34AD6e54e247865DE41E99","enr":"enr:-Iu4QDKAQ7dsqHud5m1T2FsjYcahgYRrzMiCZjjx9sRTOjnWH67n8ZEepVZ4WHp-XNn0c0CtFIB-KSBHeiKe8oDLztiAgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQPFe0POIvtf3fkTXUjkj7yLDJ2APptRF6CK8_9C8g0NvYN0Y3CC52mDdWRwgudq","config_signature":"0xc8a306ec102a83dce35be9a552d0b30a09e74b1c1b2316596245c0e8f2ffa5d063051373993ecb694d82e2d65a398ecd5ef06599661903470385998d19a702f001","enr_signature":"0x61c44ee84dfc3991fe9de0d25f2bc3cad7feb0d3341208c894f455a233bcb1d2647b4fd29c376ceb2e5d2990fb2af7b6eae10bfe5b55963404ea2571222b350c01"},{"address":"0xfDdd1CF7733Fd8a638020e963792f9Fcc0182Bf4","enr":"enr:-Iu4QNgEtRy6wbpdPCXrj52_rF4Ur7OQf6mOg1xfRpmzPgRYW-QSA-oUslOTmIPL8etUIg95quQoRJg9FIILIa6990uAgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQLM5474DZHwbqwSbQFLrAO8PNh2AdZOXTYGy1ZItyDJaoN0Y3CC52uDdWRwguds","config_signature":"0xddb059b67e0603c0073536225e3a5ae5a7eeae178ec4c31dda521ed9b0209dc90b7e3295ee5ad4f35fbe0b8d62adf4aa4cfbddd3106f7563d234e0b134d9e93701","enr_signature":"0x3c556e60c4b44a4ea015f860759fa01e2e8664d105455b71e55845f451d7ec5e048ee56999c9f5333024bfcc087471fcb7d8e8a2685cb59d63266b4df092104501"}],"uuid":"04513690-AA41-CE01-6281-7901E9FB6D87","version":"v1.2.0","timestamp":"2022-09-07T18:46:30+05:30","num_validators":1,"threshold":2,"fee_recipient_address":"0xd805a5fcea20d3d27d3eee59d5dd5749e3271617","withdrawal_address":"0x75e896f172869cf3ade31c97f681cc1a4015ceed","dkg_algorithm":"default","fork_version":"0x00000000"}`) |
There was a problem hiding this comment.
not a fan of this wall of text. Making changes to this is impossible. Rather:
- unmarshal valid definition above from disk into map[string]any
- delete the config and definition hashes
- marshal to disk
| }) | ||
| } | ||
| } | ||
|
|
||
| func compareDefinitions(t *testing.T, a, b cluster.Definition) bool { |
There was a problem hiding this comment.
rather add an Equal method to Definition
|
taken care by #1099 |
Configure --no-verify for config_hash and definition_hash verifications.
category: refactor
ticket: #1084