Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change asset symbol internal implementation #1549 #1577

Merged
merged 7 commits into from Oct 4, 2017

Conversation

theoreticalbts
Copy link
Contributor

@theoreticalbts theoreticalbts commented Sep 15, 2017

PR for #1549. This PR is based on #1576, so you should merge #1576 first EDIT: #1576 was merged.

This PR is a refactoring, code that uses asset should have equivalent functionality (although minor changes to the interface required simple changes to a few source files). I've implemented the following changes in this PR:

  • Upgrade asset_symbol_type from uint64_t to a class
  • Implement custom (de)serialization methods pack(), unpack(), to_variant() and from_variant() so it has identical binary and JSON forms to the old version.
  • Re-implement string conversion functions
  • Be a little more precise about what regular expressions are accepted as strings
  • Be a little more precise about what bit patterns represent what asset, and what bit patterns are reserved
  • Implement validate() function for symbols
  • Implement space() and NAI range constants which will be used in a future PR implementing NAI's (numerical asset identifiers)
  • Actually store asset internally as 32-bit number called asset_num [1]

[1] Realistically only 2 bits would be needed if we're only storing STEEM|SBD|VESTS. But while this code is a refactoring that preserves functionality, its ambition isn't limited to refactoring. It's changing the code into a form that will be able to quickly evolve support for NAI's. I'm thinking 32 bits for NAI's (although the spec calls for 9 digits, we'll need to reserve 4 bits for precision; I'm thinking of making the 9th digit a check digit for cases of manual entry).

@theoreticalbts
Copy link
Contributor Author

A few unit tests were (in some cases accidentally) initializing assets with illegal strings that are now caught during parsing. I think I got them all.

@theoreticalbts theoreticalbts force-pushed the 1549-asset-symbol-assign branch 2 times, most recently from b11f656 to 718276b Compare September 15, 2017 15:01
Copy link
Contributor

@mvandeberg mvandeberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2960303ms database.cpp:2704             _apply_block         ] 10 assert_exception: Assert Exception
next_block.transaction_merkle_root == merkle_root: Merkle check failed
    {"next_block.transaction_merkle_root":"8195f855f6d54d6856f9a08a08b21bdd6b14472e","calc":"80fb0889cd52a00f57d0817a756679973eecd623","next_block":{"previous":"000f2bbfcbdad7bb80bc42c476567c750badd90b","timestamp":"2016-04-28T23:22:18","witness":"steemychicken1","transaction_merkle_root":"8195f855f6d54d6856f9a08a08b21bdd6b14472e","extensions":[],"witness_signature":"1f2ce40298d1569ba0f118146f71482ff18afdb02d844321addf3ebd7e970d65a625d4cb65aae4a9c884f896f0c2c6939603287997377d1f2d440d6f09825aab97","transactions":[{"ref_block_num":11199,"ref_block_prefix":3151485643,"expiration":"2016-04-28T23:22:45","operations":[["witness_update",{"owner":"liondani","url":"https://bitcointalk.org/index.php?topic=1410943.msg14643675#msg14643675","block_signing_key":"STM66crmnX96YMyWUPnFzSkv7DsGESbWaoSGbmoNq4EWDYq2wnwPf","props":{"account_creation_fee":"0.001 STEEM","maximum_block_size":131072,"sbd_interest_rate":1000},"fee":"0.000 STEEM"}]],"extensions":[],"signatures":["1f1fd68fd2b2ec919357c8e534d1473a16f5505a7719bbb2a2100478f212f9353272c38b12d1468d1c3eb830f659e80a28a5a985661bb2c79e4acd2ef34bc919b4"]}]},"id":"000f2bc023d45d15a543cb9bb19b56c25012d15c"}
    database.cpp:2615 _apply_block
2960303ms database.cpp:2704             _apply_block         ] next_block.block_num(): 994240 
10 assert_exception: Assert Exception
next_block.transaction_merkle_root == merkle_root: Merkle check failed
    {"next_block.transaction_merkle_root":"8195f855f6d54d6856f9a08a08b21bdd6b14472e","calc":"80fb0889cd52a00f57d0817a756679973eecd623","next_block":{"previous":"000f2bbfcbdad7bb80bc42c476567c750badd90b","timestamp":"2016-04-28T23:22:18","witness":"steemychicken1","transaction_merkle_root":"8195f855f6d54d6856f9a08a08b21bdd6b14472e","extensions":[],"witness_signature":"1f2ce40298d1569ba0f118146f71482ff18afdb02d844321addf3ebd7e970d65a625d4cb65aae4a9c884f896f0c2c6939603287997377d1f2d440d6f09825aab97","transactions":[{"ref_block_num":11199,"ref_block_prefix":3151485643,"expiration":"2016-04-28T23:22:45","operations":[["witness_update",{"owner":"liondani","url":"https://bitcointalk.org/index.php?topic=1410943.msg14643675#msg14643675","block_signing_key":"STM66crmnX96YMyWUPnFzSkv7DsGESbWaoSGbmoNq4EWDYq2wnwPf","props":{"account_creation_fee":"0.001 STEEM","maximum_block_size":131072,"sbd_interest_rate":1000},"fee":"0.000 STEEM"}]],"extensions":[],"signatures":["1f1fd68fd2b2ec919357c8e534d1473a16f5505a7719bbb2a2100478f212f9353272c38b12d1468d1c3eb830f659e80a28a5a985661bb2c79e4acd2ef34bc919b4"]}]},"id":"000f2bc023d45d15a543cb9bb19b56c25012d15c"}
    database.cpp:2615 _apply_block
rethrow
    {"next_block.block_num()":994240}
    database.cpp:2704 _apply_block

    {"next_block":{"previous":"000f2bbfcbdad7bb80bc42c476567c750badd90b","timestamp":"2016-04-28T23:22:18","witness":"steemychicken1","transaction_merkle_root":"8195f855f6d54d6856f9a08a08b21bdd6b14472e","extensions":[],"witness_signature":"1f2ce40298d1569ba0f118146f71482ff18afdb02d844321addf3ebd7e970d65a625d4cb65aae4a9c884f896f0c2c6939603287997377d1f2d440d6f09825aab97","transactions":[{"ref_block_num":11199,"ref_block_prefix":3151485643,"expiration":"2016-04-28T23:22:45","operations":[["witness_update",{"owner":"liondani","url":"https://bitcointalk.org/index.php?topic=1410943.msg14643675#msg14643675","block_signing_key":"STM66crmnX96YMyWUPnFzSkv7DsGESbWaoSGbmoNq4EWDYq2wnwPf","props":{"account_creation_fee":"0.001 STEEM","maximum_block_size":131072,"sbd_interest_rate":1000},"fee":"0.000 STEEM"}]],"extensions":[],"signatures":["1f1fd68fd2b2ec919357c8e534d1473a16f5505a7719bbb2a2100478f212f9353272c38b12d1468d1c3eb830f659e80a28a5a985661bb2c79e4acd2ef34bc919b4"]}]}}
    database.cpp:2579 apply_block

    {"data_dir":"/Users/mdv/git/steem/./appbase_dir/blockchain","shared_mem_dir":"/Users/mdv/git/steem/./appbase_dir/blockchain"}
    database.cpp:201 reindex


#define VESTS_SYMBOL_SER (uint64_t(6) | (VESTS_SYMBOL_U64 << 8)) ///< VESTS with 6 digits of precision
#define STEEM_SYMBOL_SER (uint64_t(3) | (STEEM_SYMBOL_U64 << 8)) ///< STEEM with 3 digits of precision
#define SBD_SYMBOL_SER (uint64_t(3) | (SBD_SYMBOL_U64 << 8)) ///< Test Backed Dollars with 3 digits of precision
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Test Backed Dollars" should read "Steem Backed Dollars" or "SBD". This is not in a test net def.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was copy-pasted, I'll change it.

@theoreticalbts
Copy link
Contributor Author

Oops, accidentally added the fix to #1581 rather than this PR. All should be well now.

@theoreticalbts
Copy link
Contributor Author

theoreticalbts commented Oct 4, 2017

The problem is #419 which allowed some improper fee symbols to make it into blocks in the live blockchain. We have a couple options to fix this:

  • (a) Allow the new parser to accept the wrong assets that got into the chain as alternate binary "spellings" of STEEM.
  • (b) Redefine the types used in the problematic operation, allowing the binary parser to only accept alternate "spellings" of STEEM for this specific operation.

In the interest of making all new code use the "tight" parser by default, to hopefully avoid problems like this in the future, I decided to implement option (b). I'll add the code to this PR. Also, since the chain_properties in the witness_update_operation now needs to use special types to indicate its binary serialization can differ, I moved the chain_properties type to chain.

In addition, I rebased on latest develop.

@mvandeberg
Copy link
Contributor

Oddities introduced by this refactor will largely be addressed with #1620

@theoreticalbts
Copy link
Contributor Author

I've verified it will replay through HF14.

@mvandeberg mvandeberg merged commit c8cb93e into develop Oct 4, 2017
@mvandeberg mvandeberg added this to Done in Steem 0.21.0 Oct 4, 2017
@mvandeberg mvandeberg deleted the 1549-asset-symbol-assign branch October 5, 2017 17:44
@youkaicountry youkaicountry added project/smt_v05 Stories created before July rescope and removed project/hf21 labels Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project/smt_v05 Stories created before July rescope
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants