Skip to content

feat: Include genesis accounts in changesets#2606

Merged
gakonst merged 1 commit intoparadigmxyz:mainfrom
petertdavies:genesis-changesets
May 9, 2023
Merged

feat: Include genesis accounts in changesets#2606
gakonst merged 1 commit intoparadigmxyz:mainfrom
petertdavies:genesis-changesets

Conversation

@petertdavies
Copy link
Contributor

This PR changes the way genesis accounts/storage are handled. They are now treated as if they are changes that occur during the genesis block. This creates an invariant that an account that has never been written to is non-existent and storage key that has never been written is zero, which enables some lookup optimizations.

The GenesisAccount type unhelpfully distinguishes between accounts with no bytecode and accounts with empty bytecode. The previous code wrote an account with bytecode: None in the first case and bytecode: Some(<Hash of empty bytes>) in the second. I have preserved this dubious behaviour.

@petertdavies petertdavies requested a review from gakonst as a code owner May 8, 2023 19:28
@codecov-commenter
Copy link

codecov-commenter commented May 8, 2023

Codecov Report

Merging #2606 (c520718) into main (443572a) will decrease coverage by 11.65%.
The diff coverage is 46.66%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##             main    #2606       +/-   ##
===========================================
- Coverage   71.72%   60.08%   -11.65%     
===========================================
  Files         489      496        +7     
  Lines       61424    62559     +1135     
===========================================
- Hits        44059    37587     -6472     
- Misses      17365    24972     +7607     
Flag Coverage Δ
integration-tests 17.89% <0.00%> (-0.22%) ⬇️
unit-tests 54.19% <46.66%> (-12.40%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
crates/staged-sync/src/utils/init.rs 80.15% <46.66%> (-8.13%) ⬇️

... and 217 files with indirect coverage changes

@mattsse mattsse requested review from Rjected and rkrasiuk May 8, 2023 19:52
@mattsse mattsse added the A-db Related to the database label May 8, 2023
Copy link
Collaborator

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

lgtm

bytecode_cursor.upsert(hash, Bytecode::new_raw_with_hash(code.0.clone(), hash))?;
bytecode_hash = Some(hash);
let bytecode = Bytecode::new_raw(code.0.clone());
// FIXME: Can bytecode_hash be Some(Bytes::new()) here?
Copy link
Collaborator

Choose a reason for hiding this comment

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

no, if they have bytecode the hash must match since that is how we load bytecode from the database. otherwise the account would have no bytecode associated with it

Suggested change
// FIXME: Can bytecode_hash be Some(Bytes::new()) here?

Copy link
Contributor

@rkrasiuk rkrasiuk left a comment

Choose a reason for hiding this comment

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

lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-db Related to the database

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants