Tx lookup skip #400

ceyonur · 2023-11-21T00:01:22Z

It modifies type of TxLookupLimit flag from uint64 type to int64 with support for negative numbers which indicates to skip tx indexing.

Using negative number also means stop the tx unindexer from removing old indices. This is because unindexer still writes tail to DB and tries to keep up with the latest tip (even though the actual tx indexing is not performed on DB). Stopping (

Closes: #397

patrick-ogrady · 2023-11-21T00:29:59Z

plugin/evm/config.go

@@ -193,9 +196,10 @@ type Config struct {

 	// TxLookupLimit is the maximum number of blocks from head whose tx indices
 	// are reserved:
+	// 	* -1:  means skip tx indexing


Should also clarify that no txs will be cleaned up in this mode here.

same nit here to align the formatting of -1 with the values below

aaronbuchwald · 2023-11-21T00:32:02Z

eth/ethconfig/config.go

@@ -153,7 +153,8 @@ type Config struct {

 	// TxLookupLimit is the maximum number of blocks from head whose tx indices
 	// are reserved:
+	// 	* -1:  means skip tx indexing


nit: align formatting with the values below

patrick-ogrady · 2023-11-21T03:17:47Z

core/blockchain.go

@@ -413,7 +413,10 @@ func NewBlockChain(
 // unindexBlocks unindexes transactions depending on user configuration
 func (bc *BlockChain) unindexBlocks(tail uint64, head uint64, done chan struct{}) {
 	start := time.Now()
-	txLookupLimit := bc.cacheConfig.TxLookupLimit
+	if bc.cacheConfig.TxLookupLimit < 0 {


Because unindexBlocks is spawned by dispatchTxUnindexer, do we need to worry about this?

yup correct. I put it to be extra safe (in case this is directly called by tests), but I guess it's fine to just add it as a comment.

patrick-ogrady · 2023-11-21T03:18:16Z

core/blockchain.go

@@ -431,7 +434,10 @@ func (bc *BlockChain) unindexBlocks(tail uint64, head uint64, done chan struct{}
 // Meaning that this function should never be called.
 func (bc *BlockChain) dispatchTxUnindexer() {
 	defer bc.wg.Done()
-	txLookupLimit := bc.cacheConfig.TxLookupLimit
+	if bc.cacheConfig.TxLookupLimit < 0 {


Because we check TxLookupLimit before spawning dispatchTxUnindexer, do we need to worry about this?

responded: #400 (comment)

patrick-ogrady · 2023-11-21T03:22:48Z

This is very close. I think it is functionally correct right now (just has unnecessary checks).

aaronbuchwald · 2023-11-21T14:37:37Z

core/blockchain_test.go

+	limit := []int64{130 /* 129 + 1 reserve all */, 64 /* drop stale */, 32 /* shorten history */, -1 /* skip indexing */, -1 /* skip indexing */}
+	tails := []uint64{0 /* reserve all */, 67 /* 130 - 64 + 1 */, 100 /* 131 - 32 + 1 */, 100 /* skip indexing */, 100 /* skip indexing */}
+	// lastIndexed should stop with limit=-1 otherwise it should be the current block number
+	lastIndexed := []uint64{129 /* current block number */, 130 /* current block number */, 131 /* current block number */, 131 /* skip indexing */, 131 /* skip indexing */}


we should convert this to a struct in the future. It was a bit hard to follow before and going from 2 to 3 values makes it a bit harder to follow.

might be nice to do this as part of this PR (or immediate follow-up), doesn't seem like a big change.

darioush · 2023-11-21T17:09:08Z

core/blockchain.go

@@ -160,7 +160,7 @@ type CacheConfig struct {
 	SnapshotVerify                  bool          // Verify generated snapshots
 	Preimages                       bool          // Whether to store preimage of trie key to the disk
 	AcceptedCacheSize               int           // Depth of accepted headers cache and accepted logs cache at the accepted tip
-	TxLookupLimit                   uint64        // Number of recent blocks for which to maintain transaction lookup indices
+	TxLookupLimit                   int64         // Number of recent blocks for which to maintain transaction lookup indices


Should we add a new parameter liketx-indexing-disabled instead of adding -1?
Seems this would simplify validation

In that case we should validate it against TxLookupLimit and it might be confusing to use both of them at the same time.

darioush · 2023-11-21T17:11:38Z

core/blockchain.go

 		bc.wg.Add(1)
 		go bc.dispatchTxUnindexer()
 	}
 	return bc, nil
 }

 // unindexBlocks unindexes transactions depending on user configuration
+// Invariant: This should not be called if TxLookupLimit is negative.


would it be clearer to take a uint param here & to dispatchTxUnindexer?

darioush · 2023-11-21T17:12:42Z

core/blockchain.go

@@ -485,7 +487,10 @@ func (bc *BlockChain) dispatchTxUnindexer() {
 // - updating the acceptor tip index
 func (bc *BlockChain) writeBlockAcceptedIndices(b *types.Block) error {
 	batch := bc.db.NewBatch()
-	rawdb.WriteTxLookupEntriesByBlock(batch, b)
+	// -1 means skip writing tx lookup entries
+	if bc.cacheConfig.TxLookupLimit != -1 {


Let's be consistent with using < 0 here

I specifically kept it -1 to make it consistent with docs/comments.

darioush · 2023-11-21T17:18:28Z

core/blockchain_test.go

@@ -749,7 +775,7 @@ func TestTransactionIndices(t *testing.T) {
 // TestCanonicalHashMarker tests all the canonical hash markers are updated/deleted
 // correctly in case reorg is called.
 func TestCanonicalHashMarker(t *testing.T) {
-	var cases = []struct {
+	cases := []struct {


could we revert the unnecessary changes?

darioush · 2023-11-21T17:20:16Z

core/blockchain_test.go

+	limit := []int64{130 /* 129 + 1 reserve all */, 64 /* drop stale */, 32 /* shorten history */, -1 /* skip indexing */, -1 /* skip indexing */}
+	tails := []uint64{0 /* reserve all */, 67 /* 130 - 64 + 1 */, 100 /* 131 - 32 + 1 */, 100 /* skip indexing */, 100 /* skip indexing */}
+	// lastIndexed should stop with limit=-1 otherwise it should be the current block number
+	lastIndexed := []uint64{129 /* current block number */, 130 /* current block number */, 131 /* current block number */, 131 /* skip indexing */, 131 /* skip indexing */}


might be nice to do this as part of this PR (or immediate follow-up), doesn't seem like a big change.

ceyonur · 2023-11-21T19:34:31Z

closing in favor of #399

ceyonur added 2 commits November 21, 2023 02:56

skip tx lookup indexing

e68bbd4

restore format

0459c63

ceyonur added the enhancement New feature or request label Nov 21, 2023

ceyonur self-assigned this Nov 21, 2023

fix config tests

5327d12

ceyonur mentioned this pull request Nov 21, 2023

add skip index flag #399

Merged

ceyonur added 2 commits November 21, 2023 03:10

update comment

c07fdd0

use -1 for the config

ea80bf7

ceyonur marked this pull request as ready for review November 21, 2023 00:20

ceyonur requested review from aaronbuchwald and darioush as code owners November 21, 2023 00:20

ceyonur requested a review from patrick-ogrady November 21, 2023 00:20

restore format

5d24e22

patrick-ogrady reviewed Nov 21, 2023

View reviewed changes

aaronbuchwald reviewed Nov 21, 2023

View reviewed changes

update comment

b5946a9

aaronbuchwald previously approved these changes Nov 21, 2023

View reviewed changes

improve test logs

96a0f15

ceyonur dismissed aaronbuchwald’s stale review via 96a0f15 November 21, 2023 00:57

ceyonur added 2 commits November 21, 2023 04:14

prevent pointer modification

e0190b7

restore format

093000f

patrick-ogrady reviewed Nov 21, 2023

View reviewed changes

remove extra checks

81442c7

aaronbuchwald reviewed Nov 21, 2023

View reviewed changes

ceyonur added 2 commits November 21, 2023 18:59

increase sleep

828a706

Merge branch 'master' into tx-lookup-skip

f096763

darioush reviewed Nov 21, 2023

View reviewed changes

ceyonur closed this Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tx lookup skip #400

Tx lookup skip #400

ceyonur commented Nov 21, 2023 •

edited

patrick-ogrady Nov 21, 2023

aaronbuchwald Nov 21, 2023

aaronbuchwald Nov 21, 2023

patrick-ogrady Nov 21, 2023

ceyonur Nov 21, 2023

patrick-ogrady Nov 21, 2023

ceyonur Nov 21, 2023

patrick-ogrady commented Nov 21, 2023

aaronbuchwald Nov 21, 2023

darioush Nov 21, 2023

darioush Nov 21, 2023

ceyonur Nov 21, 2023

darioush Nov 21, 2023

darioush Nov 21, 2023

ceyonur Nov 21, 2023

darioush Nov 21, 2023

darioush Nov 21, 2023

ceyonur commented Nov 21, 2023

Tx lookup skip #400

Tx lookup skip #400

Conversation

ceyonur commented Nov 21, 2023 • edited

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

patrick-ogrady commented Nov 21, 2023

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

Choose a reason for hiding this comment

ceyonur commented Nov 21, 2023

ceyonur commented Nov 21, 2023 •

edited