Order fill events notified before order creation events

[abitmore]

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:

bitshares-core/libraries/app/database_api.cpp

Lines 2311 to 2313 in d94cc57

	if( market.valid() && _market_subscriptions.count(*market) )
	// FIXME this may cause fill_order_operation be pushed before order creation
	subscribed_markets_ops[*market].emplace_back( std::move( std::make_pair( op.op, op.result ) ) );

Probably because on_applied_block() is called before on_objects_changed():

bitshares-core/libraries/chain/db_block.cpp

Lines 540 to 544 in d94cc57

	// notify observers that the block has been applied
	notify_applied_block( next_block ); //emit
	_applied_ops.clear();
	notify_changed_objects();

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.

• 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

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]

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.

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

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]

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

Ok, will do now

[abitmore]

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.

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"?

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