Transfer - split process_transfer/processTransfers + tests + fixes (#422

)

* Prepare test suite for transfers

* split API process_transfer / processTransfers

* Add range checks on transfer

* Fix invalid transfer conditions

* don't test on windows 64-bit #435

beacon_chain/spec/state_transition_block.nim

@@ -414,78 +414,97 @@ proc processVoluntaryExits(state: var BeaconState, blck: BeaconBlock, flags: Upd
   return true
 
 # https://github.com/ethereum/eth2.0-specs/blob/v0.7.1/specs/core/0_beacon-chain.md#transfers
-proc processTransfers(state: var BeaconState, blck: BeaconBlock,
-                      flags: UpdateFlags, stateCache: var StateCache): bool =
-  if not (len(blck.body.transfers) <= MAX_TRANSFERS):
-    notice "Transfer: too many transfers"
+proc process_transfer*(
+       state: var BeaconState,
+       transfer: Transfer,
+       stateCache: var StateCache,
+       flags: UpdateFlags): bool =
+
+  # Not in spec
+  if transfer.sender.int >= state.balances.len:
+    notice "Transfer: invalid sender ID"
     return false
 
-  for transfer in blck.body.transfers:
-    let sender_balance = state.balances[transfer.sender.int]
+  # Not in spec
+  if transfer.recipient.int >= state.balances.len:
+    notice "Transfer: invalid recipient ID"
+    return false
 
-    ## Verify the amount and fee are not individually too big (for anti-overflow
-    ## purposes)
-    if not (sender_balance >= max(transfer.amount, transfer.fee)):
-      notice "Transfer: sender balance too low for transfer amount or fee"
-      return false
+  let sender_balance = state.balances[transfer.sender.int]
 
-    # A transfer is valid in only one slot
-    if not (state.slot == transfer.slot):
-      notice "Transfer: slot mismatch"
-      return false
+  ## Verify the balance the covers amount and fee (with overflow protection)
+  if sender_balance < max(transfer.amount + transfer.fee, max(transfer.amount, transfer.fee)):
+    notice "Transfer: sender balance too low for transfer amount or fee"
+    return false
 
-    ## Sender must be not yet eligible for activation, withdrawn, or transfer
-    ## balance over MAX_EFFECTIVE_BALANCE
-    if not (
-      state.validators[transfer.sender.int].activation_epoch ==
-        FAR_FUTURE_EPOCH or
-      get_current_epoch(state) >=
-        state.validators[
-          transfer.sender.int].withdrawable_epoch or
-      transfer.amount + transfer.fee + MAX_EFFECTIVE_BALANCE <=
-        state.balances[transfer.sender.int]):
-      notice "Transfer: only withdrawn or not-activated accounts with sufficient balance can transfer"
-      return false
+  # A transfer is valid in only one slot
+  if state.slot != transfer.slot:
+    notice "Transfer: slot mismatch"
+    return false
 
-    # Verify that the pubkey is valid
-    let wc = state.validators[transfer.sender.int].
-      withdrawal_credentials
-    if not (wc.data[0] == BLS_WITHDRAWAL_PREFIX and
-            wc.data[1..^1] == eth2hash(transfer.pubkey.getBytes).data[1..^1]):
-      notice "Transfer: incorrect withdrawal credentials"
-      return false
+  ## Sender must statisfy at least one of the following:
+  if not (
+      # 1) Never have been eligible for activation
+      state.validators[transfer.sender.int].activation_eligibility_epoch == FAR_FUTURE_EPOCH or
+      # 2) Be withdrawable
+      get_current_epoch(state) >= state.validators[transfer.sender.int].withdrawable_epoch or
+      # 3) Have a balance of at least MAX_EFFECTIVE_BALANCE after the transfer
+      state.balances[transfer.sender.int] >= transfer.amount + transfer.fee + MAX_EFFECTIVE_BALANCE
+    ):
+    notice "Transfer: only senders who either 1) have never been eligible for activation or 2) are withdrawable or 3) have a balance of MAX_EFFECTIVE_BALANCE after the transfer are valid."
+    return false
 
-    # Verify that the signature is valid
-    if skipValidation notin flags:
-      if not bls_verify(
-          transfer.pubkey, signing_root(transfer).data, transfer.signature,
-          get_domain(state, DOMAIN_TRANSFER)):
-        notice "Transfer: incorrect signature"
-        return false
+  # Verify that the pubkey is valid
+  let wc = state.validators[transfer.sender.int].withdrawal_credentials
+  if not (wc.data[0] == BLS_WITHDRAWAL_PREFIX and
+          wc.data[1..^1] == eth2hash(transfer.pubkey.getBytes).data[1..^1]):
+    notice "Transfer: incorrect withdrawal credentials"
+    return false
 
-    # Process the transfer
-    decrease_balance(
-      state, transfer.sender.ValidatorIndex, transfer.amount + transfer.fee)
-    increase_balance(
-      state, transfer.recipient.ValidatorIndex, transfer.amount)
-    increase_balance(
-      state, get_beacon_proposer_index(state, stateCache), transfer.fee)
-
-    # Verify balances are not dust
-    if not (
-        0'u64 < state.balances[transfer.sender.int] and
-        state.balances[transfer.sender.int] < MIN_DEPOSIT_AMOUNT):
-      notice "Transfer: sender balance too low for transfer amount or fee"
+  # Verify that the signature is valid
+  if skipValidation notin flags:
+    if not bls_verify(
+        transfer.pubkey, signing_root(transfer).data, transfer.signature,
+        get_domain(state, DOMAIN_TRANSFER)):
+      notice "Transfer: incorrect signature"
       return false
 
-    if not (
-        0'u64 < state.balances[transfer.recipient.int] and
-        state.balances[transfer.recipient.int] < MIN_DEPOSIT_AMOUNT):
-      notice "Transfer: sender balance too low for transfer amount or fee"
-      return false
+  # Process the transfer
+  decrease_balance(
+    state, transfer.sender.ValidatorIndex, transfer.amount + transfer.fee)
+  increase_balance(
+    state, transfer.recipient.ValidatorIndex, transfer.amount)
+  increase_balance(
+    state, get_beacon_proposer_index(state, stateCache), transfer.fee)
+
+  # Verify balances are not dust
+  # TODO: is the spec assuming here that balances are signed integers
+  if (
+      0'u64 < state.balances[transfer.sender.int] and
+      state.balances[transfer.sender.int] < MIN_DEPOSIT_AMOUNT):
+    notice "Transfer: sender balance too low for transfer amount or fee"
+    return false
+
+  if (
+      0'u64 < state.balances[transfer.recipient.int] and
+      state.balances[transfer.recipient.int] < MIN_DEPOSIT_AMOUNT):
+    notice "Transfer: recipient balance too low for transfer amount or fee"
+    return false
 
   true
 
+proc processTransfers(state: var BeaconState, blck: BeaconBlock,
+                      stateCache: var StateCache,
+                      flags: UpdateFlags): bool =
+  if not (len(blck.body.transfers) <= MAX_TRANSFERS):
+    notice "Transfer: too many transfers"
+    return false
+
+  for transfer in blck.body.transfers:
+    if not process_transfer(state, transfer, stateCache, flags):
+      return false
+
+  return true
 
 proc processBlock*(
     state: var BeaconState, blck: BeaconBlock, flags: UpdateFlags,
@@ -529,7 +548,7 @@ proc processBlock*(
     debug "[Block processing - Voluntary Exit] Exit processing failure", slot = shortLog(state.slot)
     return false
 
-  if not processTransfers(state, blck, flags, stateCache):
+  if not processTransfers(state, blck, stateCache, flags):
     debug "[Block processing] Transfer processing failure", slot = shortLog(state.slot)
     return false
 

tests/official/all_fixtures_require_ssz.nim

@@ -17,4 +17,6 @@ import
   ./test_fixture_operations_attester_slashings,
   ./test_fixture_operations_block_header,
   ./test_fixture_operations_proposer_slashings,
-  ./test_fixture_operations_voluntary_exit
+  ./test_fixture_operations_transfer,
+  ./test_fixture_operations_voluntary_exit
+

tests/official/test_fixture_operations_transfer.nim

@@ -0,0 +1,97 @@
+# beacon_chain
+# Copyright (c) 2018-Present Status Research & Development GmbH
+# Licensed and distributed under either of
+#   * MIT license (license terms in the root directory or at http://opensource.org/licenses/MIT).
+#   * Apache v2 license (license terms in the root directory or at http://www.apache.org/licenses/LICENSE-2.0).
+# at your option. This file may not be copied, modified, or distributed except according to those terms.
+
+import
+  # Standard library
+  os, unittest, strutils,
+  # Beacon chain internals
+  ../../beacon_chain/spec/[datatypes, state_transition_block, validator],
+  ../../beacon_chain/[ssz, extras],
+  # Test utilities
+  ../testutil,
+  ./fixtures_utils,
+  ../helpers/debug_state
+
+const OpTransferDir = SszTestsDir/const_preset/"phase0"/"operations"/"transfer"/"pyspec_tests"
+
+template runTest(identifier: untyped) =
+  # We wrap the tests in a proc to avoid running out of globals
+  # in the future: Nim supports up to 3500 globals
+  # but unittest with the macro/templates put everything as globals
+  # https://github.com/nim-lang/Nim/issues/12084#issue-486866402
+
+  const testDir = OpTransferDir / astToStr(identifier)
+
+  proc `testImpl _ transfer _ identifier`() =
+
+    var flags: UpdateFlags
+    var prefix: string
+    if not existsFile(testDir/"meta.yaml"):
+      flags.incl skipValidation
+    if existsFile(testDir/"post.ssz"):
+      prefix = "[Valid]   "
+    else:
+      prefix = "[Invalid] "
+
+    test prefix & astToStr(identifier):
+      var stateRef, postRef: ref BeaconState
+      var transfer: ref Transfer
+      new transfer
+      new stateRef
+
+      transfer[] = parseTest(testDir/"transfer.ssz", SSZ, Transfer)
+      stateRef[] = parseTest(testDir/"pre.ssz", SSZ, BeaconState)
+
+      if existsFile(testDir/"post.ssz"):
+        new postRef
+        postRef[] = parseTest(testDir/"post.ssz", SSZ, BeaconState)
+
+      var cache = get_empty_per_epoch_cache()
+
+      if postRef.isNil:
+        let done = process_transfer(stateRef[], transfer[], cache, flags)
+        doAssert done == false, "We didn't expect this invalid transfer to be processed."
+      else:
+        let done = process_transfer(stateRef[], transfer[], cache, flags)
+        doAssert done, "Valid transfer not processed"
+        check: stateRef.hash_tree_root() == postRef.hash_tree_root()
+        reportDiff(stateRef, postRef)
+
+  `testImpl _ transfer _ identifier`()
+
+suite "Official - Operations - Transfers " & preset():
+  # TODO https://github.com/status-im/nim-beacon-chain/issues/435
+  # CI Win64 - "The parameter is incorrect"
+  when not (defined(windows) and sizeof(int) == 8):
+    when const_preset == "minimal":
+      runTest(success_non_activated)
+      runTest(success_withdrawable)
+      runTest(success_active_above_max_effective)
+      runTest(success_active_above_max_effective_fee)
+      runTest(invalid_signature)
+      runTest(active_but_transfer_past_effective_balance)
+      runTest(incorrect_slot)
+      runTest(transfer_clean)
+      runTest(transfer_clean_split_to_fee)
+      runTest(insufficient_balance_for_fee)
+      runTest(insufficient_balance_for_amount_result_full)
+      runTest(insufficient_balance_for_combined_result_dust)
+      runTest(insufficient_balance_for_combined_result_full)
+      runTest(insufficient_balance_for_combined_big_amount)
+      runTest(insufficient_balance_for_combined_big_fee)
+      runTest(insufficient_balance_off_by_1_fee)
+      runTest(insufficient_balance_off_by_1_amount)
+      runTest(insufficient_balance_duplicate_as_fee_and_amount)
+      runTest(no_dust_sender)
+      runTest(no_dust_recipient)
+      runTest(non_existent_sender)
+      runTest(non_existent_recipient)
+      runTest(invalid_pubkey)
+    else:
+      echo "          No transfer tests in mainnet preset"
+  else:
+    echo "          Skipped for Windows 64-bit CI"