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

Deterministic numbering of virtual operations #1675

Closed
abitmore opened this issue Mar 24, 2019 · 4 comments

Comments

Projects
3 participants
@abitmore
Copy link
Member

commented Mar 24, 2019

User Story

These fields are in operation_history_object:

         /** the block that caused this operation */
         uint32_t          block_num = 0;
         /** the transaction in the block */
         uint16_t          trx_in_block = 0;
         /** the operation within the transaction */
         uint16_t          op_in_trx = 0;
         /** any virtual operations implied by operation in block */
         uint16_t          virtual_op = 0;

Earlier described in #206, numbering of virtual operations (read: virtual_op field) is not deterministic, which have caused confusions to client application developers.

There is a virtual_op field in operation_history_object ... is filled with the variable _current_virtual_op in database.hpp ..., but _current_virtual_op has never been saved to or loaded from disk (but only increases forever). If witness_node is started without --replay-blockchain, _current_virtual_op is reset to 0 ...

oh.virtual_op = _current_virtual_op++;

That said, different API nodes may return different virtual_op values when being queried with same parameters.

Here I propose a rule about deterministic numbering of virtual operations:

  • For real operations (which are explicitly included in a signed transaction), let virtual_op = 0;
    • Note: both trx_in_block and op_in_trx are indexes start from 0, E.G. if a block contains 2 transactions, trx_in_block of the operation_history_objects may be 0 and 1.
  • For virtual operations caused by a real operation (E.G. two fill_order_operations caused by a limit_order_create_operation), let (block_num,trx_in_block,op_in_trx) be the same as the real operation, and let virtual_ops be 1, 2, ...
  • For virtual operations which are not directly caused by a real operation (E.G. virtual limit_order_cancel_operations due to expiration),
    • let trx_in_block be the number of transactions in the block, E.G. let it be 2 if the block contains 2 transactions,
    • let op_in_trx = 0,
    • let virtual_ops be 0, 1, ...

Impacts
Describe which portion(s) of BitShares Core may be impacted by your request. Please tick at least one box.

  • API (the application programming interface)
  • Build (the build process or something prior to compiled code)
  • CLI (the command line wallet)
  • Deployment (the deployment process after building such as Docker, Travis, etc.)
  • DEX (the Decentralized EXchange, market engine, etc.)
  • P2P (the peer-to-peer network for transaction/block propagation)
  • Performance (system or user efficiency, etc.)
  • Protocol (the blockchain logic, consensus, validation, etc.)
  • Security (the security of system or user data, etc.)
  • UX (the User Experience)
  • Other (please add below)

Additional Context (optional)
Add any other context about your request here.

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation
@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Makes sense. Added "Protocol" label because there should be an implicit consensus about the numbering, although it is technically not part of the protocol.

abitmore added a commit that referenced this issue Mar 25, 2019

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Currently this Issues is assigned to a Feature Release (3.1). Should it be moved to the Protocol Upgrade (4.0.0) release?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

No, it can go into the feature release. This non-deterministic behaviour should be fixed in the next release.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

Merged #1676. Closing.

@abitmore abitmore closed this Apr 10, 2019

Feature Release (3.1.0) automation moved this from To do to Done Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.