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

Dao fix reorg issues #2056

Merged
merged 5 commits into from Dec 5, 2018

Conversation

Projects
None yet
3 participants
@ManfredKarrer
Copy link
Member

ManfredKarrer commented Dec 5, 2018

No description provided.

ManfredKarrer added some commits Dec 4, 2018

Fix reorg issues
In case of reorgs there are several scenarios:
1. We have no snapshot yet: We start from genesis again
2. We have a snapshot and go back to that: We use last snapshot
3. We have a snapshot but the reorg does deeper so our last block in the
snapshot is invalid as well: -> we go back to genesis

There are many edge cases

There was one important bug fix with a == instead of an equals
comparison.

Added also the check that the first block need to be the genesis block.

Unfortunately the chainHeight is set to the genesis height initially
which is not right (should be 0 as we dont have any block and therefore
no chainHeight). To change that will be a bit risky, but for mainnet we
should consider it. There are several usages where a change might break
stuff, for instance the parameter handling.

@ManfredKarrer ManfredKarrer requested a review from sqrrm Dec 5, 2018

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Dec 5, 2018

utACK

@sqrrm
Copy link
Member

sqrrm left a comment

Please have a look at my comments on recursion

ArrayList<RawBlock> tempPendingBlocks = new ArrayList<>(pendingBlocks);
for (RawBlock tempPendingBlock : tempPendingBlocks) {
try {
doParseBlock(tempPendingBlock);

This comment has been minimized.

@sqrrm

sqrrm Dec 5, 2018

Member

This recursion seems a bit strange, it has to copy pendingBlocks into a temporary array for each recursion level, probably ok but a bit wasteful.

This comment has been minimized.

@ManfredKarrer

ManfredKarrer Dec 5, 2018

Member

Yes right I think that is wrong. It would iterate multiple times with lists which have been already applied. E.g. if you have 5 items and first recursive call copies the list then removes the current block from the source list and then copy the list again and start another loop with 4 itmes. Once that recursion is done the parent recursion would still iterate the remaining elements but those have been already processed. So either we add a check there if the block is still in the source list or we dont use a loop but only call with the first element (list is sorted) and so we avoid the remaining iterations from the current loop.
What do you think?

Show resolved Hide resolved core/src/main/java/bisq/core/dao/node/BsqNode.java
for (RawBlock tempPendingBlock : tempPendingBlocks) {
try {
doParseBlock(tempPendingBlock);
} catch (RequiredReorgFromSnapshotException e1) {

This comment has been minimized.

@sqrrm

sqrrm Dec 5, 2018

Member

Catching the RequiredReorgFromSnapshotException here and braking will stop this loop but if there was a loop in a previous level of recursion that would continue running and parsing in a now known bad state. I think the exception has to be rethrown here.

This comment has been minimized.

@sqrrm

sqrrm Dec 5, 2018

Member

Tested a bit and did a 6 block reorg by generating 10 blocks, invalidating the fourth of those blocks and generating 10 more blocks. Results in almost 8200 useless iterations.

It also seems to have broken the phase/cycle handling. Created an issue about it, shouldn't have since it's probably due to the reorg handling, #2064

This comment has been minimized.

@ManfredKarrer

ManfredKarrer Dec 5, 2018

Member

Good point about the RequiredReorgFromSnapshotException. I think if we use the first item instead of loop it should be ok. I will look into the other issue. Have not tested how it effect other aspects. We should make a client with all features used (requests, voting, bonds,...) and then apply a reorg to see if it break anything. Might be valid result if stuff become invalid by deep reorgs - e.g. < break, with < break it should not change results.

Show resolved Hide resolved core/src/main/java/bisq/core/dao/node/full/FullNode.java
@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Dec 5, 2018

I will merge as it is at least much better as current state. But will start a new PR with the recursion issue and we need to test more all other aspects with reorgs.

@ManfredKarrer ManfredKarrer merged commit 45a514e into bisq-network:master Dec 5, 2018

1 check passed

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

@ManfredKarrer ManfredKarrer deleted the ManfredKarrer:DAO-fix-reorg-issues branch Dec 5, 2018

@sqrrm sqrrm referenced this pull request Dec 29, 2018

Closed

For December 2018 #190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment