ethereum / go-ethereum

Go implementation of the Ethereum protocol
https://geth.ethereum.org
GNU Lesser General Public License v3.0
47.49k stars 20.1k forks source link

accounts not being created in the correct order in the array. #807

Closed alexvandesande closed 9 years ago

alexvandesande commented 9 years ago

Here's a nice snippet of code for checking all your accounts, in order:

function checkAllBalances() { 
    var i =0; 
    eth.accounts.forEach( function(e){
        console.log("\tAccount["+i+"]: \t" +  e + " \tbalance: \t" + web3.fromWei(eth.getBalance(e), "ether") + " ether"); 
        i++; 
    })
}; 

checkAllBalances();

Check all your balances and note their order. Now do a

admin.newAccount()

and run a

checkAllBalances();

Ideally, the newest account should be added in the end of the array. Instead its seemingly added at random, some times in be [0] slot, sometimes in the last slot, sometimes somewhere in the middle. It's not a but in the checkBalances code, you can check each manually by doing a eth.accounts[x].

Currently my accounts were created in this order:

index      order created
  0                 4th
  1                 1st
  2                 3rd
  3                 2nd
  4                 5th
tgerring commented 9 years ago

They're being listed in alphabetical order which makes me think this is a result of how they're displayed rather than how they're created.

> checkAllBalances();
    Account[0]:     0xd1ade25ccd3d550a7eb532ac759cac7be09c2719  balance:    25.11095 ether
    Account[1]:     0xf4dd5c3794f1fd0cdc0327a83aa472609c806e99  balance:    6 ether
> admin.newAccount()
The new account will be encrypted with a passphrase.
Please enter a passphrase now.
Passphrase: 
Repeat Passphrase: 
'0xda65665fc30803cb1fb7e6d86691e20b1826dee0'
> checkAllBalances();
    Account[0]:     0xd1ade25ccd3d550a7eb532ac759cac7be09c2719  balance:    25.11095 ether
    Account[1]:     0xda65665fc30803cb1fb7e6d86691e20b1826dee0  balance:    0 ether
    Account[2]:     0xf4dd5c3794f1fd0cdc0327a83aa472609c806e99  balance:    6 ether
> admin.newAccount()
The new account will be encrypted with a passphrase.
Please enter a passphrase now.
Passphrase: 
Repeat Passphrase: 
'0xe470b1a7d2c9c5c6f03bbaa8fa20db6d404a0c32'
> checkAllBalances();
    Account[0]:     0xd1ade25ccd3d550a7eb532ac759cac7be09c2719  balance:    25.11095 ether
    Account[1]:     0xda65665fc30803cb1fb7e6d86691e20b1826dee0  balance:    0 ether
    Account[2]:     0xe470b1a7d2c9c5c6f03bbaa8fa20db6d404a0c32  balance:    0 ether
    Account[3]:     0xf4dd5c3794f1fd0cdc0327a83aa472609c806e99  balance:    6 ether
tgerring commented 9 years ago

@obscuren Does the account keystore have a natural alphanum ordering on output or are we maintaining any sort of index?

obscuren commented 9 years ago

The key stores use GetKeyAddresses (https://github.com/ethereum/go-ethereum/blob/develop/crypto/key_store_plain.go#L115) which uses ioutil.ReadDir. Investigating a little shows it uses sort.Sort(byName(list)) https://golang.org/src/io/ioutil/ioutil.go?s=3208:3259#L90

zelig commented 9 years ago

I've been thinking about this and give my 2c

Currently the ordering is alphanumeric and the notion of primary account is defined as the first. This is problematic because the it should not change, but by creating an account that will come before alphabetically, the primary account changes. This is clearly bad.

Some suggested before that the keyfiles should be sorted by creation time to give the indexing. This might make sense for locally created accounts or accounts created by importing a wallet or private key, since they create a new file. But also problematic: we want the individual key files to be portable, this is not reliable. Therefore there is no way to have portability and fixed indexing purely based on file properties. Note that this is true irrespective of whether we store the property in the json or let the file system (name, metadata) carry the info.

As a consequence we also have no way to fix the notion of default account.

There are 2 solutions:

Give up portability of individual key files with simple copy and we require explicit import of keys into a node and it will just create a new file that is guaranteed to be sequentially after the ones already on the account. This solution is really cumbersome, high maintenance. It is just a question of time that people will request ability to modify primary account which will require json encoding which will necessitate further complication in the spec (in that primariness is not preserved via export-import) And surely the next thing will be named accounts, etc etc, we should stop this before its late.

Pro portability arguments also apply to file corruption, compormised keys and key deletion. So I hope this is decided.

My suggestion is radically simple.

On the level of geth (as in the ethereum stack backend), there is only a set of addresses. It will be up to Dapps, wrappers or users to remember which password belongs to which address and how you retrieve them. Luckily we got the best tool to help you with that:

myregistrar.reserve.sendTransaction("primary")
myregistrar.setAddress.sendTransaction("primary", eth.accounts[9], true, {from: myregistrar.addr("mum's account")})

Of just use a full wallet contract. So we dont even need to change the documentation much only where primary account is defined.

For completeness we need to make sure etherbase has no default value and must always be explicitly given when mining. In the special case there is exactly one account, it could theoretically default to that, but even that is wrong cos etherbase can also be an external address, so it really shouldnt matter if it belongs to a local account or external.

This requires only

This looks both necessary and realistic for frontier

tgerring commented 9 years ago

Thanks for the detailed analysis @zelig!

A couple of thoughts I have:

"Interpret the indexing in the response for geth account list and web.js eth.accounts as random (i.e non-sematic like the order of keys iterating over a map) and document it well"

We should be careful with this and at least provide some guarantee of when it might change, i.e. program restart or account import/creation. Otherwise, the user may encounter unpredictable behaviour when running scripts on the console and the indexing changes underneath them

"extra check for etherbase (allow setting in admin console would also be nice)"

I think we need the option to set the etherbase from console regardless of what happens to account management. Currently, it's entirely a bit useless having it default to eth.accounts[0]

zelig commented 9 years ago

thanks @tgerring I agree with both. I will implement creation date ordering but emphasize caveats in docs. We eliminate the notion of primary account or replace it with namereg notion. Finally etherbase will need to be explicitly specified, and yes setting it will be supported on the console though the admin API. https://github.com/ethereum/go-ethereum/pull/1283