AcalaNetwork / Acala

Acala - cross-chain DeFi hub and stablecoin based on Substrate for Polkadot and Kusama.
https://acala.network
GNU General Public License v3.0
741 stars 456 forks source link

Impl block prevrandao #2736

Closed zjb0807 closed 4 months ago

zjb0807 commented 4 months ago

Closes: #2693

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 64.89%. Comparing base (1bead11) to head (2967694). Report is 1 commits behind head on master.

Files Patch % Lines
modules/evm/src/lib.rs 0.00% 3 Missing :warning:
modules/support/src/mocks.rs 0.00% 3 Missing :warning:
modules/evm/src/runner/stack.rs 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2736 +/- ## ========================================== - Coverage 65.35% 64.89% -0.47% ========================================== Files 69 66 -3 Lines 9797 8563 -1234 ========================================== - Hits 6403 5557 -846 + Misses 3394 3006 -388 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ermalkaleci commented 4 months ago

I have 2 concerns with this:

  1. Is this going to work for tx executive on_initialize?
  2. This is a read and doing hashing every time is called. Any risk this can be abused?
xlc commented 4 months ago
  1. Is this going to work for tx executive on_initialize?

An insecure random value that's based on parent block hash will be provided. But there are only few limited ways to trigger EVM call in on_initialize including scheduler and XCM. XCM processing will be moved out from on_initialize in future and scheduler usage is governance protected so it should be fine.

  1. This is a read and doing hashing every time is called. Any risk this can be abused?

I guess we can cache the value? Otherwise we need to ensure we charge enough gas.

ermalkaleci commented 4 months ago
  1. Is this going to work for tx executive on_initialize?

An insecure random value that's based on parent block hash will be provided. But there are only few limited ways to trigger EVM call in on_initialize including scheduler and XCM. XCM processing will be moved out from on_initialize in future and scheduler usage is governance protected so it should be fine.

  1. This is a read and doing hashing every time is called. Any risk this can be abused?

I guess we can cache the value? Otherwise we need to ensure we charge enough gas.

I think we need to be careful with this, at least enable only on mandala & karura first

xlc commented 4 months ago

It is known prevrandao have its limitation and cannot be used solely for cases requires strong random number, extra randomness source are required so I am not too worried about abuse.