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 support for ecosystem specific consensus hashes #502

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@zathras-crypto

zathras-crypto commented Nov 7, 2017

This PR adds support for ecosystem specific consensus hashes.

The purpose is to allow the quick verification of consensus for a specific ecosystem rather than for the entire state. For example clients may allow new transaction types such as UIT in the test ecosystem which would cause a differing consensus hash between clients when the whole state is considered. With this amendment we can thus generate a hash specifically for the main (or test) ecosystem.

The default is still to generate a hash that covers both ecosystems, but an optional ecosystem parameter can be supplied with the consensus hash RPCs that will specify an ecosystem.

Examples:

omni_getcurrentconsensushash

{
  "block": 493420,
  "blockhash": "0000000000000000001e2dec9f2d1ba3315c5a6e661e2049bedd7f3dd72d83cd",
  "ecosystem": "both",
  "consensushash": "4a8193fbba907fbba52395e52fab375c2239f53902ae34cd54a057b2a511414b"
}
omni_getcurrentconsensushash 1

{
  "block": 493420,
  "blockhash": "0000000000000000001e2dec9f2d1ba3315c5a6e661e2049bedd7f3dd72d83cd",
  "ecosystem": "main",
  "consensushash": "df26c1ad23ee6dd912fc2c7cf91996661afefd175045448cc2d42c435823c489"
}
omni_getcurrentconsensushash 2

{
  "block": 493420,
  "blockhash": "0000000000000000001e2dec9f2d1ba3315c5a6e661e2049bedd7f3dd72d83cd",
  "ecosystem": "test",
  "consensushash": "2192cbdf25809d15642cd1fe0443d432153be12bacb3c630b3aab423cd29fc83"
}

Thanks
Z

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Nov 7, 2017

Member

This is nice to have, but I somehow fail to see the use of it.

Isn't the global hash good enough, given that it also covers the desired ecosystem?

Member

dexX7 commented Nov 7, 2017

This is nice to have, but I somehow fail to see the use of it.

Isn't the global hash good enough, given that it also covers the desired ecosystem?

@dexX7 dexX7 added this to the Next release milestone Nov 7, 2017

@zathras-crypto

This comment has been minimized.

Show comment
Hide comment
@zathras-crypto

zathras-crypto Nov 8, 2017

This is nice to have, but I somehow fail to see the use of it. Isn't the global hash good enough, given that it also covers the desired ecosystem?

Probably easiest to explain with an example. Craig wants to run UIT in the test ecosystem before activation which means at least OW & OE would need to be running a client that supports UIT. Once UIT transactions are in the test ecosystem, clients that understand them are going to give a different consensus hash for the test ecosystem than clients that don't understand them.

The only way to make sure that the production ecosystem is unaffected and all these clients are still in production eco consensus is to split out the consensus hash.

Hopefully that makes sense! Let me know if it's still unclear and I'll try and be clearer dude :)

zathras-crypto commented Nov 8, 2017

This is nice to have, but I somehow fail to see the use of it. Isn't the global hash good enough, given that it also covers the desired ecosystem?

Probably easiest to explain with an example. Craig wants to run UIT in the test ecosystem before activation which means at least OW & OE would need to be running a client that supports UIT. Once UIT transactions are in the test ecosystem, clients that understand them are going to give a different consensus hash for the test ecosystem than clients that don't understand them.

The only way to make sure that the production ecosystem is unaffected and all these clients are still in production eco consensus is to split out the consensus hash.

Hopefully that makes sense! Let me know if it's still unclear and I'll try and be clearer dude :)

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Nov 10, 2017

Member

The only way to make sure that the production ecosystem is unaffected and all these clients are still in production eco consensus is to split out the consensus hash.

So this is just an extra safe guard?

Member

dexX7 commented Nov 10, 2017

The only way to make sure that the production ecosystem is unaffected and all these clients are still in production eco consensus is to split out the consensus hash.

So this is just an extra safe guard?

@zathras-crypto

This comment has been minimized.

Show comment
Hide comment
@zathras-crypto

zathras-crypto Nov 13, 2017

So this is just an extra safe guard?

Yeah it's just for verification. For example:

Client            Combined        Main Eco       Test Eco
---------------------------------------------------------------
0.0.12            mnop            abcd           efgh         
0.2               mnop            abcd           efgh         
0.2.99-UIT        qrst            abcd           ijkl         

zathras-crypto commented Nov 13, 2017

So this is just an extra safe guard?

Yeah it's just for verification. For example:

Client            Combined        Main Eco       Test Eco
---------------------------------------------------------------
0.0.12            mnop            abcd           efgh         
0.2               mnop            abcd           efgh         
0.2.99-UIT        qrst            abcd           ijkl         
@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Nov 14, 2017

Member

Alright!

Member

dexX7 commented Nov 14, 2017

Alright!

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Nov 23, 2017

Member

Sorry @zathras-crypto, looks like I created a conflict by merging the other PR first! Can you please rebase?

Member

dexX7 commented Nov 23, 2017

Sorry @zathras-crypto, looks like I created a conflict by merging the other PR first! Can you please rebase?

@dexX7 dexX7 modified the milestones: 0.3.0, Next release Dec 11, 2017

@dexX7 dexX7 added status: on hold and removed status: ready labels Jan 30, 2018

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Jan 30, 2018

Member

Given this PR is outdated and needs further review, it's going to be closed for now.

Member

dexX7 commented Jan 30, 2018

Given this PR is outdated and needs further review, it's going to be closed for now.

@dexX7 dexX7 closed this Jan 30, 2018

@dexX7 dexX7 removed this from the Next release milestone Aug 9, 2018

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