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 RPC commands and tests for block reorganizations #72

Merged
merged 5 commits into from
Apr 23, 2015

Conversation

dexX7
Copy link
Member

@dexX7 dexX7 commented Apr 23, 2015

This PR adds the required RPC calls (and some related) to test block reorganzations.

The tested transaction types and aspects of Omni Core are:

  • simple send
  • send to owners
  • property creation

There are two significant downsides: the "invalidateblock" command was introduced with Bitcoin Core 0.10, and is therefore not available on the CI server, but worse: I had no handy way to clear the memory pool, and added an extra extension for this.

All reorganization tests are skipped, if "clearmempool" is not available, or if "setgenerate" doesn't return a list of block hashes, indicating a client version, which is not based on 0.10.

I'm not sure, if this PR should be merged, given the context. Ideally, there would be a workaround to clear the memory pool, the client version properly detected, and tests ignored otherwise, instead of being skipped.

dexX7 added 5 commits April 23, 2015 14:53
Permanently marks a block as invalid, as if it violated a consensus rule. This can be used for block reorganization tests.
Removes invalidity status of a block and its descendants, reconsider them for activation. This can be used to undo the effects of "invalidateblock".
Return information about all known tips in the block tree, including the main chain as well as orphaned branches.
Clears the memory pool and returns a list of the removed transactions.
The tests require a client, which is based on Bitcoin Core 0.10, as well as the following RPC extension "clearmempool":

https://github.com/OmniLayer/omnicore/compare/omnicore-0.0.10...dexX7:oc-0.10-rpc-clearmempool?expand=1
msgilligan added a commit that referenced this pull request Apr 23, 2015
Add RPC commands and tests for block reorganizations
@msgilligan msgilligan merged commit 9e629df into OmniLayer:master Apr 23, 2015
@msgilligan
Copy link
Member

Maybe I should have read this more carefully before merging, but worse case I can @ignore the tests.

@msgilligan
Copy link
Member

Skipping the tests works just fine. I want to migrate the CI server to 0.10.0 soon, so that gets us part of the way there.

What's the likelihood of the clearmempool extension making it into either Omni Core or upstream (Bitcoin Core)?

I marked BaseReorgSpec as abstract: b00cdff This will prevent the test runner from running that class as a test.

@dexX7 Can you submit an additional PR with some of your notes from above in the JavaDoc comments for the new methods in BitcoinClient.java? (e.g. Which calls require 0.10.0 and which ones require your "extra extension")

@dexX7
Copy link
Member Author

dexX7 commented Apr 23, 2015

Reorganziation tests are important, and there are likely going to be more in the future, but I'd consider adding "clearmempool" as last resort, and I have some ideas for alternatives, which we might do in OmniJ directly.

@zathras-crypto: maybe you have an idea, how to "force" clearing the mempool? The idea I had was to explicitly "taint" transactions with coins from a block, which is later going to be invalidated, and to trigger mempool rejections as result, because the original source of coins would no longer be valid.

I'm going to push comment improvements tomorrow, and while we're at it: @msgilligan: Omni Core 0.0.10 has a bunch of new commands, such as "trade_MP", "getorderbook_MP", ... , and currently pending (actually waiting for more testing), also transaction creation commands for the missing transaction types. I didn't add them yet, because it's not clear to me, how different client versions are to be handled, but starting with clear comments is a good idea.

@msgilligan
Copy link
Member

Given that OmniJ is "pre-alpha" and the API can change "without notice", that Bitcoin 0.10.x has been available for some time now, and that the Omni Core testing "use case" of OmniJ will be focusing on 0.10.x very soon, it seems the general strategy should be to move to 0.10 APIs as quickly as possible.

Since we are still using 0.9.x on the CI server and Omni/Master Core 0.9.x is still in production, we should maintain backwards compatibility with 0.9.x in the RPC clients until 0.10.x ships.

This means we can add the new commands at any time as long as they're documented as requiring 0.10.x.

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.

2 participants