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

core/state: changes to journalling, part 1 #28880

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Jan 26, 2024

This is a follow-up to #29520, and a preparatory PR to a more thorough change in the journalling system.

API methods instead of append operations

This PR hides the journal-implementation details away, so that the statedb invokes methods like JournalCreate, instead of explicitly appending journal-events in a list. This means that it's up to the journal whether to implement it as a sequence of events or aggregate/merge events.

Snapshot-management inside the journal

This PR also makes it so that management of valid snapshots is moved inside the journal, exposed via the methods Snapshot() int and RevertToSnapshot(revid int, s *StateDB).

SetCode

JournalSetCode journals the setting of code: it is implicit that the previous values were "no code" and emptyCodeHash. Therefore, we can simplify the setCode journal.

Selfdestruct

The self-destruct journalling is a bit strange: we allow the selfdestruct operation to be journalled several times. This makes it so that we also are forced to store whether the account was already destructed.

What we can do instead, is to only journal the first destruction, and after that only journal balance-changes, but not journal the selfdestruct itself.

This simplifies the journalling, so that internals about state management does not leak into the journal-API.

Preimages

Preimages were, for some reason, integrated into the journal management, despite not being a consensus-critical data structure. This PR undoes that.

@holiman
Copy link
Contributor Author

holiman commented Jan 29, 2024

(content moved to PR description)

@holiman
Copy link
Contributor Author

holiman commented Jan 29, 2024

Where these changes are leading to is the following:

type scopedJournal struct {
	accountChanges map[common.Address][]byte
	refund         int64
	logs           []types.Log
        .... 
}

The accountChanges are a representation of an account (probably rlp-data for types.SlimAccount). It is the entry-type which replaces individual balanceChange, nonceChange, creationChange, touchChange and selfdestructChange. We only ever need it once, per scope. Example:

1. add 1 to X
2. add 2 to X
3. set nonce=500 for X
4. X is selfdestructed
5. x is set to zero balance
6. revert

Instead of collecting this as 6 separate entries, it's sufficient if the very first add 1 to X stores the pre-state of X. After that, we can omit expanding the journal for this scope.

Therefore, it is good if we can remove implementation-flags such as destructed from entering the journal. And it's the same with code, if we can prevent code from entering into the journal, that change too can be encompassed by this system.

Example of how it can look:

// journalAccountChange is the common shared implementation for all account-changes.
// These changes all fall back to this method:
// - balance change
// - nonce change
// - creation change (in this case, the account is nil)
func (j *scopedJournal) journaAccountChange(address common.Address, account types.StateAccount) {
	// Unless the account has already been journalled, journal it now
	if _, ok := accountChanges[address]; !ok {
		if account != nil {
			accountChanges[address] = types.SlimAccountRLP(account)
		} else {
			accountChanges[address] = nil
		}
		j.dirties[address]++
	}
}
func (j *scopedJournal) JournalNonceChange(addr common.Address, account types.StateAccount) {
	j.journaAccountChange(addr, account)
}

func (j *scopedJournal) JournalBalanceChange(addr common.Address, account types.StateAccount) {
	j.journaAccountChange(address, account)
}

func (j *scopedJournal) JournalCreate(addr common.Address) {
	j.journaAccountChange(addr, nil)
}

@holiman
Copy link
Contributor Author

holiman commented Jan 30, 2024

The effect of the changes -- not on this branch, but no the even more experimental journal_changes_pt2
The burntpix benchmark (the execution time differs pretty wildly between 7-11 seconds on both versions).

With master:

EVM gas used:    1194877856
execution time:  8.303515894s
allocations:     497_562
allocated bytes: 107_153_152

With set-based journal:

EVM gas used:    1194877856
execution time:  9.63126912s
allocations:     30_046
allocated bytes: 28_804_928

@holiman holiman changed the title Journal changes core/state: changes to journalling, part 1 Apr 18, 2024
@holiman holiman force-pushed the journal_changes branch 4 times, most recently from 2290286 to 5eeeabd Compare April 24, 2024 11:40
@holiman holiman marked this pull request as ready for review April 24, 2024 11:43
@holiman
Copy link
Contributor Author

holiman commented Apr 25, 2024

Now rebased to follow on top of #29627

@holiman holiman force-pushed the journal_changes branch 2 times, most recently from c802f78 to 442fcf4 Compare April 26, 2024 12:26
The self-destruct journalling is a bit strange: we allow the
'selfdestruct' operation to be journalled several times. This makes it
so that we also are forced to store whether the account was
already destructed.
What we can do instead, is to only journal the first destruction, and after
that only journal balance-changes, but not journal the selfdestruct itself.

This simplifies the journalling, so that internals about state management
does not leak into the journal-API.
We do not strictly need to journal preimages: preimages are stored and
served for the benefit of remote RPC users, e.g. for debugging solidity
storage. There is no 'consensus requirement' for these keys to be
reverted in case of reverted scopes.
func (j *journal) Reset() {
j.entries = j.entries[:0]
j.validRevisions = j.validRevisions[:0]
j.dirties = make(map[common.Address]int)
Copy link
Member

Choose a reason for hiding this comment

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

Might consider clear(j.dirties)?

Copy link
Member

Choose a reason for hiding this comment

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

Supposedly it empties a map's contents without deallocating it's capacity.

prevvalue: prevvalue,
})
s.db.journal.JournalSetState(s.address, key, prevvalue)
s.setState(key, &value)
Copy link
Member

Choose a reason for hiding this comment

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

The other methods called the logger first and then did the actual set. This PR moves this setState above it - but not in the other log events. I'm assuming it's a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logger invocation is moved down, yes, because it's not "core". I wanted it to not be mixed in with the actual consensus code.

Since the values are already loaded into variables, it should be fine to do so.

Copy link
Member

Choose a reason for hiding this comment

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

But every single other call has the logger happening first and then the state change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because consider this one:

    if s.db.logger != nil && s.db.logger.OnBalanceChange != nil {
        s.db.logger.OnBalanceChange(s.address, s.Balance().ToBig(), amount.ToBig(), reason)
   }

In the call to the logger, it actually uses the s.Balance(). So it needs to be done before setting the new value. Alternatively, stash the value in a variable, so the logger-call later on can use that.

So we're left with two crappy choices: introduce a (possibly unused) tempvar, or or do the logging-call in the middle of the core consensus mechanics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants