Order fill events notified before order creation events

abitmore commented 6 years ago

Bug Description

the market subscription thread seems to be guaranteed to fire before the transaction callback thread, which doesn't make sense for clients because it implies that the market order went through before it actually did.

A client that subscribed to a market always receives order fill events before order creation events if the orders are partially filled in a same block. Note: no notification on order creation if it gets completely filled in same block (not sure if it need to be fixed so far).

There is a FIXME in code: https://github.com/bitshares/bitshares-core/blob/d94cc574b2ca460dd636e573f25f0c7737b4a355/libraries/app/database_api.cpp#L2311-L2313

Probably because on_applied_block() is called before on_objects_changed(): https://github.com/bitshares/bitshares-core/blob/d94cc574b2ca460dd636e573f25f0c7737b4a355/libraries/chain/db_block.cpp#L540-L544

Expected Behavior When a new order is placed, clients receive an order creation event first, then an order fill event.

Additional Context (optional) I'm not sure if it's safe to simply move on_objects_changed() to be called before on_applied_block().

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

[x] 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)

CORE TEAM TASK LIST

[ ] Evaluate / Prioritize Bug Report
[ ] Refine User Stories / Requirements
[ ] Define Test Cases
[ ] Design / Develop Solution
[ ] Perform QA/Testing
[ ] Update Documentation

ghost commented 6 years ago

Note that this problem also occurs with fill orders. If an order fills instantly on creation then the market subscription callback is sometimes called first and then the transaction is processed. The order of the callbacks in this case seems to be indeterministic, unlike the bug mentioned above which seems to be deterministic from my tests.

abitmore commented 6 years ago

I think we can defer the fill_order notification until notifications about changed object are sent, e.g. temporarily save it somewhere, then send it after sent other notifications.

ghost commented 6 years ago

Actually I misread the issue above. The problem you described is concerning the fill orders. The original problem we discussed in earlier chats was about the order creation notification (ie subscribe_to_market firing before the response of broadcast_transaction_synchronous is sent back to the client.

For example, the output of:

"{ 'id': -1, 'method': 'call', 'params': [2, subscribe_to_market, [9001, "1.3.0", "1.3.21"]] }"

is being returned to the client before the output of:

"{ 'id': 1, 'method': 'call', 'params': [network_broadcast, broadcast_transaction_synchronous,[%s]]}"

One would expect that the transaction would be confirmed first before it is broadcasted as an event. I think this is a minor issue though, the fill order bug is certainly more problematic

ghost commented 6 years ago

I think that if we consider the above bug^^ I mentioned to be not a problem, then its fine to leave the fill order bug as it currently is because the client can just listen for fill orders that do not have a matching order and put them in an "orphan" list, then when the order arrives the client can fill the order. It's a bit troublesome but I think if implemented well the code can run without any issues.

What are your thoughts @abitmore @pmconrad @elmato ?

abitmore commented 6 years ago

@arminnaderi actually I misunderstood your question earlier. Please create a new issue for that.

ghost commented 6 years ago

Ok, will do now

abitmore commented 6 years ago

Your issue is still related though. Notification of broadcast_transaction_synchronous is fired by applied_block signal (which is same to order fills but of different API container), notification of new orders is fired by changed_objects signal, there are threading involved so the order is not deterministic. In addition, because they're not in the same container (one is in network_broadcast_api and the other is in database_api), I think it's harder to fix.

ghost commented 6 years ago

Yeah, maybe we can continue the discussion in this issue, the two issues are very related IMO. I don't have a problem with this not being fixed because there is a workaround I can code on the client side, but I just want to confirm that the two mentioned problems aren't considered bugs and they won't be "fixed" in the future, because if I do code the workarounds and these methods gets patched it will be a waste of effort. Better to fix it now if it will be fixed in the future anyway

Do you think the threads will be synchronized in the future, or if this problem is considered a "bug"?

ghost commented 6 years ago

Don't think this issue can be fixed in a manner that doesn't cause side effect problems, requesting close

abitmore commented 6 years ago

It's still somewhat a bug. Although we're unable to fix it now, but perhaps can fix it in the future. So I'll keep this issue open.

ghost commented 6 years ago

What I meant is order fill events and order creation events are not the only two events that are pushed by on_applied_block() and on_objects_changed(). Some clients might actually have no preference in the order of these events. However, I agree that order fill events should probably come after order creation events, if its fixed it will affect all other events that are pushed by these methods as a side effect

ryanRfox commented 5 years ago

Assigned to @OpenLedgerApp

oxarbitrage commented 5 years ago

I initialized some work in this issue however i didn't assigned myself and i didn't made a lot yet. Will like to know where @openledgerapp is up as i maybe can keep it or if they are advanced i can just abandon it.

OpenLedgerApp commented 5 years ago

We have already spent our hours on this issue. So, we want to keep it on us

oxarbitrage commented 5 years ago

ok, no worries.

OpenLedgerApp commented 5 years ago

Thanks!

ryanRfox commented 5 years ago

Sorry, @oxarbitrage I did not realize you already had some effort into this one. If you have anything to PR that may assist @OpenLedgerApp in their development, please do so. Thanks

OpenLedgerApp commented 5 years ago

Currently, from one side: Database_api subscribes to the events from DB (new, changed, removed) object. Filters and accumulates events related to (call_order_object, limit_order_object, force_settlement_object) and finally via fc::async notifies listeners.

From other side: Database_api subscribes to the event from DB (applied_block), Filters operations and accumulates events related to (fill_order_operation) and via fc::async notifies listeners.

There are some inconveniences: 1: Full match in the same block - Database_api emits only two fill_order events 2: Order of events from Database_api is not well defined as Database_api in fc::async use different queue for listeners. 3: Listeners receive different objects: *_order_oject, {fill_order_operation, fill_order_operation_result} or object_id

This is draft version: and could be extended. We propose to extend Database_api: Use only applied_block for filtering operations and accumulates events related to (limit_order_create_operation, fill_order_operation, limit_order_cancel_operation) and notifies listeners. In this approach listeners will receive notifications regardless where they were happened (in the same block or not) and order will be defined. See https://github.com/openledger/bitshares-core/commit/677a1517d2fb3f5be372f077ead80ce82ac7ad55

ghost commented 5 years ago

I'm the original person who brought this issue up. It's not a problem anymore as it can be fixed client-side.

Websocket events are asynchronous and not guaranteed to come in any particular order.

@OpenLedgerApp Could you clarify what you mean by 1), "Full match in the same block" ? (With an example please)

OpenLedgerApp commented 5 years ago

create_sell_order(alice, core.amount(100), usd.amount(100)); create_sell_order(bob, usd.amount(100), core.amount(100)); If these operations are matched in the same block, listener will get only two fill_order notifications. @arminnaderi Could you share a bit more context where you faced with this problem (#1277)?

ghost commented 5 years ago

@OpenLedgerApp You mean the listener will only send 2 fill_order notifications, and 0 order_creation (transaction callbacks) ?

I faced this problem when creating an order which was instantly filled in the same block. But the problem can be fixed client-side, it's not actually a technical bug.

Please check if you are receiving the transaction callbacks and paste your findings here - You need to set an id when sending broadcast_transaction_synchronous and check if you receive a response with a matching id

OpenLedgerApp commented 5 years ago

You mean the listener will only send 2 fill_order notifications, and 0 order_creation (transaction callbacks) ?

Yes, database_api raises 2 fill_order notifications, and 0 order_creation.

I wonder if it is important for listener to receive notifications {order_creation, fill_order, cancel_order}? If Yes, it https://github.com/bitshares/bitshares-core/issues/1277#issuecomment-467416486 could help, Otherwise it is not done in current implementation. Use case: It is the case if orders are particularly matched in the different blocks: steps: 1: create_sell_order(alice, core.amount(100), usd.amount(100)); 2: wait some blocks 3: create_sell_order(bob, usd.amount(200), core.amount(200)); result: we have the following order of events: order_creation -> fill_order->fill_odrer -> order_creation as it is mentioned #1277.

It is correct behavior! In the 1st step order is created and not matched. Then in the 3d step partly matching is happened (fill_order, fill_order ) and finally a new order was created (order_creation).

ghost commented 5 years ago

If the order was fully filled the moment it was created, no order_creation will be notified. This is normal behavior. Instead, a full_order_object will be sent to the websocket listener, which is basically a copy of the order

OpenLedgerApp commented 5 years ago

If the order was fully filled the moment it was created, no order_creation will be notified. This is normal behavior. Instead, a full_order_object will be sent to the websocket listener, which is basically a copy of the order

Yes, the same logic I've mentioned above. Are there any more requirements to #1277?

ghost commented 5 years ago

Nope, that's why I mentioned there is no bug. You have all the information you need to properly handle the relevant events.

ghost commented 5 years ago

Also, @OpenLedgerApp , please vote against the "Reduce MSSR of bitCNY to 1.02" poll worker if you want bitCNY to not go into global settlement. If a global settlement on an asset as crucial as bitCNY happens, many users will leave and it will hurt your bottom line.

pmconrad commented 5 years ago

No politics here please.

abitmore commented 5 years ago

@OpenLedgerApp expected behavior is described in OP:

when a new order is placed, clients receive an order creation event first, then an order fill event.

Take your use case as an example:

Use case: It is the case if orders are particularly matched in the different blocks: steps: 1: create_sell_order(alice, core.amount(100), usd.amount(100)); 2: wait some blocks 3: create_sell_order(bob, usd.amount(200), core.amount(200)); result: we have the following order of events: order_creation -> fill_order->fill_odrer -> order_creation as it is mentioned #1277.

It is correct behavior! In the 1st step order is created and not matched. Then in the 3d step partly matching is happened (fill_order, fill_order ) and finally a new order was created (order_creation).

IMHO the correct order of events should be: order_creation(alice), order_creation(bob), fill_order(alice), fill_order(bob), no matter if alice's order or bob's has been fully filled or both.

OpenLedgerApp commented 5 years ago

@OpenLedgerApp expected behavior is described in OP:

when a new order is placed, clients receive an order creation event first, then an order fill event.

Take your use case as an example:

Use case: It is the case if orders are particularly matched in the different blocks: steps: 1: create_sell_order(alice, core.amount(100), usd.amount(100)); 2: wait some blocks 3: create_sell_order(bob, usd.amount(200), core.amount(200)); result: we have the following order of events: order_creation -> fill_order->fill_odrer -> order_creation as it is mentioned #1277. It is correct behavior! In the 1st step order is created and not matched. Then in the 3d step partly matching is happened (fill_order, fill_order ) and finally a new order was created (order_creation).

IMHO the correct order of events should be: order_creation(alice), order_creation(bob), fill_order(alice), fill_order(bob), no matter if alice's order or bob's has been fully filled or both.

@abitmore it could be done via operations. see https://github.com/openledger/bitshares-core/commit/677a1517d2fb3f5be372f077ead80ce82ac7ad55. You are welcome to discuss it. Thanks.

pmconrad commented 5 years ago

We have two issues here:

there is no notification for order object creation if an incoming order is filled immediately
if an incoming order is partially filled, the notification for the fill will be sent before the notification for order object creation

The ~second~first point is a bug IMO, while the ~first~second may simply be unexpected but valid behaviour (if message order is not guaranteed like @arminnaderi said).

ghost commented 5 years ago

@pmconrad I think the first issue is a bug from the perspective of writing clients, because you don't actually know if your order was created or not. Assuming that an order was created because a fill event pops up is too complicated.

Also, as I mentioned a few posts above, when the order is fully filled on creation, a full_order_object is sent to the websocket listener instead of the callback to broadcast_transaction - I agree it's a bit complicated to check for the full_order_object though

OpenLedgerApp commented 5 years ago

We have made PR - https://github.com/bitshares/bitshares-core/pull/1636

This PR describes a draft vision of market events model.

jmjatlanta commented 5 years ago

I have built some unit tests for creating markets and trades.

https://github.com/bitshares/bitshares-core/pull/1661

xloem commented 5 years ago

A simple bandaid for this that might make life easier for the developers of client apps would be to include more order fields in the fill notification, notably the "for_sale" quantity, such that it could be used to initialize an object locally and to verify ordering of events.

OpenLedgerApp commented 5 years ago

I have built some unit tests for creating markets and trades. This may help test this issue. I will update this comment when I find the relevant code.

When you can provide this test?

jmjatlanta commented 5 years ago

Sorry. I found it and then got distracted. Here it is:

https://github.com/bitshares/bitshares-core/pull/1661

Oh look! A squirrel....

OpenLedgerApp commented 5 years ago

Sorry. I found it and then got distracted. Here it is:

1661

Oh look! A squirrel....

Thanks. Sure, this may help test this issue, but the use case, mentioned above, that reproduces the #1277 is not so complicated.

bitshares / bitshares-core

Order fill events notified before order creation events #1277

CORE TEAM TASK LIST

1661