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

Fix too-aggressive database clean in reorg event #368

Merged
merged 1 commit into from May 3, 2016

Conversation

Projects
None yet
2 participants
@zathras-crypto
Copy link

zathras-crypto commented May 3, 2016

I found during testing reorgs that the trade DB is cleared one block too far in the event a chain reorg occurs.

  • Block 101 : Trade ABC
  • Block 102 : Trade DEF
  • Block 102 is disconnected and a new block is mined.

The deleteAboveBlock() calls execute, and trade DEF is correctly deleted from the trades database. Unfortunately so is trade ABC.

The cause is related to the whether the deleteAboveBlock() calls are inclusive. This PR fixes the issue above, standardizes the behaviour of the corresponding function for the STO database, and updates the commenting to make things a bit clearer.

@zathras-crypto

This comment has been minimized.

Copy link
Author

zathras-crypto commented May 3, 2016

Should have mentioned, not consensus affecting (these databases are used to provide history on the RPC layer).

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 3, 2016

Interesting, and very good catch!

We have reorg tests for STO, so I'm very curious what happens here.

@zathras-crypto

This comment has been minimized.

Copy link
Author

zathras-crypto commented May 3, 2016

Interesting, and very good catch!

Thanks, I picked it up in my latest tests fixing up fee rollbacks (083d80e)...

We have reorg tests for STO, so I'm very curious what happens here.

Awesome :) The code change shouldn't be a behaviour change, it'll be interesting to see!

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 3, 2016

The code change shouldn't be a behaviour change, it'll be interesting to see!

How is that? How can it not be a behavior change?

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 3, 2016

Only the MetaDEx unit test related tests failed during the CI run, so the STO reorg tests pass.

It's not yet clear to me: is there a blind spot, and the initial behavior, which was fixed, can be tested?

We actually have a test "Historical STO transactions are not affected by reorganizations", which creates two STOs, reverts the last one, and confirms the effect of the first one is still valid:

https://github.com/OmniLayer/OmniJ/blob/master/omnij-rpc/src/integ/groovy/foundation/omni/test/rpc/reorgs/SendToOwnersReorgSpec.groovy#L108

@zathras-crypto

This comment has been minimized.

Copy link
Author

zathras-crypto commented May 3, 2016

How is that? How can it not be a behavior change?

This is for the STO database. Let's say nHeight = 100.

Before:
Called deleteAboveBlock() with nHeight -1, check uses <=
Eg : if (block_of_record <= 99)
After:
Called deleteAboveBlock() with nHeight, check uses <
Eg : if (block_of_record < 100)

Is the phrasing I used bad practice?

@zathras-crypto

This comment has been minimized.

Copy link
Author

zathras-crypto commented May 3, 2016

It's not yet clear to me: is there a blind spot, and the initial behavior, which was fixed, can be tested?

Sure, build without this patch, spin up regtest, execute a trade, mine a block, execute another trade, mine another block, roll back the chain one block.

If you check omnicore.log you'll see that both trades were deleted from the trades database instead of just the second one.

Sorry I did have a bash regtest script for this but I forgot to copy it before I butchered it for a different test :(

@zathras-crypto

This comment has been minimized.

Copy link
Author

zathras-crypto commented May 3, 2016

We actually have a test "Historical STO transactions are not affected by reorganizations"

I love the quality of these tests, that's fantastic :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 3, 2016

This is for the STO database.

Ahh, I see. So there was only an issue with the MetaDEx database, and the submission also adjusts the behavior of the STODB, so it's the same for all. Now I got it.. hehe. :)

Sorry I did have a bash regtest script for this but I forgot to copy it before I butchered it for a different test :(

No worries, and no need. It's clear to me now. I was just wondering, whether the STO test linked above could be faulty.

As a note to myself: would be useful to do automated reorg tests for the MetaDEx also. :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 3, 2016

I tested this PR locally once more, and it's all good. Thanks!

@dexX7 dexX7 merged commit 33f1853 into OmniLayer:omnicore-0.0.10 May 3, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

dexX7 added a commit that referenced this pull request May 3, 2016

Merge pull request #368
33f1853 Fix record deletion in event of reorg (zathras-crypto)

@dexX7 dexX7 modified the milestone: 0.0.10.1 May 17, 2016

@zathras-crypto zathras-crypto deleted the zathras-crypto:0.0.10-Z-FixDBReorgProtection branch Jul 4, 2017

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.