From 3dc2b87e6a78019b7609257d5d4e1cdeca7cd438 Mon Sep 17 00:00:00 2001 From: Mamy Ratsimbazafy Date: Wed, 11 Sep 2019 16:29:00 -0400 Subject: [PATCH] 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 | 139 ++++++++++-------- tests/official/all_fixtures_require_ssz.nim | 4 +- .../test_fixture_operations_transfer.nim | 97 ++++++++++++ 3 files changed, 179 insertions(+), 61 deletions(-) create mode 100644 tests/official/test_fixture_operations_transfer.nim diff --git a/beacon_chain/spec/state_transition_block.nim b/beacon_chain/spec/state_transition_block.nim index 9e144a93ee..2109030cfb 100644 --- a/beacon_chain/spec/state_transition_block.nim +++ b/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 diff --git a/tests/official/all_fixtures_require_ssz.nim b/tests/official/all_fixtures_require_ssz.nim index 3ff23d060c..c9963d2fea 100644 --- a/tests/official/all_fixtures_require_ssz.nim +++ b/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 \ No newline at end of file + ./test_fixture_operations_transfer, + ./test_fixture_operations_voluntary_exit + \ No newline at end of file diff --git a/tests/official/test_fixture_operations_transfer.nim b/tests/official/test_fixture_operations_transfer.nim new file mode 100644 index 0000000000..8dec2b2934 --- /dev/null +++ b/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"