Issue375 - slow comparison of public_key_type

[sierra19XX]

No description provided.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information

[nathanielhourt]

Unfortunately, it is my understanding from the BitShares folks that this will cause a hardfork. The sorting order of objects can affect consensus, and while it shouldn't matter in sign_state, it does matter, for example, in the database.

The best way forward, I think, is to put together a list of all places where the operator defined here would be used, audit each one to determine whether or not it can affect consensus, and then put hardfork guards around each of them to ensure that the behavior change is handled smoothly.

[sierra19XX]

Unfortunately, it is my understanding from the BitShares folks that this will cause a hardfork. The sorting order of objects can affect consensus, and while it shouldn't matter in sign_state, it does matter, for example, in the database.

The best way forward, I think, is to put together a list of all places where the operator defined here would be used, audit each one to determine whether or not it can affect consensus, and then put hardfork guards around each of them to ensure that the behavior change is handled smoothly.

@nathanhourt
Thanks for pointing this out. Come to think of it, sign_state auth force validation might also fail.
Please correct me if I'm wrong, but I think the below scenario can occur during tx verification.
Eg.
flat_map<public_key_type,weight_type> key_auths;
Before fix Authority keys order can be - (puk1, 5) (puk2, 10) Weight Threshold = 10
After fix order can be - (puk2,10) (puk1, 5), this can make puk1 signature redundant and thereby reject the transaction through remove_unused_signatures

[nathanielhourt]

Ahh, I believe you are correct, yes. Ideally it would check the authenticators in an authority from greatest to least weight, which would have made this issue irrelevant while also being a better check against redundant signatures... But this can't be done now without a hardfork for exactly the reason you described. :P

In general, it may be tricky to predict accurately whether a particular sort change could cause a fork or not, which is why I suggest just guarding all of them.

There is some complexity yet to be dealt with, however, if any database tables are ordered based on public_key_type, which is that at the time of the fork, the affected indexes will need to be completely re-sorted. I'm not sure if there's a reliable way to do this with boost multi index containers, except to remove and reinsert all objects in the index.