Change handling of dirty objects in state #15225
Conversation
|
As I wrote above:
Turned out that was indeed the case, it blew up on Both were executed with default settings for
I've changed the logic a bit, and will try a full import again. |
|
With the latest changes, I've successfully imported blocks With the master-code: With this PR:
|
|
Some more experiments. With
|
|
With this PR, import block 3M to 4M, |
|
Import made it to |
|
I think this method maybe needs to be amended, since |
core/state/statedb.go
Outdated
| @@ -450,20 +450,22 @@ func (self *StateDB) Copy() *StateDB { | |||
| self.lock.Lock() | |||
| defer self.lock.Unlock() | |||
|
|
|||
| dirtyObjects := self.journal.flatten() | |||
There was a problem hiding this comment.
This seems dangerous to me because it iterates over all the previous opcodes. So we have an O(opcodes * calls) complexity here
There was a problem hiding this comment.
Not 'opcodes' technically, just the ones that modify the state in some manner. Why would it be O( n*m) ?
Afict it's O( n ) , n being the length of the journal.
There was a problem hiding this comment.
But yes, I'm not 100% certain about how to view Copy, and what the semantics of that should be. The copy should (?) have a clean and fresh journal, but needs a map of uncommitted changes.
|
Didn't have time to look into this yet. Will take a stab next week. |
karalabe
force-pushed
the
test_removefrom_dirtyset
branch
from
5297257 to
44fc96b
Compare
core/state/statedb.go
Outdated
| @@ -585,6 +591,12 @@ func (s *StateDB) clearJournalAndRefund() { | |||
| func (s *StateDB) Commit(deleteEmptyObjects bool) (root common.Hash, err error) { | |||
| defer s.clearJournalAndRefund() | |||
|
|
|||
| dirtyObjects := s.journal.flatten() | |||
|
|
|||
| for addr, v := range dirtyObjects { | |||
There was a problem hiding this comment.
Nitpick, you can replace dirtyObjects with s.journal.flatten(). Less code, cleaner.
core/state/statedb.go
Outdated
|
|
||
| dirtyObjects := s.journal.flatten() | ||
|
|
||
| for addr, v := range dirtyObjects { |
There was a problem hiding this comment.
Nitpick, you can replace dirtyObjects with s.journal.flatten(). Less code, cleaner.
core/state/statedb.go
Outdated
| } | ||
| //func (self *StateDB) MarkStateObjectDirty(addr common.Address) { | ||
| // self.stateObjectsDirty[addr] = struct{}{} | ||
| //} |
core/state/journal.go
Outdated
| } | ||
|
|
||
| type journal []journalEntry | ||
| //type journal []journalEntry |
core/state/journal.go
Outdated
|
|
||
| func (ch addPreimageChange) undo(s *StateDB) { | ||
| delete(s.preimages, ch.hash) | ||
| } | ||
| func (ch addPreimageChange) getAccount() *common.Address { |
There was a problem hiding this comment.
Please leave a space an empty line between all methods in this file.
holiman
force-pushed
the
test_removefrom_dirtyset
branch
from
7c528ad to
c6dfe74
Compare
|
Addressed the concerns |
holiman
force-pushed
the
test_removefrom_dirtyset
branch
from
c6dfe74 to
513a55a
Compare
|
|
|
|
karalabe
force-pushed
the
test_removefrom_dirtyset
branch
from
513a55a to
8500470
Compare
core/state/statedb.go
Outdated
| @@ -488,7 +488,7 @@ func (self *StateDB) Copy() *StateDB { | |||
| func (self *StateDB) Snapshot() int { | |||
| id := self.nextRevisionId | |||
| self.nextRevisionId++ | |||
| self.validRevisions = append(self.validRevisions, revision{id, self.journal.Length()}) | |||
| self.validRevisions = append(self.validRevisions, revision{id, len(self.journal.entries)}) | |||
There was a problem hiding this comment.
Not sure why you removed Length ? Is this really cleaner?
karalabe
force-pushed
the
test_removefrom_dirtyset
branch
from
8500470 to
2a2e368
Compare
karalabe
force-pushed
the
test_removefrom_dirtyset
branch
from
2a2e368 to
d985b90
Compare
|
Testing with these changes now: #git diff
diff --git a/core/state/state_object.go b/core/state/state_object.go
index 3c40c20..fdac884 100644
--- a/core/state/state_object.go
+++ b/core/state/state_object.go
@@ -242,7 +242,7 @@ func (c *stateObject) AddBalance(amount *big.Int) {
// EIP158: We must check emptiness for the objects such that the account
// clearing (0,0,0 objects) can take effect.
if amount.Sign() == 0 {
- if c.empty() {
+ if c.empty() && !c.db.IgnoresTouchedness{
c.touch()
}
diff --git a/core/state/statedb.go b/core/state/statedb.go
index c7ef49f..64b637b 100644
--- a/core/state/statedb.go
+++ b/core/state/statedb.go
@@ -79,7 +79,7 @@ type StateDB struct {
journal *journal
validRevisions []revision
nextRevisionId int
-
+ IgnoresTouchedness bool // transactions prior to EIP 158 does not have a notion about 'touched'
lock sync.Mutex
}
diff --git a/core/state_processor.go b/core/state_processor.go
index 4dc58b9..c1b334a 100644
--- a/core/state_processor.go
+++ b/core/state_processor.go
@@ -65,6 +65,9 @@ func (p *StateProcessor) Process(block *types.Block, statedb *state.StateDB, cfg
if p.config.DAOForkSupport && p.config.DAOForkBlock != nil && p.config.DAOForkBlock.Cmp(block.Number()) == 0 {
misc.ApplyDAOHardFork(statedb)
}
+ if block.Number().Cmp(p.config.EIP158Block) < 0{
+ statedb.IgnoresTouchedness = true
+ }
// Iterate over and process the individual transactions
for i, tx := range block.Transactions() {
statedb.Prepare(tx.Hash(), block.Hash(), i)
|
|
|
|
For context: at block= If we do special treatment of That would make some things simpler; we encode the homestead state transition in on implementation, and never again touch that code in future hardforks. And we have a separate one for eip158, which has the notion of |
|
|
|
@karalabe A 'cleaner' solution than my fix which made state processor rule-aware, is to simply do a nil-check and continue loop if nil. An object can be in the journal but not the stateobjects_dirty, in the specific case of a |
|
I will now try out my cleaner solution, and force-push here if it works out.
|
holiman
force-pushed
the
test_removefrom_dirtyset
branch
from
18247ab to
a819c8f
Compare
|
Pushed new fix |
core/state/statedb.go
Outdated
| if !exist { | ||
| // ripeMD is 'touched' at block 1714175, in tx 0x1237f737031e40bcde4a8b7e717b2d15e3ecadfe49bb1bbc71ee9deb09c6fcf2 | ||
| // That tx goes out of gas, and although the notion of 'touched' does not exist there, the | ||
| // touch-even will still be recorded in the journal. Since ripeMD is a special snowflake, |
core/state/statedb.go
Outdated
| // ripeMD is 'touched' at block 1714175, in tx 0x1237f737031e40bcde4a8b7e717b2d15e3ecadfe49bb1bbc71ee9deb09c6fcf2 | ||
| // That tx goes out of gas, and although the notion of 'touched' does not exist there, the | ||
| // touch-even will still be recorded in the journal. Since ripeMD is a special snowflake, | ||
| // it will persist in the journal even though the journal is reverted. In this special circumstance , |
holiman
force-pushed
the
test_removefrom_dirtyset
branch
from
a819c8f to
8c31d28
Compare
|
@holiman @karalabe @fjl I observed a weird difference in behavior that seem to be due to this PR. I wrote a small repro: db, _ := ethdb.NewMemDatabase()
sdb1, _ := state.New(common.Hash{}, state.NewDatabase(db))
addr := common.HexToAddress("aaaa")
sdb1.SetBalance(addr, big.NewInt(42))
sdb2 := sdb1.Copy()
fmt.Printf("Balance before: %v\n", sdb2.GetBalance(addr))
sdb3 := sdb2.Copy()
fmt.Printf("Balance after: %v\n", sdb3.GetBalance(addr))
// Output with PR #15225:
// Balance before: 42
// Balance after: 0
// Output without PR #15225:
// Balance before: 42
// Balance after: 42I would expect it to print 42 twice since copy should create a deep copy. Is this not the desired behavior anymore? |
|
Nice catch, thank you! |
|
I've made a PR which fixes this, I hope nothing else is br0ken... #16485 |
|
@holiman Great! Thank you for suggesting a fix this quickly! |
|
@holiman how do you produce the graphs above? I am in the process of testing different DBs other than LevelDB. instructions on how to do the benchmark will be amazing and super helpful to me. |
|
@siong1987 The graphs are from Datadog, https://app.datadoghq.com/. Here is a part of the yaml config used to deploy the instance, which configures it to expose If you run it like that, you can visit http://127.0.0.1:6060/debug/metrics and get internal metrics from the instance. The machine also runs a The Then it's just a matter of composing charts with the available metrics inside datadog. The swarm-team use grafana more, which seems like a nice alternative -- you can use whatever graphing tool you like, but |
This PR has some major changes to how we track
dirtyaccounts, before committing changes to database.At present (without this PR), dirty accounts are tracked through
stateObjectsDirty. This tracking mechanism is not based on the journal, but kept totally paralell. Because of this, thestate_objects:s haveonDirtycallback mechanisms to set the dirtyness. It's quite complicated, to be honest.It has an additional quadratic overhead: the between-transaction processing will iterate through the entire
stateObjectsDirtycollection, and update the objects. For a transaction towards the end of the block, this means that ~150 accounts can be processed while in fact there should only be two affected by that transactions.The PR changes that.
stateObjectsDirtyis generated from the flattened journals, which are inserted into the cache.ripeMDhas been moved fromjournalinto the state_object, by explicitly inserting it into thestateObjectsDirty(which means that even if a journal is reverted, it will still reside in the block-level cache ( This could be a (historic) consensus error, since it may mean that the ripeMD isn't completely cleaned until after the end of the block, and not end of transaction. ))Additionally, it happens to pass a couple of more tests than earlier. In general, this is more robust; instead of having dirty-tracking "in paralell", it answers the question "what is dirty" by looking at the canonical source of truth: the journal.
OBS This is a bit of experimental PR.