accounts: cache key addresses #2284
Conversation
fjl
force-pushed
the
accounts-addr-cache
branch
2 times, most recently
from
53a269e
to
c06922e
Compare
Current coverage is
|
|
This solution seems unnecessary complex. How about simply tracking a map of addresses to key files in a metadata file? Then getting all addresses is a single file read, which will be cached automatically by the OS if called often. This would also resolve the privacy issue of having addresses in (exported) key file (names), though it would require a user to import a key using a geth command, which would need to decrypt the file in order to get the address to update the map. IMO that would be a good thing though, as it prevents import of files the user has forgotten the password to, and also avoids users from having to manually place files into a internal directory. |
fjl
force-pushed
the
accounts-addr-cache
branch
3 times, most recently
from
be07070
to
6d1ef71
Compare
fjl
force-pushed
the
accounts-addr-cache
branch
from
632ed91
to
612c9a0
Compare
| } | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Wouldn't it be better to check the error first and then zero the key? If not, could you add a comment as to why not?
There was a problem hiding this comment.
The call to GetKey verifies that the password is correct. We don't actually need the key, so it should be zeroed always even if loading it produces an unrelated error.
There was a problem hiding this comment.
Shouldn't GetKey ensure that if a password is wrong, it does not give you back the key?
There was a problem hiding this comment.
Yes, but I think it is good style not to assume anything about the relationship between key and err if the intention is to zero it in all possible cases.
|
I have thought about, and don't understand, the privacy concern about keeping the address recorded in the key file. As of this PR, keys can have any file name but the address must be present in the JSON. In order for {sign,unlock}-by-address to work, the addresses need to be tracked somehow. |
fjl
force-pushed
the
accounts-addr-cache
branch
6 times, most recently
from
f47f876
to
7388a48
Compare
This message will be emitted when ethereum/go-ethereum#2284 is merged.
This message will be emitted when ethereum/go-ethereum#2284 is merged.
|
@fjl Merge conflict |
fjl
force-pushed
the
accounts-addr-cache
branch
from
48087e4
to
faa6051
Compare
|
fixed |
|
@fjl Test compile error :P |
Unlocking the accounts in the test doesn't help with anything.
fjl
force-pushed
the
accounts-addr-cache
branch
2 times, most recently
from
f686a2f
to
3ad8d6e
Compare
These changes make prompting behave consistently on all platforms: * The input buffer is now global. Buffering was previously set up for each prompt, which can cause weird behaviour, e.g. when running "geth account update <input.txt" where input.txt contains three lines. In this case, the first password prompt would fill up the buffer with all lines and then use only the first one. * Print the "unsupported terminal" warning only once. Now that stdin prompting has global state, we can use it to track the warning there. * Work around small liner issues, particularly on Windows. Prompting didn't work under most of the third-party terminal emulators on Windows because liner assumes line editing is always available.
The account management API was originally implemented as a thin layer around crypto.KeyStore, on the grounds that several kinds of key stores would be implemented later on. It turns out that this won't happen so KeyStore is a superflous abstraction. In this commit crypto.KeyStore and everything related to it moves to package accounts and is unexported.
- Manager.Accounts no longer returns an error. - Manager methods take Account instead of common.Address. - All uses of Account with unkeyed fields are converted.
In order to avoid disk thrashing for Accounts and HasAccount, address->key file mappings are now cached in memory. This makes it no longer necessary to keep the key address in the file name. The address of each key is derived from file content instead. There are minor user-visible changes: - "geth account list" now reports key file paths alongside the address. - If multiple keys are present for an address, unlocking by address is not possible. Users are directed to remove the duplicate files instead. Unlocking by index is still possible. - Key files are overwritten written in place when updating the password.
Text files created on Windows typically have \r\n line endings. Trim them when reading password files.
- Sign takes common.Address, not Account - Import/Export methods work with encrypted JSON keys
fjl
force-pushed
the
accounts-addr-cache
branch
from
3ad8d6e
to
6498df7
Compare
|
fixed again |
| return os.Rename(f.Name(), file) | ||
| } | ||
|
|
||
| // keyFileName implements the naming convention for keyfiles: |
There was a problem hiding this comment.
@fjl hello! Where can I'm read this convention?
In this PR:
AccountsandHasAccountdo not hit diskA few minor issues remain before this can be considered ready:
On kqueue platforms (OS X, FreeBSD...), the fsnotify package polls every 100ms. I submitted a fix upstream (kqueue: wait without timeout fsnotify/fsnotify#124).On kqueue platforms, fsnotify uses lots of file descriptors (3 + one for each key) to provide per-file events. We don't need this granularity, but there is no way to prevent it.Fixes #2100
Fixes #1785