Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Sep 11, 2019

Conversation

mratsim
Copy link
Contributor

@mratsim mratsim commented Sep 9, 2019

No description provided.

@mratsim mratsim requested a review from tersec September 9, 2019 21:31
@mratsim
Copy link
Contributor Author

mratsim commented Sep 10, 2019

Same failures on Windows x64 as #408, it works on 32-bit

https://ci.appveyor.com/project/nimbus/nim-beacon-chain/builds/27293393/job/0b600abqo06snpgi#L3130

[Suite] Official - Operations - Transfers  [Preset: minimal]
  [OK] [Valid]   success_non_activated
  [OK] [Valid]   success_withdrawable
  [OK] [Valid]   success_active_above_max_effective
  [OK] [Valid]   success_active_above_max_effective_fee
NOT 2019-09-09 23:14:47+00:00 Transfer: incorrect signature              tid=2716
  [OK] [Invalid] invalid_signature
NOT 2019-09-09 23:14:47+00:00 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. tid=2716
  [OK] [Invalid] active_but_transfer_past_effective_balance
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] incorrect_slot
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Valid]   transfer_clean
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Valid]   transfer_clean_split_to_fee
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] insufficient_balance_for_fee
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] insufficient_balance_for_amount_result_full
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] insufficient_balance_for_combined_result_dust
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] insufficient_balance_for_combined_result_full
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] insufficient_balance_for_combined_big_amount
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] insufficient_balance_for_combined_big_fee
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] insufficient_balance_off_by_1_fee
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] insufficient_balance_off_by_1_amount
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] insufficient_balance_duplicate_as_fee_and_amount
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] no_dust_sender
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] no_dust_recipient
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] non_existent_sender
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] non_existent_recipient
    Unhandled exception: The parameter is incorrect.
 [OSError]
  [FAILED] [Invalid] invalid_pubkey

Copy link
Contributor

@tersec tersec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the CI failure is known to be spurious, it seems better not to break that in nim-beacon-chain master to avoid unrelated regressions.

Might it be preferable to enable only some subset of tests which might be be compatible with this CI failure with some kind of marker to return later?

Because the commit itself seems basically fine, otherwise.

@mratsim
Copy link
Contributor Author

mratsim commented Sep 10, 2019

Yes I don't want to merge yet, I'll debug it on my main computer.

@tersec
Copy link
Contributor

tersec commented Sep 11, 2019

I'd be happy to merge this with something akin to #438 included either as part of the initial merge or after the fact.

@mratsim
Copy link
Contributor Author

mratsim commented Sep 11, 2019

let's merge this.

@tersec
Copy link
Contributor

tersec commented Sep 11, 2019

Ah, I see it's only failing because proposer slashing's now breaking for the same reason. LGTM.

@tersec tersec merged commit 3dc2b87 into master Sep 11, 2019
@delete-merged-branch delete-merged-branch bot deleted the transfer branch September 11, 2019 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants