cryptonomex / graphene

MIT License
1.05k stars 336 forks source link

Missing Active Authority while adding active authority #630

Closed abitmore closed 8 years ago

abitmore commented 8 years ago

Exception Missing Active Authority is reported while trying to add TEST56ankGHKf6qUsQe7vPsXTSEqST6Dt1ff73aV3YQbedzRua8NLQ as another active key authority to http://testnet.bitshares.eu/#/account/testaccount1/permissions/, even if threshold set to 1 and weight of all keys set to 1. Private key of the new new public key is 5K9piFpyJ15zcYBc4R3gAeLFYb6PuRFEiDCbzLkSL9C4d7fWLyB.

Original key pairs of that account:

The account can vote without issue (no change to owner or active authorities), so perhaps an issue while validating the new key.

abitmore commented 8 years ago

Found something interesting: the transaction generated by GUI is rejected by witness_node, because witness_node think it's signed by other keys but not the active key and owner key of the account.

The transaction is:

{  
   "ref_block_num":1954,
   "ref_block_prefix":2101285432,
   "expiration":"2016-03-21T11:32:18",
   "operations":[  
      [  
         6,
         {  
            "fee":{  
               "amount":2016992,
               "asset_id":"1.3.0"
            },
            "account":"1.2.321",
            "owner":{  
               "weight_threshold":1,
               "account_auths":[  

               ],
               "key_auths":[  
                  [  
                     "TEST6vGWLjXrB971RXKoejCsZoAsv1wbfEmwNK7deN3kcdYQFck8wb",
                     1
                  ]
               ],
               "address_auths":[  

               ]
            },
            "active":{  
               "weight_threshold":1,
               "account_auths":[  

               ],
               "key_auths":[  
                  [  
                     "TEST8me6d9PqzTgcoHxx6b4rnvWVTqz11kafidRAZwfacJkcJtfd75",
                     1
                  ],
                  [  
                     "TEST56ankGHKf6qUsQe7vPsXTSEqST6Dt1ff73aV3YQbedzRua8NLQ",
                     1
                  ]
               ],
               "address_auths":[  

               ]
            },
            "new_options":{  
               "memo_key":"TEST8me6d9PqzTgcoHxx6b4rnvWVTqz11kafidRAZwfacJkcJtfd75",
               "voting_account":"1.2.113",
               "num_witness":0,
               "num_committee":0,
               "votes":[  

               ],
               "extensions":[  

               ]
            },
            "extensions":{  

            }
         }
      ]
   ],
   "extensions":[  

   ],
   "signatures":[  
      "1f668c6580fa54eaee9536288cedd020279dc3c1339acf79df4855dd9deeb73f1f1912f5bf082385696a8e88e77b443d59f3a645d9e56416653aa1d6175ffaa551",
      "20210d5280d69bd229fa0624958a52cc10c1e3536b6b2dd3fb5d11a7ee440f3c1870a31e9c12320021d374999fe1af14d6c476d7867f0c423c73eb75905c2e6972"
   ]
}

witness_node think the transaction is signed by

sigs: ["TEST7uHmxmEjiAtatdYwfR9FvPCs3aQPsAm6SDY18BjtEdQxGthZSZ","TEST8ANgHxhznRLsk1Je7zswisiu4QY1st5DADoqRnzJbKU5AYW9W3"]

Chain-id of testnet is "39f5e2ede1f8bc1a3a54a7914414e3779e33193f1f5693510e73cb7a87617447"

Either something wrong while signing the transaction, or something wrong while verifying the signatures. Thoughts?

abitmore commented 8 years ago

Same transaction can be signed in CLI wallet and got accepted by witness_node without issue. So it's perhaps a GUI issue.

jcalfee commented 8 years ago

In the stealth branch, I can find testaccount1 and add that public key. In the transaction preview the TEST5... key comes before the TEST8... key (unlike what you see above in the " transaction generated by GUI is rejected by witness_node,"). The same code is in master, but I'm having a hard time starting the master node to confirm, due to a missing loader.

jcalfee commented 8 years ago

Here is the fix... It looks like addresses and public keys should be sorted using descending order unlike most other types (which would be sorted ascending).

https://github.com/cryptonomex/graphene-ui/commit/947f5bae225d6754087bcd225b0665e24c150a73

abitmore commented 8 years ago

Unfortunately there is no explicit operate < defined for struct public_key_type in Graphene, so the sorting mechanism of public keys in witness_node is something I don't know right now. Here is a result of key sorting test (the output is sorted):

base58key                                              binary
TEST5epyDkfDRaJNvH4x9imTb1QxQcZ4755fhegh5Xsq9CiuGQzGed 0264ace332b75e6c245dbe668da5247432bc44e81ee3d49328edf4059614683a6f
TEST7Epiyj977z9oGKgFRCaM6tStvgqT5Q9frGGjqcRej38waQJ4qm 0335901fafcb51073c61d60e87164773e257f6821dd6ad4094270cd33c437c5c04
TEST7s8oDfgagLemh87scWi65oFHGbdqAR1HKguCPPsM3Hg7xMU1CA 0388034f1656feb97ed535a75610db3b0aa39717ef5c86fde33e4d8d3503031ed2
TEST7dHScifVyx1FfncnEXtNwrutdAGBjt6Ag6m2RB6NX4HzB47vMM 036890097947593fe45b9f84ac357aa3932a77312008a9fb9339e18b891b37242d
TEST5a2TmhxanP6Jm81qM12JdY3qUiLHUaeNXy6kHqQXNRTnNMBvwL 0259c5aabe4768a46cb93f76c2262d0409c168fc4ae8f2c4bba3cbbc16340fe319
TEST7nyEzMsbuLSq9i1gKqXYW84pF37z57GW5t6JzDNubnEuLB9eZP 037e8e6c9185037dab02ce041a561cc5b4fc8f9314cea6a541272fe4f046097ecc
TEST7oD6Xf7di34VMmT9DtNLZBCe37yjY9cdjQja3UDqc6baUW2v61 037f19455fa19e866fcec045b646b82e9a12923ca625453c2b9d4276e6bfbe7ef5
TEST5QWRmkUmawYwV9Nh74yfrzAtDWUmDuX7x9yx4naoBDbV2TRYmV 024429255c10e82c747dc098fec6038d6720771f63e86e15f29224f19d42b0fb3f
TEST8ihkTEdf7pWXwfB4QecnKADQMgn7kTXkM3w66edfWarXSG46Mk 03f8903ed30843840009854e942645f0157f46346a5ffa7165a2e77d575e90b412
TEST6TSGWY6t9snvMMTmUL97JL6kDAJdkaRtBScRki66dKVUhnBGZ5 02ce817edef12e67ad975fe7fef8930451cb39206759966f87e99a0ae879c52313

It's terrible for 3rd-party clients, including Graphene-ui. The patch provided by @jcalfee in above comment won't work in all cases. We need a hard fork to clearly define the sorting mechanism of public keys. @theoreticalbts thoughts?

By the way, according to the code, addresses are sorted by the binary representation, so likely same to base58 string sorting. A result of address sorting test is here:

TEST22hAH19ooWPXYbgqagG6NyG3HkFHQ9iiC
TEST2ycM6oYtYyutLYaFLwgJj5aAGzDwGyy9k
TEST4H6EGrk7jcFkXDsPevxT8HS4tF7bSkKHC
TEST4MYvRqDjgKpngXoazgnNdyfLMduPTqfD8
TEST9ygFXAxrVvnvKCNoVxxYurPG6pYVZY5JJ
TESTBuP52oNQW5M7r34TvwkmqeyrATcyKhmiq
TESTGKM4KLUqifrHCYKLeSGQ1Z33823Bvy5Ff
TESTGYwJsCThRf3frkDXGW31xxYjaz83Vk7zc
TESTGkYxuf5WyFYnGcmuWd1Yts33Tk7uYTJPj
TESTQJAzzAiyD6n6xr6q2gyxYvUc9nSLBMymF

So GUI code which hasn't applied above patch should work with addresses. However, address_authority is implemented for backward compatibility only, probably we'll disable it in the future.

theoreticalbts commented 8 years ago

Yeah, the issue here is that we have

  flat_map<public_key_type,weight_type> key_auths;

which means the key_auths are sorted when this structure is serialized. So if the GUI isn't using the same sorting as the C++, it's not going to accept the sig.

theoreticalbts commented 8 years ago

The currently used sorting mechanism is to convert the keys to addresses, then sort by address. Addresses themselves are sorted by hex value.

The above branch (updated below with a better commit message) implements a sort-keys binary which creates 100 public keys and sorts them. The GUI should work if it sorts the keys in the same order. Sorting consists of converting the keys to addresses, converting the addresses to hex and then sorting the hex values. Addresses should be sorted the same way for address_auths.

abitmore commented 8 years ago

OK, at least we don't need to hard fork. @theoreticalbts may I ask why (where is the code) public keys are sorting in this way?

I'm going to close this ticket and move discussion to graphene-ui repo.

theoreticalbts commented 8 years ago

It's not done explicitly in the code, rather since public_key_type doesn't have operator< but is convertible to address which does have operator<, the compiler decides to convert it. There's probably somewhere in the C++11 standard telling exactly how that decision is made.

Figuring out that was what the code is doing is non-trivial. I wrote the unit test above (it's not really a test, it just creates two keys and compares them). Then set a breakpoint at the line that compares the two keys and stepped into it with GDB.

We should probably implement the ordering as a function

pureland commented 8 years ago

the reason is https://bitsharestalk.org/index.php?action=profile;area=showposts;u=5815

jcalfee commented 8 years ago

Their are 5 different address formats... I think I know which one to use though, we have a default one. I'll look at the code and maybe I can use your unit test to confirm.

abitmore commented 8 years ago

Fixed in GUI 2.0.160406