WIP accounts/keystore: don't scan all files, only file affected by event by holiman · Pull Request #15580 · ethereum/go-ethereum

holiman · 2017-11-29T09:36:17Z

In the current codebase, whenever the watcher receives notifications about changes in the keystore directory, it triggers a rescan of files in the directory. This scan uses mtime to see what files are changed, which does not seem to be all to stable (see #15410 and #15498).

This PR instead

Acts on every event
Uses the path from the event, but ignores the other particulars.
Checks the path against already known files/accounts, to determine if the file was modified, changed, or deleted (or nothing)
Updates the cache accordingly.
Uses a 'canonicalized' path for temporary keystores: earlier, the path returned from osx when creating tempfiles contained a soft link. That link is now resolved, to ensure that the path returned from the fs notification is identical to the one used by the keystore.

See https://godoc.org/github.com/rjeczalik/notify for info about the notify library.
The PR fixes #15410 (comment) , and reverts #15498, since those changes are seemingly no longer needed.

holiman · 2017-11-29T13:38:21Z

Failures:

?   	github.com/ethereum/go-ethereum/accounts/abi/bind/backends	[no test files]

--- FAIL: TestWalletNotifications (1.80s)

	keystore_test.go:344: wallet list doesn't match required accounts: have 707, want 540

	keystore_test.go:372: can't find event with Kind=2 for 1f6bdcbda3e60e9e2b4ff95fe4f4b0058fd3604f

--- FAIL: TestWatchNewFile (7.40s)

	account_cache_test.go:91: got ([]accounts.Account) (len=3 cap=3) {

		 (accounts.Account) {

		  Address: (common.Address) (len=20 cap=20) 0x7EF5A6135f1FD6a02593eEdC869c6D41D934aef8,

		  URL: (accounts.URL) keystore:///private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/UTC--2016-03-22T12-57-55.920751759Z--7ef5a6135f1fd6a02593eedc869c6d41d934aef8

		 },

		 (accounts.Account) {

		  Address: (common.Address) (len=20 cap=20) 0xf466859eAD1932D743d622CB74FC058882E8648A,

		  URL: (accounts.URL) keystore:///private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/aaa

		 },

		 (accounts.Account) {

		  Address: (common.Address) (len=20 cap=20) 0x289d485D9771714CCe91D3393D764E1311907ACc,

		  URL: (accounts.URL) keystore:///private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/zzz

		 }

		}

		, want ([]accounts.Account) (len=3 cap=3) {

		 (accounts.Account) {

		  Address: (common.Address) (len=20 cap=20) 0x7EF5A6135f1FD6a02593eEdC869c6D41D934aef8,

		  URL: (accounts.URL) keystore:///var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/UTC--2016-03-22T12-57-55.920751759Z--7ef5a6135f1fd6a02593eedc869c6d41d934aef8

		 },

		 (accounts.Account) {

		  Address: (common.Address) (len=20 cap=20) 0xf466859eAD1932D743d622CB74FC058882E8648A,

		  URL: (accounts.URL) keystore:///var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/aaa

		 },

		 (accounts.Account) {

		  Address: (common.Address) (len=20 cap=20) 0x289d485D9771714CCe91D3393D764E1311907ACc,

		  URL: (accounts.URL) keystore:///var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/zzz

		 }

		}

--- FAIL: TestUpdatedKeyfileContents (17.29s)

	account_cache_test.go:363: First replacement failed

	account_cache_test.go:364: 

		got  [{[244 102 133 158 173 25 50 215 67 214 34 203 116 252 5 136 130 232 100 138] keystore:///private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-watch-test-5729-4925858546685333139/aaa} {[126 245 166 19 95 31 214 160 37 147 238 220 134 156 109 65 217 52 174 248] keystore:///var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-watch-test-5729-4925858546685333139/aaa}]

		want [{[244 102 133 158 173 25 50 215 67 214 34 203 116 252 5 136 130 232 100 138] keystore:///var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-watch-test-5729-4925858546685333139/aaa}]

FAIL

I wonder if the 1.9.1 travis-builder, which ran for 26 min 55 sec, is just damn slow, and the changes are handled to slowly for the watcher to update the cache in the allotted time.

holiman · 2017-11-29T13:48:55Z

Hm, I don't see what's wrong with this, looks correct to me:

	account_cache_test.go:91: got ([]accounts.Account) (len=3 cap=3) {
		 (accounts.Account) {
		  Address: (common.Address) (len=20 cap=20) 0x7EF5A6135f1FD6a02593eEdC869c6D41D934aef8,
		  URL: (accounts.URL) keystore:///private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/UTC--2016-03-22T12-57-55.920751759Z--7ef5a6135f1fd6a02593eedc869c6d41d934aef8
		 },
		 (accounts.Account) {
		  Address: (common.Address) (len=20 cap=20) 0xf466859eAD1932D743d622CB74FC058882E8648A,
		  URL: (accounts.URL) keystore:///private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/aaa
		 },
		 (accounts.Account) {
		  Address: (common.Address) (len=20 cap=20) 0x289d485D9771714CCe91D3393D764E1311907ACc,
		  URL: (accounts.URL) keystore:///private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/zzz
		 }
		}
		, want ([]accounts.Account) (len=3 cap=3) {
		 (accounts.Account) {
		  Address: (common.Address) (len=20 cap=20) 0x7EF5A6135f1FD6a02593eEdC869c6D41D934aef8,
		  URL: (accounts.URL) keystore:///var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/UTC--2016-03-22T12-57-55.920751759Z--7ef5a6135f1fd6a02593eedc869c6d41d934aef8
		 },
		 (accounts.Account) {
		  Address: (common.Address) (len=20 cap=20) 0xf466859eAD1932D743d622CB74FC058882E8648A,
		  URL: (accounts.URL) keystore:///var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/aaa
		 },
		 (accounts.Account) {
		  Address: (common.Address) (len=20 cap=20) 0x289d485D9771714CCe91D3393D764E1311907ACc,
		  URL: (accounts.URL) keystore:///var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/zzz
		 }
		}

holiman · 2017-11-29T13:51:51Z

Oh:
keystore:///private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/zzz versus keystore:///var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-test737915495/zzz

The path returned from the notify-library is absolute on the host, whereas the want - path is absolute on the container. Or something like that. Weird.

holiman · 2017-11-29T14:00:43Z

Same error on TestUpdatedKeyfileContents! The actual data is correct:

	got  [{[244 102 133 158 173 25 50 215 67 214 34 203 116 252 5 136 130 232 100 138] 
	want [{[244 102 133 158 173 25 50 215 67 214 34 203 116 252 5 136 130 232 100 138]

However, the got part also did not remove the old one: keystore:///var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/eth-keystore-watch-test-5729-4925858546685333139/aaa. This totally makes sense, since the new logic detected a change in /private/var.., and didn't bother flagging the one at /var/... as deleted.

So, afaict, this actually does work, but there's some issue with resolving the paths properly.

holiman · 2017-11-30T09:01:42Z

This is the problem:https://stackoverflow.com/questions/45122459/docker-mounts-denied-the-paths-are-not-shared-from-os-x-and-are-not-known/45123074#45123074

/var in macOS is a symbolic link into /private.

The go-test is using

	dir := filepath.Join(os.TempDir(), fmt.Sprintf("eth-keystore-watch-test-%d-%d", os.Getpid(), rand.Int()))

so os.TempDir on osx appears to return /var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/. I've now added a fix which canonicalises the path returned from os.Tempdir(), using EvalSymlinks. Let's see if it works.

holiman · 2017-11-30T12:22:39Z

The PR seems to work, the remaining travis-failure does not seem to be related (https://travis-ci.org/ethereum/go-ethereum/jobs/309404326)

fjl · 2017-12-06T15:25:01Z

+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(tmpdir, fmt.Sprintf("eth-keystore-watch-test-%d-%d", os.Getpid(), rand.Int())), nil


Please move the call to rand.Seed into func init() { ... }. You can also remove other seed calls further down in the file.

fjl · 2017-12-06T15:25:01Z

-	creates, deletes, updates, err := ac.fileC.scan(ac.keydir)
+// readAccount is a helper-function to read an encrypted keyfile
+func readAccount(path string, buf *bufio.Reader) *accounts.Account {
+


Please remove blank lines at the beginning and end of functions.

fjl · 2017-12-21T09:44:37Z

I'm fine with how the code looks now, but tests are still flaking.

holiman · 2017-12-21T10:05:21Z

Sigh, you're right. The appveyor one is a swarm-test going OOM, but the travis test is exactly the type of error that this PR aims to solve. Let's not merge this at the moment, I'll see if I have some time to fix it up perhaps after christmas.

fjl · 2018-05-29T15:53:39Z

The only question is: which Christmas?

accounts/keystore: don't scan all files, only file affected by event

abd14b6

accounts/keystore: canonicalize path to temp directory

eaa880a

gbotrel mentioned this pull request Nov 30, 2017

accounts/keystore: fix cache update and TestUpdatedKeyfileContents fa… #15410

Closed

accounts/keystore: reverted ethereum#15498, added docs

b77771c

holiman changed the title ~~WIP:accounts/keystore: don't scan all files, only file affected by event~~ accounts/keystore: don't scan all files, only file affected by event Dec 1, 2017

holiman requested review from fjl and karalabe December 1, 2017 10:22

fjl requested changes Dec 6, 2017

View reviewed changes

holiman and others added 2 commits December 13, 2017 14:56

accounts/keystore: address review concerns

7945640

accounts/keystore: fix formatting nits

e798e8f

holiman changed the title ~~accounts/keystore: don't scan all files, only file affected by event~~ WIP accounts/keystore: don't scan all files, only file affected by event Dec 21, 2017

holiman added the help wanted label Dec 21, 2017

fjl closed this May 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP accounts/keystore: don't scan all files, only file affected by event#15580

WIP accounts/keystore: don't scan all files, only file affected by event#15580
holiman wants to merge 5 commits into
ethereum:masterfrom
holiman:account_scan_selectively

holiman commented Nov 29, 2017 •

edited

Loading

Uh oh!

holiman commented Nov 29, 2017

Uh oh!

holiman commented Nov 29, 2017

Uh oh!

holiman commented Nov 29, 2017 •

edited

Loading

Uh oh!

holiman commented Nov 29, 2017

Uh oh!

holiman commented Nov 30, 2017

Uh oh!

holiman commented Nov 30, 2017

Uh oh!

fjl Dec 6, 2017

Uh oh!

fjl Dec 6, 2017

Uh oh!

fjl commented Dec 21, 2017

Uh oh!

holiman commented Dec 21, 2017

Uh oh!

fjl commented May 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

holiman commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

holiman commented Nov 29, 2017

Uh oh!

holiman commented Nov 29, 2017

Uh oh!

holiman commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

holiman commented Nov 29, 2017

Uh oh!

holiman commented Nov 30, 2017

Uh oh!

holiman commented Nov 30, 2017

Uh oh!

fjl Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

fjl Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

fjl commented Dec 21, 2017

Uh oh!

holiman commented Dec 21, 2017

Uh oh!

fjl commented May 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

holiman commented Nov 29, 2017 •

edited

Loading

holiman commented Nov 29, 2017 •

edited

Loading