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 and update seed blocks up to block 390000 #324

Merged
merged 2 commits into from Jan 4, 2016

Conversation

Projects
None yet
2 participants
@zathras-crypto
Copy link

zathras-crypto commented Jan 1, 2016

  • Fixes MAX_SEED_BLOCK value (now set to 390000)
  • Fixes incorrect seed blocks between 370000 and 380000
  • Adds seed blocks between 380000 and 390000
  • Passes checkpoints (obtained without seed block filter)

edit from @dexX7 to auto-close related issues: resolves #319, resolves #322.

@dexX7 dexX7 added this to the 0.0.10.1 milestone Jan 1, 2016

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Jan 1, 2016

This branch is out-of-date with the base branch

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Jan 1, 2016

This branch is out-of-date with the base branch

Yeah, I messed up a little and hadn't updated my local branch, which also meant I wasn't actually verifying against the new 390,000 checkpoint (which turned up another seed block issue).

Fixed up & corrected, just running a testing pass now and then will push :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Jan 1, 2016

which turned up another seed block issue

Could you roughly outline how you generated them this time?

@zathras-crypto zathras-crypto force-pushed the zathras-crypto:0.0.10.1-Z-SeedBlocks390000 branch Jan 1, 2016

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Jan 1, 2016

OK, that should be it now - rebased and sorted the remaining seed block issues :)

To test, I use consensus_hash_every_block debug option and compare results from this PR vs disabled seed block filter.

Could you roughly outline how you generated them this time?

Sure - I cheated hehe... The hooks I was using were hacky and seemed to give bad results, so I just used Chest's database instead this time around... FYI I intend to revisit the upgraded consensus hashing PR and include in it the ability to generate seed block lists directly over RPC by querying levelDB so we've got a defined and re-usable process (to be honest I found this whole thing very long winded, manual and error prone this time - I don't remember it being like that last time I did it, but that's probably why last time's results were faulty! So having a coded up process for seed block generation along with a more in-depth checkpointing will make me feel a bit better about using seed blocks).

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Jan 1, 2016

Note, seed blocks 370,000 to 390,000 also pass checkpoints with the upgraded consensus hashing system. I'd like to add in the issuer address to the property data hashed and give it one more run through just to triple check before we merge.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Jan 1, 2016

to be honest I found this whole thing very long winded, manual and error prone this time - I don't remember it being like that last time I did it, but that's probably why last time's results were faulty!

If I'm not mistaken, then we both created seedblock lists and ended up with different results (mostly related to structurally valid/invalid IIRC etc.) and I manually added hooks and whatnot. :)

Having a more easy and straight forward way to deal with this would definitely be a big win!

I'd like to add in the issuer address to the property data hashed and give it one more run through just to triple check before we merge.

Please let me know once this is good to go. I'm currently still unable to test it myself, but given the effort and testing you're doing, I'm fine with a merge, especially since the CI server also provides additional feedback (and checkpoint tests) due to the reparsing. :)

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Jan 4, 2016

Quick note, this is good to go. I've verified the seed block lists using the new consensus hashing upgrades and it all still passes, so if there is still something amiss I'm unable to spot it :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Jan 4, 2016

Sorry, once again:

This branch is out-of-date with the base branch

.

zathras-crypto added some commits Jan 1, 2016

Fix and update seed blocks
- Fixes MAX_SEED_BLOCK value (now set to 390000)
- Fixes incorrect seed blocks between 370000 and 380000
- Adds seed blocks between 380000 and 390000
- Passes checkpoints (obtained without seed block filter)

@zathras-crypto zathras-crypto force-pushed the zathras-crypto:0.0.10.1-Z-SeedBlocks390000 branch to 2a90e9f Jan 4, 2016

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Jan 4, 2016

This branch is out-of-date with the base branch

Sorry, rebased :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Jan 4, 2016

Yeah, I really start to believe we should disable this check.. it's kinda annoying and stops the flow.

I'm going to merge this one, and then #327 (which has to be rebased then too..).

@dexX7 dexX7 merged commit 2a90e9f into OmniLayer:omnicore-0.0.10 Jan 4, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

dexX7 added a commit that referenced this pull request Jan 4, 2016

Merge pull request #324
2a90e9f Fix missing seed blocks between 380000 and 390000 (zathras-crypto)
99fb8c5 Fix and update seed blocks (zathras-crypto)

@dexX7 dexX7 changed the title Fix and update seed blocks Fix and update seed blocks up to block 390000 Jan 6, 2016

@zathras-crypto zathras-crypto deleted the zathras-crypto:0.0.10.1-Z-SeedBlocks390000 branch May 8, 2016

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