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

getblockchaininfo: make bip9_softforks an object, not an array. #7863

Merged
merged 2 commits into from Apr 14, 2016

Conversation

Projects
None yet
6 participants
@rustyrussell
Contributor

rustyrussell commented Apr 12, 2016

We can't change "softforks", but it seems far more logical to use tags
in an object rather than using an "id" field in an array.

For example, to get the csv status before, you need to iterate the
array to find the entry with 'id' field equal to "csv":

jq '.bip9_softforks | map(select(.id == "csv"))[] | .status'

Now:
jq '.bip9_softforks.csv.status'

There is no issue with fork names being incompatible with JSON tags,
since we're selecting them ourselves.

Signed-off-by: Rusty Russell rusty@rustcorp.com.au

@NicolasDorier

This comment has been minimized.

Show comment
Hide comment
@NicolasDorier

NicolasDorier Apr 12, 2016

Member

it probably broke py tests

Member

NicolasDorier commented Apr 12, 2016

it probably broke py tests

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 12, 2016

Contributor

The array also implies that id may not be unique.

utACK bc1ebf5.

Contributor

dcousens commented Apr 12, 2016

The array also implies that id may not be unique.

utACK bc1ebf5.

@NicolasDorier

This comment has been minimized.

Show comment
Hide comment
@NicolasDorier

NicolasDorier Apr 12, 2016

Member

I don't strongly oppose, but using an 'id' is easier for parsers who use class reflection (every statically typed language does it) for parsing the JSON. (In .NET for example, you don't have to parse by hand, you just create your class with an "id" field and that's it)

Member

NicolasDorier commented Apr 12, 2016

I don't strongly oppose, but using an 'id' is easier for parsers who use class reflection (every statically typed language does it) for parsing the JSON. (In .NET for example, you don't have to parse by hand, you just create your class with an "id" field and that's it)

@rustyrussell

This comment has been minimized.

Show comment
Hide comment
@rustyrussell

rustyrussell Apr 12, 2016

Contributor

Thanks @NicolasDorier it did break tests. It's exactly this kind of code which is why I think the format is ins... sub-optimal:

-    for row in info['bip9_softforks']:
-        if row['id'] == key:
-            return row
-    raise IndexError ('key:"%s" not found' % key)
+    return info['bip9_softforks'][key]
Contributor

rustyrussell commented Apr 12, 2016

Thanks @NicolasDorier it did break tests. It's exactly this kind of code which is why I think the format is ins... sub-optimal:

-    for row in info['bip9_softforks']:
-        if row['id'] == key:
-            return row
-    raise IndexError ('key:"%s" not found' % key)
+    return info['bip9_softforks'][key]
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 12, 2016

Contributor

@rustyrussell agreed, for selecting it changes the lookup complexity to O(1) (not that performance matters, but, it certainly looks/feels nicer)
@NicolasDorier surely those languages can derive their field from the key, maybe through a custom constructor?

Contributor

dcousens commented Apr 12, 2016

@rustyrussell agreed, for selecting it changes the lookup complexity to O(1) (not that performance matters, but, it certainly looks/feels nicer)
@NicolasDorier surely those languages can derive their field from the key, maybe through a custom constructor?

@NicolasDorier

This comment has been minimized.

Show comment
Hide comment
@NicolasDorier

NicolasDorier Apr 12, 2016

Member

@dcousens static languages derive their field the shape of the JSON. Changing it that way will mean there will be one different field for each fork proposal instead of a simple non typed array.

But by thinking about it is in reality it is not so much a problem, because the list of bip9 is static anyway, so it makes sense for a static language to have a different field for each different fork proposal.

So now I'm rather neutral about this change.

Member

NicolasDorier commented Apr 12, 2016

@dcousens static languages derive their field the shape of the JSON. Changing it that way will mean there will be one different field for each fork proposal instead of a simple non typed array.

But by thinking about it is in reality it is not so much a problem, because the list of bip9 is static anyway, so it makes sense for a static language to have a different field for each different fork proposal.

So now I'm rather neutral about this change.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Apr 12, 2016

Contributor

@NicolasDorier keeping the id field seems like it would entirely be a special case just for libraries that automatically parse the JSON into class objects.
For those that might access the JSON using a more traditional approach json["key"]["sub-key"] (etc), it just becomes a tedium.

I've only ever used the RPC using Python/Javascript and C++, all of which that traditional approach ruled supreme, and with which searching the JSON Array would be a tedium.

Contributor

dcousens commented Apr 12, 2016

@NicolasDorier keeping the id field seems like it would entirely be a special case just for libraries that automatically parse the JSON into class objects.
For those that might access the JSON using a more traditional approach json["key"]["sub-key"] (etc), it just becomes a tedium.

I've only ever used the RPC using Python/Javascript and C++, all of which that traditional approach ruled supreme, and with which searching the JSON Array would be a tedium.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 12, 2016

Member

Travis error unrelated: #7846

Member

MarcoFalke commented Apr 12, 2016

Travis error unrelated: #7846

@NicolasDorier

This comment has been minimized.

Show comment
Hide comment
@NicolasDorier

NicolasDorier Apr 12, 2016

Member

@dcousens well, when one solution improve the life of some people, it makes more difficult the life of other. Should we make life easier for hackish tool to parse json or make easier for parser library for static language is a question without responses.

What you call "traditional approach" is different for both of us, as we code in different environments.

But by thinking about it I don't think it makes life much difficult for parser libs with generated class objects after all.

Member

NicolasDorier commented Apr 12, 2016

@dcousens well, when one solution improve the life of some people, it makes more difficult the life of other. Should we make life easier for hackish tool to parse json or make easier for parser library for static language is a question without responses.

What you call "traditional approach" is different for both of us, as we code in different environments.

But by thinking about it I don't think it makes life much difficult for parser libs with generated class objects after all.

if row['id'] == key:
return row
raise IndexError ('key:"%s" not found' % key)
return info['bip9_softforks'][key]

This comment has been minimized.

@btcdrak

btcdrak Apr 12, 2016

Member

Could you change this to preserve the IndexError exception on key not found, null.

@btcdrak

btcdrak Apr 12, 2016

Member

Could you change this to preserve the IndexError exception on key not found, null.

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 12, 2016

Member

It should throw a KeyError, which is fine. Am I missing something?

@MarcoFalke

MarcoFalke Apr 12, 2016

Member

It should throw a KeyError, which is fine. Am I missing something?

This comment has been minimized.

@btcdrak

btcdrak Apr 12, 2016

Member

@MarcoFalke even better. The point is to preserve the previous behaviour of throwing a meaningful exception rather than leaving it to Python which results in Unexpected exception caught during testing: 'xxxx'.

@btcdrak

btcdrak Apr 12, 2016

Member

@MarcoFalke even better. The point is to preserve the previous behaviour of throwing a meaningful exception rather than leaving it to Python which results in Unexpected exception caught during testing: 'xxxx'.

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 12, 2016

Member

Oh, actually this is not the issue of the code here. To get a better error message, you'd have to change str into repr in this line:

print("Unexpected exception caught during testing: "+str(e))

@MarcoFalke

MarcoFalke Apr 12, 2016

Member

Oh, actually this is not the issue of the code here. To get a better error message, you'd have to change str into repr in this line:

print("Unexpected exception caught during testing: "+str(e))

rustyrussell added some commits Apr 12, 2016

getblockchaininfo: make bip9_softforks an object, not an array.
We can't change "softforks", but it seems far more logical to use tags
in an object rather than using an "id" field in an array.

For example, to get the csv status before, you need to iterate the
array to find the entry with 'id' field equal to "csv":

   jq '.bip9_softforks | map(select(.id == "csv"))[] | .status'

Now:
   jq '.bip9_softforks.csv.status'

There is no issue with fork names being incompatible with JSON tags,
since we're selecting them ourselves.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rpc-tests: handle KeyError nicely in test_framework.py
btcdrak wrote this for me.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell

This comment has been minimized.

Show comment
Hide comment
@rustyrussell

rustyrussell Apr 13, 2016

Contributor

OK, found another place in the tests which cut & paste the same function. And added BTCDrak's error cleanup as suggested by @MarcoFalke as a separate commit on top.

Contributor

rustyrussell commented Apr 13, 2016

OK, found another place in the tests which cut & paste the same function. And added BTCDrak's error cleanup as suggested by @MarcoFalke as a separate commit on top.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak
Member

btcdrak commented Apr 13, 2016

tACK d12760b

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 13, 2016

Member

utACK

Member

MarcoFalke commented Apr 13, 2016

utACK

@@ -79,11 +79,7 @@ def generate_blocks(self, number, version, test_blocks = []):
def get_bip9_status(self, key):

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 13, 2016

Member

found another place in the tests which cut & paste the same function.

I'd rather have this duplicate code removed... You change stuff in one branch of the code and later notice you have missed to apply the patch to all other copies of the code.

@MarcoFalke

MarcoFalke Apr 13, 2016

Member

found another place in the tests which cut & paste the same function.

I'd rather have this duplicate code removed... You change stuff in one branch of the code and later notice you have missed to apply the patch to all other copies of the code.

This comment has been minimized.

@btcdrak

btcdrak Apr 13, 2016

Member

Yes, this can go since the function was moved to util.py @rustyrussell

@btcdrak

btcdrak Apr 13, 2016

Member

Yes, this can go since the function was moved to util.py @rustyrussell

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 13, 2016

Member

Concept ACK

Member

laanwj commented Apr 13, 2016

Concept ACK

@laanwj laanwj merged commit d12760b into bitcoin:master Apr 14, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Apr 14, 2016

Merge #7863: getblockchaininfo: make bip9_softforks an object, not an…
… array.

d12760b rpc-tests: handle KeyError nicely in test_framework.py (Rusty Russell)
85c807c getblockchaininfo: make bip9_softforks an object, not an array. (Rusty Russell)

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jun 22, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 11, 2016

Member

Removing label 'Needs backport'. Please discuss in #8186 about the backport.

Member

MarcoFalke commented Jul 11, 2016

Removing label 'Needs backport'. Please discuss in #8186 about the backport.

kyuupichan referenced this pull request in kyuupichan/BitcoinUnlimited Mar 10, 2017

Merge #7863: getblockchaininfo: make bip9_softforks an object, not an…
… array.

d12760b rpc-tests: handle KeyError nicely in test_framework.py (Rusty Russell)
85c807c getblockchaininfo: make bip9_softforks an object, not an array. (Rusty Russell)

@kyuupichan kyuupichan referenced this pull request Mar 10, 2017

Merged

Rpc backports #359

sickpig referenced this pull request in sickpig/BitcoinUnlimited Mar 31, 2017

Merge #7863: getblockchaininfo: make bip9_softforks an object, not an…
… array.

d12760b rpc-tests: handle KeyError nicely in test_framework.py (Rusty Russell)
85c807c getblockchaininfo: make bip9_softforks an object, not an array. (Rusty Russell)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7863: getblockchaininfo: make bip9_softforks an object, not an…
… array.

d12760b rpc-tests: handle KeyError nicely in test_framework.py (Rusty Russell)
85c807c getblockchaininfo: make bip9_softforks an object, not an array. (Rusty Russell)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7863: getblockchaininfo: make bip9_softforks an object, not an…
… array.

d12760b rpc-tests: handle KeyError nicely in test_framework.py (Rusty Russell)
85c807c getblockchaininfo: make bip9_softforks an object, not an array. (Rusty Russell)

codablock added a commit to codablock/dash that referenced this pull request Dec 20, 2017

Merge #7863: getblockchaininfo: make bip9_softforks an object, not an…
… array.

d12760b rpc-tests: handle KeyError nicely in test_framework.py (Rusty Russell)
85c807c getblockchaininfo: make bip9_softforks an object, not an array. (Rusty Russell)

@dgenr8 dgenr8 referenced this pull request Sep 23, 2018

Open

bip9 cherries #488

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