core: init tx lookup in background by rjl493456442 · Pull Request #19853 · ethereum/go-ethereum

rjl493456442 · 2019-07-18T11:11:31Z

In our schema, for each transaction, we will maintain an index. The index is basically a pair like txHash->blockNumber which indicators the canonical block number this transaction is included. In another word, tx index is necessary for users to query a transaction based on the hash.

However there are 580M transactions in the mainnet. So the indexes can take more than 10GB space in the database. It's a lot.

So the idea is: for most users, they only care about the latest transactions they send. So we can drop off historical indexes to reduce disk usage.

So in this PR, we offer a new command flag called txlookuplimit. User can specify how many indexes they want to maintain in db. For the older, we will GC them.

What's more, if users want to query some historical transactions when the indexes are GCed, he can change the txlookuplimit, so that in the background Geth will re-generate indexes. If txlookuplimit is 0, it means we generate all indexes.

karalabe · 2019-07-18T12:04:16Z

We should make this a bit more flexible. Namely, I'd like to be able to specify an "recentness" flag too, below which ancient tx indices get deleted. This should be dynamic, and restarting Geth with different values should expand/contract the indexed interval appropriately.

The reasoning is that tx indexes are not needed for operating a full node, only if you want to look up ancient transactions or receipts. Most people will not want to do that, so if I am willing to not care about transactions older than 1 month for example, I can get rid of 20+GB of SSD space, without impacting the network.

Ideally, I'd like to specify a --txlookuplimit=90000:

Archive node disallows this option (i.e. reject to start).
A full syncing node would maintain an "oldest indexed block" entry, and every time a new block is imported, HEAD-90000 is unindexed. Restarting Geth with a different value would reindex (downward) or unindex (upward) from that pointer.
A fast syncing node would do the same CHT trick we have for ancient blocks, where nothing is indexed under the CHT block - 90000. Then when more blocks are imported on top, this index start pointer can be moved forward and stuff unindexed. Alternatively, we can finish a fast sync fully, and then start indexing stuff.

karalabe

Haven't reviewed all of it yet, but I think there's enough to keep you busy a bit :)

karalabe · 2019-07-25T06:43:59Z

 	CheckExclusive(ctx, DeveloperFlag, TestnetFlag, RinkebyFlag, GoerliFlag)
 	CheckExclusive(ctx, LightLegacyServFlag, LightServeFlag, SyncModeFlag, "light")
 	CheckExclusive(ctx, DeveloperFlag, ExternalSignerFlag) // Can't use both ephemeral unlocked and external signer
+	CheckExclusive(ctx, GCModeFlag, "archive", TxLookupLimitFlag)


Wondering whether we should limit it for light servers too?

Limit for light servers might be feasible.

If the Geth node already drops some ancient tx indices, later it launches with les server enabled, then it HAS TO reconstruct all missing indices.
It's a good solution for the short term. But for the long term, we definitely need to add some information into handshake package for the light client to tell it these part of indices are unavailable.
If a light client wants to access historical indices, it needs to search for a suitable server for a while. Maybe server can also advertise itself that: it has all historical tx indices available but the service fee is a bit higher.

karalabe · 2019-07-25T06:48:32Z

 	}
 	vmcfg := vm.Config{EnablePreimageRecording: ctx.GlobalBool(VMEnableDebugFlag.Name)}
-	chain, err = core.NewBlockChain(chainDb, cache, config, engine, vmcfg, nil)
+	chain, err = core.NewBlockChain(chainDb, cache, config, engine, vmcfg, nil, 0)


Shouldn't we pass the tx limit here too? I guess this is an interesting issue, because if we don't, then a geth inspect (or geth init) could end up regenerating an index in the background. On the other hand, if we do, then the user would still need to pass --txlookuplimit to these commands, which they arguably won't. We might need to have two flags on the NewBlockChain: one the limit itself, the second whether to enforce anything or just leave it be. Perhaps instead of int, we could use *int. Then 0 means no limit (and regenerate any missing), N means N block limit and nil means don't care about the limit, leave as is.

Tx indices pruning or regeneration is only triggered by ChainHeadEvent, so we don't need to worry about it :))

But you are right, there have a few edge cases we need to take care. I will check more.

Seems a bit implicit/brittle in the long term. Wouldn't it make things a bit more predictable if this was explicit? Maybe not, just thinking out aloud here.

karalabe · 2019-07-25T06:57:15Z

+			// We don't write tx lookup indices here because Geth can offer a CLI flag `txlookuplimt`
+			// with which user can choose to drop historical indices data and only keep latest indices.
+			// After the fast sync, we will reconstruct all missing indices even user requires to keep
+			// all historical indices.


I think we should be a bit smarter here. The same way that we have writeAncient and writeLive, we could make tx indexing run when actually needed based on the same mechanism. We can use the same threshold that splits live from ancient:

If the "receipt is live", assume the tx index is needed too (we can clean it up afterwards if not).

If the "receipt is ancient", calculate ancient threshold - tx lookup limit and use that to decide to write or not.

The end result would be that if we have a CHT at block 8M, and txlookuplimit is 100K, then during fast sync, we ignore the first 7.9M blocks' txs, but we do write anything above 8M-100K. Then when fast sync completes we can delete anything extra.

My rationale behind this is that people kind of assume that when fast sync completes, you have a live node. Scripts assume this. If you explicitly mess with --txlookuplimit, then yes, you need to "wait it out", but if you don't change it, just run with some number from the get go, I think Geth should be considered live when sync completes.

E.g. I simply do geth, I'd expect the node to respond after fast sync, whereas currently the background indexing will take one more hour potentially. If we do implement this change, then not specifying a txlimit will fall back to the old behavior, whereas specifying a limit of N will ensure that we have "more" than N available on sync completion, so it's fine (more than we need, but we'll clean it up).

The main concern for not adding tx indices writing logic in fast sync is: user can specify different limit value.

E.g.

First time user specifies limit as 9K and launch a fast sync

Before the fast sync finishes, user stops the Geth and restart it with limit as 0
But it's hard for Geth to know that a few historical tx indices actually has been dropped unless we record the limit value in database.

An optional solution for this is:

We only write TxIndexTail in blockchain.FastSyncCommitHead function. The tail value is the start block number of this fast sync(if txlookuplimit=0). Since we can always ensure that tx indices from start block number until povit point exists in database. Regarding the historical indices, let background gorontine fixes these.

But the drawback of this solution is: If the current fast block head is 10K, then we run fast sync with limit=0, and we fetch 1K and fast sync is finished. So we have to reindex the first 10K indices. It's kind of painful. Also it's can be confused for users that fast sync is done, but some historical transacitons are missing(but maybe we can fix it by offering a API eth.indexing)

karalabe · 2019-07-25T07:05:11Z

 	}
 	// Take ownership of this particular state
 	go bc.update()
+	go bc.updateTxIndices()


I think we can be a bit more reliable here, but I'm not sure whether we want to or not.

The current code is fully background: if I specify a smaller tx limit, it will slowly crunch in the background and delete unneeded stuff. This is desirable.

Pondering about the other way though. If I ran Geth with a limit of say 30K, and realize it's too little and change to 300K, should we do the expansion on a background thread, or should we "fix the database" on startup? Doing in the the background would be "nicer", doing it in the foreground would be more predictable.

My main concern is that we need a way to programatically decide if the node is ready or not. If we block on startup with indexing, it's obvious (APIs are not opened until indexinig finished), but it's overkill for users. Maybe a good alternative would be to keep the background approach, but have an eth.indexing API, the same way we have eth.syncing. We could add stats about tx indexing, bloombits, cht, etc.

Probably we need a way to disable this goroutine in certain cases where we don't want background updates happening (geth init, geth inspect, etc).

karalabe · 2019-07-25T07:11:40Z

+//
+// The user can adjust the txlookuplimit value for each launch, Geth will
+// automatically construct the missing indices and delete the extra indices.
+func (bc *BlockChain) updateTxIndices() {


Lets call this method maintainTxIndex. It seems more appropriate.

karalabe · 2019-07-25T07:13:56Z

+		if sub != nil {
+			sub.Unsubscribe()
+		}
+	}()


Lets move the subscription creation and defer below the method declarations. Seems cleaner code that we subscribe and immediately afterwards have the handler logic.

karalabe · 2019-07-25T07:16:06Z

+			if done == nil {
+				done = make(chan struct{})
+				if number := rawdb.ReadOldestIndexedBlock(bc.db); number == nil {
+					go initialiseIndices(head.Block.NumberU64())


Lets pass the done channel as parameter and only declare done before this for loop. The code will be a lot more readable since we don't have the done channel leaking as a closure into the methods.

karalabe · 2019-07-25T07:20:37Z

+	// If there already exists some indices, this function will find
+	// the oldest block which has been indexed and start indexing from
+	// this point.
+	initialiseIndices := func(head uint64) {


Do we need this initialiseIndices? If we fix the receipt writer in fast sync to actually push the tx indexes too for recent blocks, then I think that would already make this part unneeded. Maybe a fully empty chain would need some sanity checks, but not heavy reindexing.

karalabe · 2019-07-25T07:42:54Z

 	fastTrieProgressKey = []byte("TrieSync")

+	// oldestIndexedBlockKey tracks the oldest block whose transaction indices(txlookup) has been indexed.
+	oldestIndexedBlockKey = []byte("OldestIndexedBlock")


I think it's a bit misleading to call this "indexed block", because it's too generic. Let's call it (and update the methods associated) TxIndexTail. This way it's obvious both what it is as well as that it's only about txs, not generic block indexing.

rjl493456442 · 2019-07-31T04:55:01Z

@karalabe I pushed lots of change since your last review, could you please take another look :)

adamschmideg · 2019-11-12T08:10:46Z

@rjl493456442 Can you please give some context to the casual reader at the start of this conversation

rjl493456442 · 2019-11-12T08:21:19Z

@adamschmideg Hi Adam, thanks for looking at the PR.

The background of this PR is:

In our schema, for each transaction, we will maintain an index. The index is basically a pair like txHash->blockNumber which indicators the canonical block number this transaction is included. In another word, tx index is necessary for users to query a transaction based on the hash.

However there are 580M transactions in the mainnet. So the indexes can take more than 10GB space in the database. It's a lot.

So the idea is: for most users, they only care about the latest transactions they send. So we can drop off historical indexes to reduce disk usage.

So in this PR, we offer a new command flag called txlookuplimit. User can specify how many indexes they want to maintain in db. For the older, we will GC them.

What's more, if users want to query some historical transactions when the indexes are GCed, he can change the txlookuplimit, so that in the background Geth will re-generate indexes. If txlookuplimit is 0, it means we generate all indexes.

rjl493456442 · 2019-11-19T08:28:53Z

Close it because #20270

rjl493456442 requested review from holiman and karalabe as code owners July 18, 2019 11:11

rjl493456442 requested review from gballet and zsfelfoldi as code owners July 22, 2019 07:46

rjl493456442 force-pushed the bg-reindex branch 6 times, most recently from ace93c6 to 2dd6796 Compare July 23, 2019 07:10

karalabe suggested changes Jul 25, 2019

View reviewed changes

rjl493456442 force-pushed the bg-reindex branch 2 times, most recently from 76f5bc3 to 76fe300 Compare July 29, 2019 11:59

rjl493456442 added 11 commits August 29, 2019 15:12

core: init tx lookup in background

4f0cc2d

cmd, core, eth: version 2

2c7b166

cmd/utils: disable txlookup pruning for archive node

dac227d

core: skip tx indices writing during fast sync

a536c5d

cmd/utils, core/rawdb: address some comments

24192a9

core, eth: skip transaction indexing during fast sync if required

89ad90d

cmd/utils, core, eth/download: resolve txLookupLimit for db cmds

0180d5e

cmd, core, eth: add comments

ace22f8

core/rawdb: add chain iterator unit test

f8ffdfe

all: change unit tests

7e6c82d

core: add fixes

3461f71

karalabe force-pushed the bg-reindex branch from 76fe300 to e4965d1 Compare August 29, 2019 15:24

cmd, core: minor polishes to the transaction indexer

5744542

karalabe force-pushed the bg-reindex branch from e4965d1 to 5744542 Compare August 29, 2019 15:25

core: generate indices if txIndexTail is not nil

2d14844

karalabe mentioned this pull request Nov 12, 2019

cmd, core, eth: init tx lookup in background #20270

Closed

rjl493456442 closed this Nov 19, 2019

Conversation

rjl493456442 commented Jul 18, 2019 • edited by adamschmideg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karalabe commented Jul 18, 2019

Uh oh!

karalabe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjl493456442 Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjl493456442 commented Jul 31, 2019

Uh oh!

adamschmideg commented Nov 12, 2019

Uh oh!

rjl493456442 commented Nov 12, 2019

Uh oh!

rjl493456442 commented Nov 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rjl493456442 commented Jul 18, 2019 •

edited by adamschmideg

Loading

rjl493456442 Jul 25, 2019 •

edited

Loading