core: write chain data in atomic way #20287

rjl493456442 · 2019-11-14T08:05:32Z

This PR changes the way to write chain data.

Now we can classify the chain data into two parts:

Header chain data
Block chain data

The reason we classify it is:

for light client, it only maintains the header chain
for sync, we write header chain first and then fulfill the remaining data like body and receipts.

So that there are two separate data structures headerchain and blockchain.

The pain point is both header chain data and block chain data are divided into several components
For header chain, we have:

header
total difficulty
hash -> number mapping

For block chain, we have:

header
total difficulty
hash -> number mapping
body
receipts

So in order to ensure the integrity of the chain data, all the components have to be written in an atomic way.
So this is the first thing this PR does.

And what's more, except the chain data, there are some additional index data.
For header chain, there are:

canonical index(number -> hash mapping)
head header flag

For block chain, there are:

canonical index(number -> hash mapping)
head header flag
tx indexes(tx hash -> canonical block number)
head fast block flag
head full block flag.

So for these indexes, they also have to be written in an atomic way. But they are independent with chain data. The basic work flow is: write the chain data first irrelevant of the canonical status, then commit the indexes if it's a canonical header or block.

So this PR also ensures all indexes are written in an atomic way.

So finally we can address some issues caused by uncompleteness of chain data.

holiman

This looks correct to me, though I have some minor questions. I think we should run this a bit on some nodes -- it's really tricky to figure out possible flaws using 👀 only.

core/blockchain.go

holiman · 2019-11-14T10:58:52Z

core/blockchain.go

+	batch := bc.db.NewBatch()
+	rawdb.WriteCanonicalHash(batch, block.Hash(), block.NumberU64())
+	if updateHeads {
+		bc.hc.WriteHeadHeader(batch, block.Header())


Although the batch hasn't been flushed yet, the call to WriteHeadHeader internally calls hc.SetCurrentHeader(head). I suppose it's safe enough anyway,just wanted to point out that there's a brief interval of mismatch between what's in memory here and what's flushed..

Also, the previous code used block.Hash() which remembers the hash - here it becomes recalculated. Could you make WriteHeadHeader take the hash as input?

Thanks for pointing out here. You are right I can't call WriteHeadHeader here directly. Although the interval is very short, but it's still better to prevent it. I will use rawdb.WriteHeadHeaderHash(db, head.Hash()) instead

Well, have the tests, and find if we want this guarantee that "all changes be pushed into disk first and then update the in-memory flags", then we have to break the WriteHeadHeader into 2 separate steps.

It's total fine, but just it's not a good approach. WriteHeadHeader is kind of self-contained function which processes all logics in the headerchain. It's weird to call rawdb.WriteHeadHeaderHash(db, head.Hash()) directly.

karalabe · 2019-11-19T10:40:24Z

core/blockchain.go

-		bc.hc.SetCurrentHeader(block.Header())
-		rawdb.WriteHeadFastBlockHash(bc.db, block.Hash())
-
+		rawdb.WriteHeadFastBlockHash(batch, block.Hash())


Same issue here. We're pushing the head fast-block into the batch, but updating the memory-marker immediately.

holiman · 2019-11-19T10:40:41Z

core/headerchain.go

+// and write the marker into database.
+func (hc *HeaderChain) WriteHeadHeader(db ethdb.KeyValueWriter, head *types.Header) {
+	rawdb.WriteHeadHeaderHash(db, head.Hash())
+	hc.SetCurrentHeader(head)


This should probably not be done here (while still only writing to batch), but done by caller after the batch is committed

karalabe · 2019-11-19T10:40:57Z

core/blockchain.go

 		bc.currentFastBlock.Store(block)
 		headFastBlockGauge.Update(int64(block.NumberU64()))
 	}
+	if err := batch.Write(); err != nil {
+		log.Crit("Failed to update chain indexes and markers", "err", err)
+	}


We should only update the markers here.

Same for caches imho

karalabe · 2019-11-19T11:09:53Z

light/lightchain.go

+
+	if err := batch.Write(); err != nil {
+		log.Crit("Failed to reset genesis block", "err", err)
+	}


Lets update the genesisBlock and hc.SetGenesis after the components are written?

core/blockchain.go

holiman · 2019-11-20T08:19:28Z

core/blockchain.go

-	if err := bc.hc.WriteTd(block.Hash(), block.NumberU64(), td); err != nil {
-		return err
+	batch := bc.db.NewBatch()
+	rawdb.WriteTd(batch, block.Hash(), block.NumberU64(), td)


The new WriteTd method does not store things in to the tdCache, right? Don't we need to add it there explicitly now? Afaict that's only done through headerchain, not blockchain, so I'm not sure it will get there if we sync block by block?

Yes, it's quite annoying. In theory, td is a part of header chain data, but some times we write the whole block with the given td. So we need to explicitly set the td into the cache.

Btw we never update the other cache(bodyCache, receiptCache) when we write the block.

holiman · 2019-11-20T08:19:45Z

core/blockchain.go

+	// Note all the components of block(td, hash->number map, header, body, receipts)
+	// should be written atomically. BlockBatch is used for containing all components.
+	blockBatch := bc.db.NewBatch()
+	rawdb.WriteTd(blockBatch, block.Hash(), block.NumberU64(), externTd)


Same question about tdcache as above

holiman

This looks good to me

karalabe

I don't see anything wrong with this PR. Let's hope for the best? ;P

ref: ethereum/go-ethereum#20287

* core: write chain data in atomic way * core, light: address comments * core, light: fix linter * core, light: address comments

rjl493456442 requested review from holiman, karalabe and zsfelfoldi as code owners November 14, 2019 08:05

holiman reviewed Nov 14, 2019

View reviewed changes

karalabe reviewed Nov 19, 2019

View reviewed changes

holiman reviewed Nov 19, 2019

View reviewed changes

karalabe reviewed Nov 19, 2019

View reviewed changes

core/blockchain.go Outdated Show resolved Hide resolved

rjl493456442 added 2 commits November 20, 2019 09:46

core: write chain data in atomic way

3c57935

core, light: address comments

51390a5

rjl493456442 force-pushed the make-write-atomic branch 2 times, most recently from 99ba3c9 to 71048f7 Compare November 20, 2019 02:27

core, light: fix linter

61ba56f

rjl493456442 force-pushed the make-write-atomic branch from 71048f7 to 61ba56f Compare November 20, 2019 02:45

holiman reviewed Nov 20, 2019

View reviewed changes

core, light: address comments

6521397

adamschmideg added the status:triage label Dec 17, 2019

karalabe assigned karalabe and holiman Jan 14, 2020

karalabe removed the status:triage label Jan 14, 2020

holiman approved these changes Jan 14, 2020

View reviewed changes

karalabe added this to the 1.9.10 milestone Jan 17, 2020

karalabe approved these changes Jan 17, 2020

View reviewed changes

karalabe merged commit 770316d into ethereum:master Jan 17, 2020

holiman mentioned this pull request Feb 3, 2020

PoA miner panic when importing blocks #20614

Closed

holiman mentioned this pull request Feb 13, 2020

Segmentation fault in 1.9.0-unstable #19198

Closed

rjl493456442 mentioned this pull request Feb 25, 2020

Freezer is failed to freeze data #20239

Closed

vdamle mentioned this pull request Jan 22, 2021

Chain rewind and failure to sync with err=missing parent after non-graceful restart of nodes Consensys/quorum#1117

Closed

vdamle pushed a commit to kaleido-io/quorum that referenced this pull request Jan 22, 2021

core: write chain data in atomic way

8d128e8

ref: ethereum/go-ethereum#20287

vdamle pushed a commit to kaleido-io/quorum that referenced this pull request Jan 26, 2021

core: write chain data in atomic way

360f0cc

ref: ethereum/go-ethereum#20287

ricardolyn mentioned this pull request Feb 3, 2021

[Upgrade] Go-Ethereum release v1.9.11 Consensys/quorum#1121

Merged

7 tasks

gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 28, 2024

core: write chain data in atomic way (ethereum#20287)

d7e0e9d

gzliudan mentioned this pull request Dec 28, 2024

upgrade the core package for reorg XinFinOrg/XDPoSChain#779

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: write chain data in atomic way #20287

core: write chain data in atomic way #20287

rjl493456442 commented Nov 14, 2019 •

edited

Loading

holiman left a comment

holiman Nov 14, 2019

holiman Nov 14, 2019

rjl493456442 Nov 14, 2019

rjl493456442 Nov 14, 2019

karalabe Nov 19, 2019 •

edited

Loading

holiman Nov 19, 2019

karalabe Nov 19, 2019

karalabe Nov 19, 2019

karalabe Nov 19, 2019

holiman Nov 20, 2019

rjl493456442 Nov 20, 2019

holiman Nov 20, 2019

holiman left a comment

karalabe left a comment

core: write chain data in atomic way #20287

core: write chain data in atomic way #20287

Conversation

rjl493456442 commented Nov 14, 2019 • edited Loading

holiman left a 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

karalabe Nov 19, 2019 • edited Loading

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

holiman left a comment

Choose a reason for hiding this comment

karalabe left a comment

Choose a reason for hiding this comment

rjl493456442 commented Nov 14, 2019 •

edited

Loading

karalabe Nov 19, 2019 •

edited

Loading