EIP-1702: Generalized Account Versioning Scheme #10771

sorpaas · 2019-06-21T22:16:51Z

This is a draft implementation of EIP-1702.

Snapshots and light clients should be backward compatible. Older clients stops working once the network has its first non-legacy version account.
The actual versioning is defined by vm::Schedule::versions, where we supports PWasm (which uses the pWasm execution engine) only (because it's the only useful target right now).
For future EVM real usage, we'll define feature flags in VersionedSchedule (for example, VersionedSchedule::EVM::eip1283) that states a particular feature should be enabled in the current version.
For frontend config, one can specify wasm_version together with wasm_activation_transition, so that pWasm is activated using an account version, instead of code prefix.

dvdplm · 2019-06-23T08:20:29Z

* `pWasm` currently always create new contracts of version zero. This is a known issue.

Can you elaborate on this for my education?

sorpaas · 2019-06-23T15:04:52Z

@dvdplm This is fixed in 4e448f1. Let me know if you still need more info.

And for spec-related questions, maybe consider putting it under sorpaas/EIPs#2!

grbIzl · 2019-06-24T09:48:58Z

ethcore/src/snapshot/account.rs

@@ -84,10 +85,14 @@ pub fn to_fat_rlps(
 	let mut leftover: Option<Vec<u8>> = None;
 	loop {
 		account_stream.append(account_hash);
-		account_stream.begin_list(5);


Is it possible to make one general begin_unbounded_list for all added rlp parameters? It looks weird, when it's first limited 5-6 elements list and unlimited after it.

I'm trying to maintain backward compatibility here. If we change the snapshotting format when use_short_version is true, then old clients will break. Or do you have better ideas?

The only idea, that I've got, that doesn't break compatibility and doesn't increment the whole packet's length, is to pass the list instead of one of the parameters (for example, push (nonce, code_version) instead of nonce). And to define during decoding, is it list on this place in the packet or not. It will allow to get rid off the most of boolean flags in these two methods but it will be still "hacky" (not obvious). I mean something like this:
Encoding:
....
account_stream.begin_list(5);
account_stream.begin_list(2);
account_stream.append(&acc.nonce);
account_stream.append(&acc.code_version);
account_stream.append(&acc.balance);
...
Decoding:
let account_descr = rlp.val_at(0)?;
let mut nonce = 0;
let mut code_version = U256::zero();
if account_descr.is_list() {
nonce = account_descr.val_at(0)?;
code_version = account_descr.val_at(1)?;
} else {
nonce = account_descr.as_val()?;
}
What do you think?

I still don't see what's the advantage of this over just checking the RLP list length. That should be reliable and we're already using it in places like transaction decoding: https://github.com/paritytech/parity-ethereum/blob/825c7990f29514d699e22c6e78f73710e53e983d/ethcore/types/src/transaction/transaction.rs#L121-L134

grbIzl · 2019-06-24T09:49:34Z

ethcore/src/snapshot/account.rs

+		if !use_short_version {
+			account_stream.append(&acc.code_version);
+		}
+
 		account_stream.begin_unbounded_list();


Unbounded list starts here

grbIzl · 2019-06-24T09:53:49Z

ethcore/src/snapshot/account.rs

@@ -170,6 +179,12 @@ pub fn from_fat_rlp(
 		return Ok((ACC_EMPTY, None));
 	}

+	let use_short_version = match rlp.item_count()? {
+		5 => true,


I think, that it's a risky logic, to rely on item count in rlp packet. May be to pass this parameter explicitly?

I don't get what you mean here. Which parameter do you think we can pass explicitly?

grbIzl · 2019-06-24T09:57:12Z

ethcore/types/src/basic_account.rs

+	fn rlp_append(&self, stream: &mut rlp::RlpStream) {
+		let use_short_version = self.code_version == U256::zero();
+
+		match use_short_version {


Please also use begin_unbounded_list here.

grbIzl · 2019-06-24T09:57:48Z

ethcore/types/src/basic_account.rs

+
+impl rlp::Decodable for BasicAccount {
+	fn decode(rlp: &rlp::Rlp) -> Result<Self, rlp::DecoderError> {
+		let use_short_version = match rlp.item_count()? {


The same comment about fragile logic based on item count

Using rlp.item_count() is the only method to know whether we're at version 0 or above version 0. I don't agree it's fragile -- BasicAccount is guaranteed to be either have 4 items (version 0 case) or 5 items (version non-0 case). And we handle all errors explicitly.

…p-eip1702

ethcore/src/snapshot/account.rs

ethcore/src/factory.rs

ordian · 2019-07-04T13:37:09Z

ethcore/src/executive.rs

+							Err(err) => Err(err),
+						}
+					},
+					None => Ok(Err(vm::Error::OutOfGas)),


I'm not sure this is the correct error, is it?

This is the correct error -- None means the validation phrase fails. In that case, according to the spec we should return out of gas.

Co-Authored-By: Andronik Ordian <write@reusable.software>

grbIzl · 2019-07-05T09:18:46Z

ethcore/src/snapshot/account.rs

@@ -246,6 +268,7 @@ mod tests {
 			balance: 123456789.into(),
 			storage_root: KECCAK_NULL_RLP,
 			code_hash: KECCAK_EMPTY,
+			code_version: 0.into(),


Can you please add one more test for encoding accounts with not zero code-version?

…p-eip1702

…to sp-eip1702

sorpaas · 2019-07-08T00:22:50Z

I have added a frontend config wasmVersion (used together with wasmActivationTransition it enables pwasm via account versioning, rather than code prefix). I think the PR should be ready for review.

For testing, it's still not yet done and I think we may want to have a separate PR to do it comprehensively. (And collaborate with aleth, if possible.)

grbIzl

Looks good!

* master: EIP-1702: Generalized Account Versioning Scheme (#10771)

…-state-db * dp/chore/extricate-state-backend: Sort out the merge EIP-1702: Generalized Account Versioning Scheme (#10771)

…cation * dp/chore/extract-state-db: Sort out the merge EIP-1702: Generalized Account Versioning Scheme (#10771)

EIP-1702: Generalized Account Versioning Scheme

04b701e

sorpaas added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 21, 2019

Fix pWASM's create and create2 contract creation version

4e448f1

grbIzl reviewed Jun 24, 2019

View reviewed changes

sorpaas added 2 commits June 27, 2019 16:25

Merge branch 'master' of github.com:paritytech/parity-ethereum into s…

c1f4f59

…p-eip1702

Merge branch 'master' of github.com:paritytech/parity-ethereum into s…

2d3b2f8

…p-eip1702

ordian reviewed Jul 4, 2019

View reviewed changes

sorpaas and others added 2 commits July 4, 2019 20:14

Update ethcore/src/snapshot/account.rs

70c0889

Co-Authored-By: Andronik Ordian <write@reusable.software>

Update ethcore/src/factory.rs

c3b6ccd

Co-Authored-By: Andronik Ordian <write@reusable.software>

grbIzl reviewed Jul 5, 2019

View reviewed changes

ordian added this to the 2.7 milestone Jul 5, 2019

sorpaas added 5 commits July 8, 2019 01:45

Add pWasm frontend config

d693704

Add snapshot testing with version

b84430a

Merge branch 'master' of github.com:paritytech/parity-ethereum into s…

b03c805

…p-eip1702

Fix merge conflict

46a1ac9

Merge branch 'sp-eip1702' of github.com:paritytech/parity-ethereum in…

3b27df0

…to sp-eip1702

grbIzl approved these changes Jul 8, 2019

View reviewed changes

ordian approved these changes Jul 8, 2019

View reviewed changes

ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 8, 2019

dvdplm merged commit 141f6a0 into master Jul 8, 2019

dvdplm deleted the sp-eip1702 branch July 8, 2019 10:03

dvdplm added a commit that referenced this pull request Jul 8, 2019

Merge branch 'master' into dp/chore/extricate-state-backend

75efc47

* master: EIP-1702: Generalized Account Versioning Scheme (#10771)

dvdplm added a commit that referenced this pull request Jul 8, 2019

Merge branch 'dp/chore/extricate-state-backend' into dp/chore/extract…

cd9167e

…-state-db * dp/chore/extricate-state-backend: Sort out the merge EIP-1702: Generalized Account Versioning Scheme (#10771)

dvdplm added a commit that referenced this pull request Jul 8, 2019

Merge branch 'dp/chore/extract-state-db' into dp/chore/extract-verifi…

f30f63b

…cation * dp/chore/extract-state-db: Sort out the merge EIP-1702: Generalized Account Versioning Scheme (#10771)

niklasad1 mentioned this pull request Mar 11, 2020

[TRACKER] Berlin network upgrade #11562

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-1702: Generalized Account Versioning Scheme #10771

EIP-1702: Generalized Account Versioning Scheme #10771

sorpaas commented Jun 21, 2019 •

edited

Loading

dvdplm commented Jun 23, 2019

sorpaas commented Jun 23, 2019

grbIzl Jun 24, 2019

sorpaas Jun 24, 2019

grbIzl Jun 25, 2019

sorpaas Jun 25, 2019

grbIzl Jun 24, 2019

grbIzl Jun 24, 2019

sorpaas Jun 24, 2019

grbIzl Jun 24, 2019

grbIzl Jun 24, 2019

sorpaas Jun 24, 2019

ordian Jul 4, 2019

sorpaas Jul 4, 2019

grbIzl Jul 5, 2019

sorpaas commented Jul 8, 2019

grbIzl left a comment

EIP-1702: Generalized Account Versioning Scheme #10771

EIP-1702: Generalized Account Versioning Scheme #10771

Conversation

sorpaas commented Jun 21, 2019 • edited Loading

dvdplm commented Jun 23, 2019

sorpaas commented Jun 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorpaas commented Jul 8, 2019

grbIzl left a comment

Choose a reason for hiding this comment

sorpaas commented Jun 21, 2019 •

edited

Loading