EIP-213 (bn128 curve operations) #4999
Conversation
NikVolf
added
A0-pleasereview 🤓
ethcore/src/builtin.rs
Outdated
| pub fn execute(&self, input: &[u8], output: &mut BytesRef) { self.native.execute(input, output) } | ||
| pub fn execute(&self, input: &[u8], output: &mut BytesRef) -> Result<(), Error> { | ||
| self.native.execute(input, output)?; | ||
| Ok(()) |
There was a problem hiding this comment.
body can be unchanged: self.native.execute()
ethcore/src/builtin.rs
Outdated
| } | ||
|
|
||
| impl Impl for Bn128AddImpl { | ||
| fn execute(&self, input: &[u8], output: &mut BytesRef) -> Result<(), Error> { |
There was a problem hiding this comment.
Given that these are fallible, it makes sense to document the failure conditions.
ethcore/Cargo.toml
Outdated
| @@ -45,6 +45,7 @@ ethcore-bloom-journal = { path = "../util/bloom" } | |||
| hardware-wallet = { path = "../hw" } | |||
| stats = { path = "../util/stats" } | |||
| num = "0.1" | |||
| bn = { git = "https:/paritytech/bn" } | |||
There was a problem hiding this comment.
any plans to upstream these changes?
There was a problem hiding this comment.
yep after i'm done with pairing
|
We should not really use "alt_" prefix anywhere. As far as I can tell this is only used in libsnark to distinguish two implementations of the same curve apart. |
|
@arkpar done |
NikVolf
changed the title
arkpar
added
A8-looksgood 🦄
| @@ -70,6 +73,12 @@ impl From<Box<trie::TrieError>> for Error { | |||
| } | |||
| } | |||
|
|
|||
| impl From<builtin::Error> for Error { | |||
| fn from(err: builtin::Error) -> Self { | |||
| Error::BuiltIn(err.0) | |||
There was a problem hiding this comment.
Why not just have builtins return a String as error rather than go through the trouble of making a builtin::Error type which is just destructured here? Not sure it's worth allocating the string at all, actually, since
- it's easy to make builtins fail
- builtins are on a hot path
- errors are silently swallowed anyway
gavofyork
added
A5-grumble 🔥
NikVolf
added
A0-pleasereview 🤓
ethcore/src/executive.rs
Outdated
| @@ -276,7 +276,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { | |||
|
|
|||
| let cost = builtin.cost(data); | |||
| if cost <= params.gas { | |||
| builtin.execute(data, &mut output); | |||
| builtin.execute(data, &mut output)?; | |||
There was a problem hiding this comment.
This can fail without discarding or reverting_to_checkpoint.
Currently we don't have any state-altering builtins, but might be good to have that in mind. Also I'm not entirely sure if one leaking state snapshot would ever be a problem, but IMHO would be good to address that.
There was a problem hiding this comment.
not sure i follow
why shouldn't it fail the same way as not enough gas fails, but with different reason?
There was a problem hiding this comment.
I think every other exit from this function is either doing: state.discard_snapshot(), state.revert_to_snapshot() or enact_result (which does this stuff internally afair).
This could exit the function after creating state.checkpoint() but without reseting it. Also seems that tracing for top-level failed builtin call might be missing.
There was a problem hiding this comment.
You are right, I misjudged the program flow here
the errors and rollback/discard logic should be decoupled from each other though
| if let Err(e) = builtin.execute(data, &mut output) { | ||
| self.state.revert_to_checkpoint(); | ||
| let evm_err: evm::evm::Error = e.into(); | ||
| tracer.trace_failed_call(trace_info, vec![], evm_err.clone().into()); |
There was a problem hiding this comment.
One last tiny grumble: We're tracing only calls on `self.depth == 0 to avoid DoS attacks for tracing nodes, probably worth doing the same check here.
There was a problem hiding this comment.
But out-of-gas is tracing error on any self.depth below?
Which seems right, because you never know what caused contract to fail otherwise
There was a problem hiding this comment.
Fair enough, probably the problematic part is trace_output anyway.
arkpar
added
A8-looksgood 🦄
bnlib