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

Add a test that reproduce something-for-nothing fill bug #345

Merged
merged 2 commits into from
Aug 23, 2017

Conversation

takaaki7
Copy link
Contributor

@takaaki7 takaaki7 commented Aug 2, 2017

Added a test that reproduces something-for-nothing fill bug #184 after hardfork555.

To put it briefly, following process reproduces the bug.

create_sell_order(core_seller, core.amount(1), test.amount(2));
create_sell_order(core_seller, core.amount(1), test.amount(2));
create_sell_order(core_buyer, test.amount(3), core.amount(1));

It's because maybe_cull_small_order() logic is deferred for a taker order. Even when amount_for_receive() become 0 for the taker, the order might be matched with other orders in case amount_for_sale() isn't 0.
(https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/db_market.cpp#L182-L203 )

As discussed in related issues, this should be fixed carefully.

@pmconrad pmconrad self-requested a review August 2, 2017 09:19
Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

Excellent! Tested to reproduce the broken behaviour.

One remark though: the test should fail, because the observed behaviour is a bug.

@takaaki7
Copy link
Contributor Author

takaaki7 commented Aug 2, 2017

I thought this behavior is expected for operations in the term between hardfork555 and a next hardfork that will fix this problem. But it's too complicated and I modified this to fail.

Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

Ok, thanks!

@oxarbitrage
Copy link
Member

nice work, tested. merging.

@oxarbitrage oxarbitrage merged commit b58389e into bitshares:develop Aug 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants