search

status-im / nim-eth

Common utilities for Ethereum
https://nimbus.status.im
Apache License 2.0
83 stars 31 forks source link

Prevent silent failures in RLP serialization #15

Open arnetheduck opened 5 years ago

arnetheduck commented 5 years ago

Right now, a default serializer will swallow any type that's passed to RLP - this makes the serialization easy to break by simply forgetting an import, as can be seen in https://github.com/status-im/nim-beacon-chain/pull/144

Instead of having an overload that silently swallows mistakes, make enabling of the default serializer explicit. Consider both intrusive (pragma) and non-intrusive (for 3rd party types) options .

zah commented 5 years ago

Just a slight clarification that the issue above was not about a missing import, but rather about the presence of some empty "placeholder" procs. When you put your serialization procs in the module where the types are defined, it's much harder to make a mistake, but the usage is a bit more tricky when you import and re-export certain symbols and modules. The improved solution can attempt to detect and prevent such bad re-export situations.

arnetheduck commented 5 years ago

looks like we hit this again, with https://github.com/status-im/nim-beacon-chain/pull/173 which causes silent failures.

tersec commented 5 years ago

For reference, I'm not sure how silent the failure is -- the reason I thought it would work is that when I just (Crosslink's Epoch chosen here) put an Epoch where it'll try to get serialized by RLP, I get:

../beacon_chain/sync_protocol.nim(65, 13) template/generic instantiation from here                                                                                                                                                                                               
../beacon_chain/sync_protocol.nim(161, 5) template/generic instantiation from here                                                                                                                                                                                               
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(215, 11) template/generic instantiation from here                                                                                                                                                                                
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(237, 9) template/generic instantiation from here                                                                                                                                                                                 
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(229, 21) template/generic instantiation from here                                                                                                                                                                                
../../.nimble/pkgs/eth-1.0.0/eth/rlp/object_serialization.nim(19, 9) template/generic instantiation from here                                                                                                                                                                    
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(227, 13) template/generic instantiation from here                                                                                                                                                                                
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(215, 11) template/generic instantiation from here                                                                                                                                                                                
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(237, 9) template/generic instantiation from here                                                                                                                                                                                 
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(229, 21) template/generic instantiation from here                                                                                                                                                                                
../../.nimble/pkgs/eth-1.0.0/eth/rlp/object_serialization.nim(19, 9) template/generic instantiation from here                                                                                                                                                                    
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(227, 13) template/generic instantiation from here                                                                                                                                                                                
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(237, 9) template/generic instantiation from here                                                                                                                                                                                 
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(229, 21) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/object_serialization.nim(19, 9) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(227, 13) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(237, 9) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(229, 21) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/object_serialization.nim(19, 9) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(227, 13) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(237, 9) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(229, 21) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/object_serialization.nim(19, 9) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(227, 13) template/generic instantiation from here
../../.nimble/pkgs/eth-1.0.0/eth/rlp/writer.nim(251, 15) Error: type mismatch: got <RlpWriter, Epoch>
but expected one of: 
proc appendImpl(self; data: string)
  first type mismatch at position: 2
  required type: string
  but expression 'obj.epoch' is of type: Epoch
proc appendImpl(self; data: MemRange)
  first type mismatch at position: 2
  required type: MemRange
  but expression 'obj.epoch' is of type: Epoch
template appendImpl(self; i: Integer)
  first type mismatch at position: 2
  required type: Integer
  but expression 'obj.epoch' is of type: Epoch
template appendImpl(self; e: enum)
  first type mismatch at position: 2
  required type: enum
  but expression 'obj.epoch' is of type: Epoch
template appendImpl(self; b: bool)
  first type mismatch at position: 2
  required type: bool
  but expression 'obj.epoch' is of type: Epoch
proc appendImpl[T](self; listOrBlob: openArray[T])
  first type mismatch at position: 2
  required type: openarray[T]
  but expression 'obj.epoch' is of type: Epoch
proc appendImpl(self; data: object)
  first type mismatch at position: 2
  required type: object
  but expression 'obj.epoch' is of type: Epoch
proc appendImpl(self; data: tuple)
  first type mismatch at position: 2
  required type: tuple
  but expression 'obj.epoch' is of type: Epoch

Specifically, that's if I do:

diff --git a/beacon_chain/spec/beaconstate.nim b/beacon_chain/spec/beaconstate.nim
index 6fb2c34..316c198 100644
--- a/beacon_chain/spec/beaconstate.nim
+++ b/beacon_chain/spec/beaconstate.nim
@@ -218,7 +218,7 @@ func get_genesis_beacon_state*(

   for i in 0 ..< SHARD_COUNT:
     state.latest_crosslinks[i] = Crosslink(
-      epoch: GENESIS_EPOCH.uint64, crosslink_data_root: ZERO_HASH)
+      epoch: GENESIS_EPOCH, crosslink_data_root: ZERO_HASH)

   # Process genesis deposits
   for deposit in genesis_validator_deposits:
@@ -413,7 +413,7 @@ proc checkAttestation*(
       attestation.data.latest_crosslink,
       Crosslink(
         crosslink_data_root: attestation.data.crosslink_data_root,
-        epoch: slot_to_epoch(attestation_data_slot).uint64)]):
+        epoch: slot_to_epoch(attestation_data_slot))]):
     warn("Unexpected crosslink shard",
       state_latest_crosslinks_attestation_data_shard =
         state.latest_crosslinks[attestation.data.shard],
diff --git a/beacon_chain/spec/datatypes.nim b/beacon_chain/spec/datatypes.nim
index 1a01b11..2e0d584 100644
--- a/beacon_chain/spec/datatypes.nim
+++ b/beacon_chain/spec/datatypes.nim
@@ -451,7 +451,7 @@ type

   # https://github.com/ethereum/eth2.0-specs/blob/0.4.0/specs/core/0_beacon-chain.md#crosslink
   Crosslink* = object
-    epoch*: uint64 ##\
+    epoch*: Epoch ##\
     ## Epoch number

     crosslink_data_root*: Eth2Digest ##\
diff --git a/beacon_chain/state_transition.nim b/beacon_chain/state_transition.nim
index e8a4caa..653986f 100644
--- a/beacon_chain/state_transition.nim
+++ b/beacon_chain/state_transition.nim
@@ -717,7 +717,7 @@ func processEpoch(state: var BeaconState) =
         if 3'u64 * total_attesting_balance(crosslink_committee) >=
             2'u64 * get_total_balance(state, crosslink_committee.committee):
           state.latest_crosslinks[crosslink_committee.shard] = Crosslink(
-            epoch: slot_to_epoch(slot).uint64,
+            epoch: slot_to_epoch(slot),
             crosslink_data_root: winning_root(crosslink_committee))

   # https://github.com/ethereum/eth2.0-specs/blob/0.4.0/specs/core/0_beacon-chain.md#rewards-and-penalties