qa: Cache only chain and wallet for regtest datadir #12638

MarcoFalke commented at 6:47 PM on March 7, 2018: member

mempool.dat should be empty and I don't see a need to copy it around when restoring from the cache.

jamesob commented at 7:00 PM on March 7, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12638/commits/fa533b113944c9b5d2bf9358393448b60204ab57

laanwj commented at 7:01 PM on March 7, 2018: member

utACK, though the opposite approach, listing the files and directories to be cached instead of the ones not to be cached, might lead to less surprises in the future.

in test/functional/test_framework/test_framework.py:438 in fa533b1139 outdated

 437 | -                os.remove(log_filename(self.options.cachedir, i, "fee_estimates.dat"))
 438 | +                os.remove(os.path.join(get_datadir_path(self.options.cachedir, i), "regtest", "debug.log"))
 439 | +                os.remove(os.path.join(get_datadir_path(self.options.cachedir, i), "regtest", "mempool.dat"))
 440 | +                os.remove(os.path.join(get_datadir_path(self.options.cachedir, i), "regtest", "wallets/db.log"))
 441 | +                os.remove(os.path.join(get_datadir_path(self.options.cachedir, i), "regtest", "peers.dat"))
 442 | +                os.remove(os.path.join(get_datadir_path(self.options.cachedir, i), "regtest", "fee_estimates.dat"))

jamesob commented at 7:02 PM on March 7, 2018:

Would be nice to avoid so much duplication, e.g.

def rm_cache_path(filename):
    os.remove(os.path.join(get_datadir_path(self.options.cachedir, i), "regtest", filename))

but this is fine as-is.

fanquake added the label Tests on Mar 7, 2018

promag commented at 7:19 PM on March 7, 2018: member

utACK fa533b1.

MarcoFalke force-pushed on Mar 7, 2018

MarcoFalke commented at 7:23 PM on March 7, 2018: member

Addressed @jamesob and @laanwj feedback

MarcoFalke renamed this:
~~qa: Remove mempool.dat from cached regtest datadir~~
qa: Cache only chain and wallet for regtest datadir
on Mar 7, 2018

laanwj commented at 7:26 PM on March 7, 2018: member

Thanks, yes, I like this much better. re-utACK

in test/functional/test_framework/test_framework.py:434 in fab280a665 outdated

 429 | @@ -430,11 +430,12 @@ def _initialize_chain(self):
 430 |              self.stop_nodes()
 431 |              self.nodes = []
 432 |              self.disable_mocktime()
 433 | +            def cache_path(*paths):
 434 | +                return os.path.join(get_datadir_path(self.options.cachedir, i), "regtest", *paths)

jamesob commented at 7:26 PM on March 7, 2018:

i won't be in scope yet; have to declare this in the loop.

laanwj commented at 7:28 PM on March 7, 2018:

I don't think that's necessary. Python will add the symbol i to the scope of the function, not the for block. Though it might be clearer to just pass i in as a parameter.

jamesob commented at 7:30 PM on March 7, 2018:

Oh, @laanwj is right. Still probably best to either parameterize or move this definition as-is under the for for clarity.

laanwj commented at 7:42 PM on March 7, 2018:

Not that the performance overhead is relevant in this case, but I think defining a function inside a loop is generally frowned upon. AFAIK essentially it's instantiating an object every time.

promag commented at 7:40 PM on March 7, 2018: member

Nit, since you are importing get_datadir_path, add a commit replacing

os.path.join(self.options.cachedir, "node" + str(i))

with

get_datadir_path(self.options.cachedir, i)

qa: Cache only chain and wallet for regtest datadir fa2310572f

MarcoFalke force-pushed on Mar 7, 2018

jamesob commented at 9:15 PM on March 7, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12638/commits/fa2310572f4cfcd3322409ce7e37dde155fc4bc9

conscott commented at 3:20 AM on March 14, 2018: contributor

Tested ACK fa2310572f4cfcd3322409ce7e37dde155fc4bc9

Think travis just needs a bump.

laanwj commented at 12:13 PM on March 14, 2018: member

I've bumped travis twice, but it still fails.

Edit:

/home/travis/.travis/job_stages: line 57: depends/arm-linux-gnueabihf/native/bin/ccache: No such file or directory

I'll try nuking the cache (for this PR).

$ travis cache -d -b PR.12638

laanwj commented at 12:44 PM on March 14, 2018: member

Still no luck, I think you need to re-push.

MarcoFalke commented at 1:16 PM on March 14, 2018: member

You can safely ignore the failing signal from Travis, it is a bug on their end. I am not going to invalidate review for that

On Mar 14, 2018 13:45, "Wladimir J. van der Laan" notifications@github.com wrote:

Still no luck, I think you need to re-push.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/12638#issuecomment-373007651, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv-J9M3NmsrlhDwbhtZwCidWfiwY8ks5teRDPgaJpZM4Sg9zp .

laanwj commented at 1:18 PM on March 14, 2018: member

You can safely ignore the failing signal from Travis, it is a bug on their end. I am not going to invalidate review for that

I find that difficult, without travis passing on a PR you can't guarantee that it won't break the build. But anyhow, if you're so sure, we'll just merge...

laanwj merged this on Mar 14, 2018

laanwj closed this on Mar 14, 2018

laanwj referenced this in commit ce6ffe196e on Mar 14, 2018

MarcoFalke deleted the branch on Mar 18, 2018

MarcoFalke referenced this in commit 010ba769aa on Apr 20, 2018

MarcoFalke referenced this in commit 0a76ed232a on Apr 20, 2018

HashUnlimited referenced this in commit 81b6a8daf8 on May 13, 2018

ccebrecos referenced this in commit 2dce2c0fc8 on Sep 14, 2018

PastaPastaPasta referenced this in commit a1f52784e3 on Mar 14, 2020

PastaPastaPasta referenced this in commit ff346d607a on Mar 19, 2020

PastaPastaPasta referenced this in commit 71135422ed on Mar 21, 2020

PastaPastaPasta referenced this in commit 57d82cf722 on Mar 24, 2020

ckti referenced this in commit 41465b0ca6 on Mar 28, 2021

gades referenced this in commit 7b286d1d23 on Jun 30, 2021

MarcoFalke locked this on Sep 8, 2021