From a4503b2e1d109f73ca3b4cbc6162ca2cb89faeb6 Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 30 Aug 2022 08:35:32 -0700 Subject: [PATCH 01/15] consensus/ethash: test that ethash cache gets return expected epochs This test reproduces an error encountered on the Mordor testnet and reported here https://github.com/etclabscore/core-geth/issues/439 Ethash returns a cache value with epoch length of 30k, where it should return one appropriate for the ECIP1099-era with an epoch length of 60k. This caused header verification to fail because the mix digest was incorrect. This issue only occurred while syncing the Mordor network because the cache was in-memory and because the original-era epoch was matched later by the ECIP1099-era epoch and the LRU future item was anachronistic. Date: 2022-08-30 08:35:32-07:00 Signed-off-by: meows --- consensus/ethash/ethash_test.go | 91 +++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/consensus/ethash/ethash_test.go b/consensus/ethash/ethash_test.go index 36862eaa40..c5b993b9ae 100644 --- a/consensus/ethash/ethash_test.go +++ b/consensus/ethash/ethash_test.go @@ -21,6 +21,7 @@ import ( "math/big" "math/rand" "os" + "path/filepath" "sync" "testing" "time" @@ -28,8 +29,98 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common/hexutil" "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/log" ) +func verboseLogging() { + glogger := log.NewGlogHandler(log.StreamHandler(os.Stdout, log.TerminalFormat(false))) + glogger.Verbosity(log.Lvl(99)) + log.Root().SetHandler(glogger) +} + +func TestEthashCaches(t *testing.T) { + verboseLogging() + + // Make a copy of the default config. + conf := Config{ + CacheDir: filepath.Join(os.TempDir(), "ethash-cache-test-cachedir"), + CachesInMem: 2, + CachesOnDisk: 3, + CachesLockMmap: false, + DatasetsInMem: 1, + DatasetsOnDisk: 2, + DatasetsLockMmap: false, + DatasetDir: filepath.Join(os.TempDir(), "ethash-cache-test-datadir"), + PowMode: ModeNormal, + } + + // Clean up ahead of ourselves. + os.RemoveAll(conf.CacheDir) + os.RemoveAll(conf.DatasetDir) + + // And after ourselves. + defer os.RemoveAll(conf.CacheDir) + defer os.RemoveAll(conf.DatasetDir) + + // Use some "big" arbitrary multiple to make sure that simulate real life adequately. + testIterationMultiple := 6 + ecip1099Block := uint64(epochLengthDefault * conf.CachesInMem * testIterationMultiple) + conf.ECIP1099Block = &ecip1099Block + + // Construct our Ethash + e := New(conf, nil, false) + + trialMax := ecip1099Block * uint64(testIterationMultiple) * 2 + latestIteratedEpoch := uint64(math.MaxInt64) + for n := uint64(0); n < trialMax; n += epochLengthDefault / 300 { + + // Calculate the epoch number independently to use for logging and debugging. + epochLength := epochLengthDefault + if n >= ecip1099Block { + epochLength = epochLengthECIP1099 + } + ep := calcEpoch(n, uint64(epochLength)) + epl := calcEpochLength(n, conf.ECIP1099Block) + + if ep != latestIteratedEpoch { + t.Logf("block=%d epoch=%d epoch.len=%d ECIP1099=/%d (%0.1f%%) RANGE=/%d (%0.1f%%)", + n, + ep, epl, + ecip1099Block, float64(n)/float64(ecip1099Block)*100, + trialMax, float64(n)/float64(trialMax)*100, + ) + latestIteratedEpoch = ep + } + + // This is the tested function. + c := e.cache(n) + c.finalizer() + + // Do we get the right epoch length? + if n >= ecip1099Block && c.epochLength != epochLengthECIP1099 { + // Give the future epoch routine a chance to finish. + time.Sleep(1 * time.Second) + + // current status + t.Logf("block=%d epoch=%d epoch.len=%d ECIP1099=/%d (%0.1f%%) RANGE=/%d (%0.1f%%)", + n, + ep, epl, + ecip1099Block, float64(n)/float64(ecip1099Block)*100, + trialMax, float64(n)/float64(trialMax)*100, + ) + + // ls -l /tmp/ethash-cache-test-cachedir + entries, _ := os.ReadDir(conf.CacheDir) + t.Log("cachedir", conf.CacheDir) + for _, entry := range entries { + t.Logf(` - %s\n`, entry.Name()) + } + + t.Fatalf("Unexpected epoch length: %d", c.epochLength) + } + } +} + // Tests caches get sets correct future func TestCachesGet(t *testing.T) { ethashA := NewTester(nil, false) From 129bfc3209e1dbdfdef9e08ee3917e3df79d0f47 Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 30 Aug 2022 08:41:28 -0700 Subject: [PATCH 02/15] consensus/ethash: fix dangling future epoch from pre-ECIP1099 era The ECIP1099 transition causes the epoch numbers to halve, causing the lru.future value to be ahead of (greater than) the current epoch, making this conditional (quietly) fail. After a while (eg. syncing on Mordor), the lru.future value becomes spuriously available again because the epoch heights eventually match up again, but when they do, the lru.future value references the pre-ECIP1099 era. Date: 2022-08-30 08:41:28-07:00 Signed-off-by: meows --- consensus/ethash/ethash.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/consensus/ethash/ethash.go b/consensus/ethash/ethash.go index 4a4b8d6219..da722f6076 100644 --- a/consensus/ethash/ethash.go +++ b/consensus/ethash/ethash.go @@ -250,7 +250,9 @@ func (lru *lru) get(epoch uint64, epochLength uint64, ecip1099FBlock *uint64) (i } // Update the 'future item' if epoch is larger than previously seen. - if epoch < maxEpoch-1 && lru.future < nextEpoch { + // Last conditional clause ('lru.future > nextEpoch') handles the ECIP1099 case where + // the next epoch is expected to be LESSER THAN that of the previous state's future epoch number. + if epoch < maxEpoch-1 && (lru.future < nextEpoch || lru.future > nextEpoch) { log.Trace("Requiring new future ethash "+lru.what, "epoch", nextEpoch) future = lru.new(nextEpoch, nextEpochLength) lru.future = nextEpoch From dad1013aae51fd1346d91b95df3a5939b8806be4 Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 30 Aug 2022 08:44:22 -0700 Subject: [PATCH 03/15] consensus/ethash: avoid cache key collisions This handles potential cache collisions due to ECIP1099. epoch/epochLength is sufficiently unique, where epoch alone could be duplicated by post-ECIP1099 epochs. Date: 2022-08-30 08:44:22-07:00 Signed-off-by: meows --- consensus/ethash/ethash.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/consensus/ethash/ethash.go b/consensus/ethash/ethash.go index da722f6076..73002c7c2f 100644 --- a/consensus/ethash/ethash.go +++ b/consensus/ethash/ethash.go @@ -226,8 +226,9 @@ func (lru *lru) get(epoch uint64, epochLength uint64, ecip1099FBlock *uint64) (i lru.mu.Lock() defer lru.mu.Unlock() + cacheKey := fmt.Sprintf("%d-%d", epoch, epochLength) // Get or create the item for the requested epoch. - item, ok := lru.cache.Get(epoch) + item, ok := lru.cache.Get(cacheKey) if !ok { if lru.future > 0 && lru.future == epoch { item = lru.futureItem @@ -235,7 +236,7 @@ func (lru *lru) get(epoch uint64, epochLength uint64, ecip1099FBlock *uint64) (i log.Trace("Requiring new ethash "+lru.what, "epoch", epoch) item = lru.new(epoch, epochLength) } - lru.cache.Add(epoch, item) + lru.cache.Add(cacheKey, item) } // Ensure pre-generation handles ecip-1099 changeover correctly From f4c42dcc40b7b2778957944141d2edd1d972f038 Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 30 Aug 2022 08:46:28 -0700 Subject: [PATCH 04/15] consensus/ethash: use a more general condition for ECIP1099 activation Prior to this patch, the code assumed that ECIP1099 would (must) be activated on an epoch transition block value, ie. a multiple of 30_000. This patch removes this assumption. Date: 2022-08-30 08:46:28-07:00 Signed-off-by: meows --- consensus/ethash/ethash.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/ethash/ethash.go b/consensus/ethash/ethash.go index 73002c7c2f..e2b0c66457 100644 --- a/consensus/ethash/ethash.go +++ b/consensus/ethash/ethash.go @@ -244,7 +244,7 @@ func (lru *lru) get(epoch uint64, epochLength uint64, ecip1099FBlock *uint64) (i var nextEpochLength = epochLength if ecip1099FBlock != nil { nextEpochBlock := nextEpoch * epochLength - if nextEpochBlock == *ecip1099FBlock && epochLength == epochLengthDefault { + if nextEpochBlock >= *ecip1099FBlock && epochLength == epochLengthDefault { nextEpoch = nextEpoch / 2 nextEpochLength = epochLengthECIP1099 } From 012575fd5bdf6e3987f38ffe73a34f7e4523ee23 Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 30 Aug 2022 08:53:57 -0700 Subject: [PATCH 05/15] consensus/ethash: include epoch number in cache filename Prior to this patch, existing cache files could only be identified unidirectionally, by matching a seedhash generated for some epoch. This is problematic for ECIP1099 because epoch numbers are no longer continuous, so there were dangling cache files (since the iterator removing them had to use the contemporary cache epoch, which may have recently been halved). This patch allows cache files to be iterated and identified by the epoch they represent using scanned values from their filename. This avoids spurious cache file hits using only seed hash alone that could have been caused by dangling cache files. Legacy cache files (by matching legacy naming scheme) will be removed. Date: 2022-08-30 08:53:57-07:00 Signed-off-by: meows --- consensus/ethash/ethash.go | 43 +++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/consensus/ethash/ethash.go b/consensus/ethash/ethash.go index e2b0c66457..c6168dae62 100644 --- a/consensus/ethash/ethash.go +++ b/consensus/ethash/ethash.go @@ -340,8 +340,13 @@ func (c *cache) generate(dir string, limit int, lock bool, test bool) { if !isLittleEndian() { endian = ".be" } - path := filepath.Join(dir, fmt.Sprintf("cache-R%d-%x%s", algorithmRevision, seed[:8], endian)) - logger := log.New("epoch", c.epoch) + // The file path naming scheme was changed to include epoch values in the filename, + // which enables a filepath glob with scan to identify out-of-bounds caches and remove them. + // The legacy path declaration is provided below as a comment for reference. + // + // path := filepath.Join(dir, fmt.Sprintf("cache-R%d-%x%s", algorithmRevision, seed[:8], endian)) // LEGACY + path := filepath.Join(dir, fmt.Sprintf("cache-R%d-%d-%x%s", algorithmRevision, c.epoch, seed[:8], endian)) // CURRENT + logger := log.New("epoch", c.epoch, "epochLength", c.epochLength) // We're about to mmap the file, ensure that the mapping is cleaned up when the // cache becomes unused. @@ -370,11 +375,35 @@ func (c *cache) generate(dir string, limit int, lock bool, test bool) { c.cache = make([]uint32, size/4) generateCache(c.cache, c.epoch, c.epochLength, seed) } - // Iterate over all previous instances and delete old ones - for ep := int(c.epoch) - limit; ep >= 0; ep-- { - seed := seedHash(uint64(ep), c.epochLength) - path := filepath.Join(dir, fmt.Sprintf("cache-R%d-%x%s", algorithmRevision, seed[:8], endian)) - os.Remove(path) + + // Iterate over all cache file instances, deleting any out of bounds (where epoch is below lower limit, or above upper limit). + matches, _ := filepath.Glob(filepath.Join(dir, fmt.Sprintf("cache-R%d*", algorithmRevision))) + for _, file := range matches { + var ar int // algorithm revision + var e uint64 // epoch + var s string // seed + if _, err := fmt.Sscanf(filepath.Base(file), "cache-R%d-%d-%s"+endian, &ar, &e, &s); err != nil { + // There is an unrecognized file in this directory. + // See if the name matches the expected pattern of the legacy naming scheme. + if _, err := fmt.Sscanf(filepath.Base(file), "cache-R%d-%s"+endian, &ar, &s); err == nil { + // This file matches the previous generation naming pattern (sans epoch). + if err := os.Remove(file); err != nil { + logger.Error("Failed to remove legacy ethash cache file", "file", file, "err", err) + } else { + logger.Warn("Deleted legacy ethash cache file", "path", file) + } + } else { + // Else the file is unrecognized (unknown name format), leave it alone. + } + continue + } + if e <= c.epoch-uint64(limit) || e > c.epoch+1 { + if err := os.Remove(file); err == nil { + logger.Debug("Deleted ethash cache file", "target.epoch", e, "file", file) + } else { + logger.Error("Failed to delete ethash cache file", "target.epoch", e, "file", file, "err", err) + } + } } }) } From 838824539edf08f5d75e254b78f64c7f2e530257 Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 30 Aug 2022 08:55:11 -0700 Subject: [PATCH 06/15] consensus/ethash: fix log context for dataset generation This function generates the dataset, not the cache. Date: 2022-08-30 08:55:11-07:00 Signed-off-by: meows --- consensus/ethash/algorithm.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/ethash/algorithm.go b/consensus/ethash/algorithm.go index af9f4bc1f2..b79d6ca989 100644 --- a/consensus/ethash/algorithm.go +++ b/consensus/ethash/algorithm.go @@ -303,7 +303,7 @@ func generateDataset(dest []uint32, epoch uint64, epochLength uint64, cache []ui if elapsed > 3*time.Second { logFn = logger.Info } - logFn("Generated ethash verification cache", "epochLength", epochLength, "elapsed", common.PrettyDuration(elapsed)) + logFn("Generated ethash verification dataset", "epochLength", epochLength, "elapsed", common.PrettyDuration(elapsed)) }() // Figure out whether the bytes need to be swapped for the machine From c2a1a73e8d7b9bf98fe0609a219efb0de5ff59a1 Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 30 Aug 2022 09:02:40 -0700 Subject: [PATCH 07/15] consensus/ethash: (lint) SA9003: empty branch (staticcheck) Date: 2022-08-30 09:02:40-07:00 Signed-off-by: meows --- consensus/ethash/ethash.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/consensus/ethash/ethash.go b/consensus/ethash/ethash.go index c6168dae62..e10b7e182e 100644 --- a/consensus/ethash/ethash.go +++ b/consensus/ethash/ethash.go @@ -392,9 +392,8 @@ func (c *cache) generate(dir string, limit int, lock bool, test bool) { } else { logger.Warn("Deleted legacy ethash cache file", "path", file) } - } else { - // Else the file is unrecognized (unknown name format), leave it alone. } + // Else the file is unrecognized (unknown name format), leave it alone. continue } if e <= c.epoch-uint64(limit) || e > c.epoch+1 { From 67e205ea2e0e4e827f6e3ad73f892260e7e2748c Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 30 Aug 2022 09:03:00 -0700 Subject: [PATCH 08/15] consensus/ethash: (lint) unnecessary leading newline (whitespace) Date: 2022-08-30 09:03:00-07:00 Signed-off-by: meows --- consensus/ethash/ethash_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/consensus/ethash/ethash_test.go b/consensus/ethash/ethash_test.go index c5b993b9ae..3eae4baacc 100644 --- a/consensus/ethash/ethash_test.go +++ b/consensus/ethash/ethash_test.go @@ -73,7 +73,6 @@ func TestEthashCaches(t *testing.T) { trialMax := ecip1099Block * uint64(testIterationMultiple) * 2 latestIteratedEpoch := uint64(math.MaxInt64) for n := uint64(0); n < trialMax; n += epochLengthDefault / 300 { - // Calculate the epoch number independently to use for logging and debugging. epochLength := epochLengthDefault if n >= ecip1099Block { From 331d557a6c8cc9ba78cd784fcabbcde9063c39c4 Mon Sep 17 00:00:00 2001 From: meows Date: Wed, 31 Aug 2022 06:59:59 -0700 Subject: [PATCH 09/15] consensus/ethash: remove useless call to cache finalizer This gets called by runtime.SetFinalizer within the cache.generate method. Date: 2022-08-31 06:59:59-07:00 Signed-off-by: meows --- consensus/ethash/ethash_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/consensus/ethash/ethash_test.go b/consensus/ethash/ethash_test.go index 3eae4baacc..f9a527e2bf 100644 --- a/consensus/ethash/ethash_test.go +++ b/consensus/ethash/ethash_test.go @@ -93,7 +93,6 @@ func TestEthashCaches(t *testing.T) { // This is the tested function. c := e.cache(n) - c.finalizer() // Do we get the right epoch length? if n >= ecip1099Block && c.epochLength != epochLengthECIP1099 { From bac80dae1ea98d4acae701077c9f09d78dd406d0 Mon Sep 17 00:00:00 2001 From: meows Date: Wed, 31 Aug 2022 07:03:29 -0700 Subject: [PATCH 10/15] consensus/ethash: simplify syntax for future vs. nextEpoch condition https://github.com/etclabscore/core-geth/pull/499/files#r958753184 Date: 2022-08-31 07:03:29-07:00 Signed-off-by: meows --- consensus/ethash/ethash.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/ethash/ethash.go b/consensus/ethash/ethash.go index e10b7e182e..1c071dfd8d 100644 --- a/consensus/ethash/ethash.go +++ b/consensus/ethash/ethash.go @@ -253,7 +253,7 @@ func (lru *lru) get(epoch uint64, epochLength uint64, ecip1099FBlock *uint64) (i // Update the 'future item' if epoch is larger than previously seen. // Last conditional clause ('lru.future > nextEpoch') handles the ECIP1099 case where // the next epoch is expected to be LESSER THAN that of the previous state's future epoch number. - if epoch < maxEpoch-1 && (lru.future < nextEpoch || lru.future > nextEpoch) { + if epoch < maxEpoch-1 && lru.future != nextEpoch { log.Trace("Requiring new future ethash "+lru.what, "epoch", nextEpoch) future = lru.new(nextEpoch, nextEpochLength) lru.future = nextEpoch From 545a964e57fa490dcd8aa973e5a2ecc268625fa7 Mon Sep 17 00:00:00 2001 From: meows Date: Wed, 31 Aug 2022 07:06:27 -0700 Subject: [PATCH 11/15] consensus/ethash: dont be so generous with 1099 epoch activation With a generous activation (>=), 1099 can be activated but until the next epoch rolls around it won't actually be activated, eg. ecip1099Block=3_000_042 wont activate until 3_030_000... which is unpredictable. Better to conform to and enforce stricter expectations. Date: 2022-08-31 07:06:27-07:00 Signed-off-by: meows --- consensus/ethash/ethash.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/consensus/ethash/ethash.go b/consensus/ethash/ethash.go index 1c071dfd8d..63e9ae6199 100644 --- a/consensus/ethash/ethash.go +++ b/consensus/ethash/ethash.go @@ -244,7 +244,10 @@ func (lru *lru) get(epoch uint64, epochLength uint64, ecip1099FBlock *uint64) (i var nextEpochLength = epochLength if ecip1099FBlock != nil { nextEpochBlock := nextEpoch * epochLength - if nextEpochBlock >= *ecip1099FBlock && epochLength == epochLengthDefault { + // Note that == demands that the ECIP1099 activation block is situated + // at the beginning of an epoch. + // https://github.com/ethereumclassic/ECIPs/blob/master/_specs/ecip-1099.md#implementation + if nextEpochBlock == *ecip1099FBlock && epochLength == epochLengthDefault { nextEpoch = nextEpoch / 2 nextEpochLength = epochLengthECIP1099 } From b9d5a7d911c75ab3abd55e1f1430e61f6d67269b Mon Sep 17 00:00:00 2001 From: meows Date: Wed, 31 Aug 2022 07:26:14 -0700 Subject: [PATCH 12/15] consensus/ethash: add test for ECIP1099: unique seed hashes https://github.com/ethereumclassic/ECIPs/blob/master/_specs/ecip-1099.md#specification tells us that seed hashes wont overlap. This tests shows that indeed, for the range tested, they don't collide. Date: 2022-08-31 07:26:14-07:00 Signed-off-by: meows --- consensus/ethash/ethash_test.go | 48 +++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/consensus/ethash/ethash_test.go b/consensus/ethash/ethash_test.go index f9a527e2bf..9ab7091c58 100644 --- a/consensus/ethash/ethash_test.go +++ b/consensus/ethash/ethash_test.go @@ -38,6 +38,54 @@ func verboseLogging() { log.Root().SetHandler(glogger) } +func TestEthashECIP1099UniqueSeedHashes(t *testing.T) { + // Use some "big" arbitrary multiple to make sure that simulate real life adequately. + testIterationMultiple := 6 + ecip1099Block := uint64(epochLengthDefault * 3 * testIterationMultiple) + + // Define a table to hold our seed hashes. + // We'll reference these to see if there are any dupes. + type seedHashT struct { + epoch uint64 + epochLength uint64 + } + seedHashes := make(map[string]seedHashT) + + trialMax := ecip1099Block * uint64(testIterationMultiple) * 42 + latestIteratedEpoch := uint64(math.MaxInt64) + for n := uint64(0); n < trialMax; n += epochLengthDefault / 2 { + // Calculate the epoch number independently to use for logging and debugging. + epochLength := epochLengthDefault + if n >= ecip1099Block { + epochLength = epochLengthECIP1099 + } + ep := calcEpoch(n, uint64(epochLength)) + epl := calcEpochLength(n, &ecip1099Block) + + if ep != latestIteratedEpoch { + latestIteratedEpoch = ep + + seed := seedHash(ep, epl) + seedHex := hexutil.Encode(seed[:])[2:] + if v, ok := seedHashes[seedHex]; ok { + t.Logf("block=%d epoch=%d epoch.len=%d ECIP1099=/%d (%0.1f%%) RANGE=/%d (%0.1f%%)", + n, + ep, epl, + ecip1099Block, float64(n)/float64(ecip1099Block)*100, + trialMax, float64(n)/float64(trialMax)*100, + ) + t.Errorf("duplicate seed hash: %s a.epoch=%d a.epochLength=%d b.epoch=%d b.epochLength=%d", + seedHex, v.epoch, v.epochLength, ep, epl) + } else { + seedHashes[seedHex] = seedHashT{ + epoch: ep, + epochLength: epl, + } + } + } + } +} + func TestEthashCaches(t *testing.T) { verboseLogging() From a4415e74d3623bb2bd85c6447a42f25dd91c5e80 Mon Sep 17 00:00:00 2001 From: meows Date: Wed, 31 Aug 2022 08:52:28 -0700 Subject: [PATCH 13/15] consensus/ethash: test ensures reasonable number of cache files persisted Date: 2022-08-31 08:52:28-07:00 Signed-off-by: meows --- consensus/ethash/ethash_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/consensus/ethash/ethash_test.go b/consensus/ethash/ethash_test.go index 9ab7091c58..a8c180ef56 100644 --- a/consensus/ethash/ethash_test.go +++ b/consensus/ethash/ethash_test.go @@ -143,7 +143,7 @@ func TestEthashCaches(t *testing.T) { c := e.cache(n) // Do we get the right epoch length? - if n >= ecip1099Block && c.epochLength != epochLengthECIP1099 { + if c.epochLength != epl { // Give the future epoch routine a chance to finish. time.Sleep(1 * time.Second) @@ -164,6 +164,16 @@ func TestEthashCaches(t *testing.T) { t.Fatalf("Unexpected epoch length: %d", c.epochLength) } + + entries, _ := os.ReadDir(conf.CacheDir) + // We add +1 to CachesOnDisk because the future epoch cache is also created and can still + // be in-progress generating as a goroutine. + if len(entries) > conf.CachesOnDisk+1 { + for _, entry := range entries { + t.Logf(` - %s`, entry.Name()) + } + t.Fatalf("Too many cache files: %d", len(entries)) + } } } From 836792fb0d2aa5a88aa591f37deff7265ee91db5 Mon Sep 17 00:00:00 2001 From: meows Date: Wed, 31 Aug 2022 08:53:17 -0700 Subject: [PATCH 14/15] consensus/ethash: add test ensuring cache file eviction/allowance operates as expected Date: 2022-08-31 08:53:17-07:00 Signed-off-by: meows --- consensus/ethash/ethash_test.go | 77 +++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/consensus/ethash/ethash_test.go b/consensus/ethash/ethash_test.go index a8c180ef56..76b3a97d38 100644 --- a/consensus/ethash/ethash_test.go +++ b/consensus/ethash/ethash_test.go @@ -17,6 +17,7 @@ package ethash import ( + "fmt" "math" "math/big" "math/rand" @@ -177,6 +178,82 @@ func TestEthashCaches(t *testing.T) { } } +func TestEthashCacheFileEviction(t *testing.T) { + verboseLogging() + + // Make a copy of the default config. + conf := Config{ + CacheDir: filepath.Join(os.TempDir(), "ethash-cache-test-cachedir"), + CachesInMem: 2, + CachesOnDisk: 3, + CachesLockMmap: false, + DatasetsInMem: 1, + DatasetsOnDisk: 2, + DatasetsLockMmap: false, + DatasetDir: filepath.Join(os.TempDir(), "ethash-cache-test-datadir"), + PowMode: ModeNormal, + } + + // Clean up ahead of ourselves. + os.RemoveAll(conf.CacheDir) + os.RemoveAll(conf.DatasetDir) + + // And after ourselves. + defer os.RemoveAll(conf.CacheDir) + defer os.RemoveAll(conf.DatasetDir) + + // Use some "big" arbitrary multiple to make sure that simulate real life adequately. + testIterationMultiple := 6 + ecip1099Block := uint64(epochLengthDefault * conf.CachesInMem * testIterationMultiple) + conf.ECIP1099Block = &ecip1099Block + + // Construct our Ethash + e := New(conf, nil, false) + + bn := uint64(12_345_678) + + el := calcEpochLength(bn, conf.ECIP1099Block) + ep := calcEpoch(bn, el) + seed := seedHash(ep, el) + + os.MkdirAll(conf.CacheDir, 0700) + + // Create a legacy cache file. + // This should get removed. + legacyCacheFileBasePath := fmt.Sprintf("cache-R%d-%x", algorithmRevision, seed[:8]) + legacyCacheFilePath := filepath.Join(conf.CacheDir, legacyCacheFileBasePath) + if err := os.WriteFile(legacyCacheFilePath, []byte{}, 0644); err != nil { + t.Fatal(err) + } + // Create an unknown file in the cache dir. + // This should not get removed. + unknownCacheFilePath := filepath.Join(conf.CacheDir, "unexpected-file") + if err := os.WriteFile(unknownCacheFilePath, []byte{}, 0644); err != nil { + t.Fatal(err) + } + + // Print entries before ethash.cache method called. + entries, _ := os.ReadDir(conf.CacheDir) + for _, entry := range entries { + t.Logf(` - %s`, entry.Name()) + } + + // Call the cache method, which will clean up the cache dir after generating the cache. + e.cache(bn) + + entries, _ = os.ReadDir(conf.CacheDir) + for _, entry := range entries { + t.Logf(` - %s`, entry.Name()) + } + + if _, err := os.Stat(legacyCacheFilePath); !os.IsNotExist(err) { + t.Fatalf("legacy cache file %s not removed", legacyCacheFilePath) + } + if _, err := os.Stat(unknownCacheFilePath); err != nil { + t.Fatalf("unknown cache file %s removed", unknownCacheFilePath) + } +} + // Tests caches get sets correct future func TestCachesGet(t *testing.T) { ethashA := NewTester(nil, false) From 310ca59020c0f7887ef0952ddbefc117e8eab6da Mon Sep 17 00:00:00 2001 From: meows Date: Wed, 31 Aug 2022 19:55:29 -0700 Subject: [PATCH 15/15] consensus/ethash: apply new cache file naming pattern to full datasets too Date: 2022-08-31 19:55:29-07:00 Signed-off-by: meows --- consensus/ethash/ethash.go | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/consensus/ethash/ethash.go b/consensus/ethash/ethash.go index 63e9ae6199..2327f7c990 100644 --- a/consensus/ethash/ethash.go +++ b/consensus/ethash/ethash.go @@ -463,7 +463,7 @@ func (d *dataset) generate(dir string, limit int, lock bool, test bool) { if !isLittleEndian() { endian = ".be" } - path := filepath.Join(dir, fmt.Sprintf("full-R%d-%x%s", algorithmRevision, seed[:8], endian)) + path := filepath.Join(dir, fmt.Sprintf("full-R%d-%d-%x%s", algorithmRevision, d.epoch, seed[:8], endian)) logger := log.New("epoch", d.epoch) // We're about to mmap the file, ensure that the mapping is cleaned up when the @@ -499,11 +499,34 @@ func (d *dataset) generate(dir string, limit int, lock bool, test bool) { d.dataset = make([]uint32, dsize/4) generateDataset(d.dataset, d.epoch, d.epochLength, cache) } - // Iterate over all previous instances and delete old ones - for ep := int(d.epoch) - limit; ep >= 0; ep-- { - seed := seedHash(uint64(ep), d.epochLength) - path := filepath.Join(dir, fmt.Sprintf("full-R%d-%x%s", algorithmRevision, seed[:8], endian)) - os.Remove(path) + + // Iterate over all full file instances, deleting any out of bounds (where epoch is below lower limit, or above upper limit). + matches, _ := filepath.Glob(filepath.Join(dir, fmt.Sprintf("full-R%d*", algorithmRevision))) + for _, file := range matches { + var ar int // algorithm revision + var e uint64 // epoch + var s string // seed + if _, err := fmt.Sscanf(filepath.Base(file), "full-R%d-%d-%s"+endian, &ar, &e, &s); err != nil { + // There is an unrecognized file in this directory. + // See if the name matches the expected pattern of the legacy naming scheme. + if _, err := fmt.Sscanf(filepath.Base(file), "full-R%d-%s"+endian, &ar, &s); err == nil { + // This file matches the previous generation naming pattern (sans epoch). + if err := os.Remove(file); err != nil { + logger.Error("Failed to remove legacy ethash full file", "file", file, "err", err) + } else { + logger.Warn("Deleted legacy ethash full file", "path", file) + } + } + // Else the file is unrecognized (unknown name format), leave it alone. + continue + } + if e <= d.epoch-uint64(limit) || e > d.epoch+1 { + if err := os.Remove(file); err == nil { + logger.Debug("Deleted ethash full file", "target.epoch", e, "file", file) + } else { + logger.Error("Failed to delete ethash full file", "target.epoch", e, "file", file, "err", err) + } + } } }) }