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

(WIP) Return headers as JSON array instead of concatenated string #109

Closed
wants to merge 9 commits into from

Conversation

JeremyRand
Copy link
Contributor

This PR makes the blockchain.block.headers method return a JSON array of headers instead of a concatenated string, which improves semantic correctness.

@JeremyRand
Copy link
Contributor Author

Marked as WIP since the protocol docs haven't been updated yet. This change was discussed on IRC last year and there wasn't any objection to the concept; not sure if any concerns might have appeared since that discussion. @SomberNight Is this something we could possibly roll into the protocol bump in #90 to minimize the number of protocol bumps?

@JeremyRand
Copy link
Contributor Author

(Also happy to squash this PR on request.)

@JeremyRand
Copy link
Contributor Author

Not sure if the coverage decrease is a dealbreaker, let me know if it is.

@SomberNight
Copy link
Member

Ok, we can do this as part of protocol 1.5.
IIRC you want this because namecoin has variable sized headers?

I guess it adds around 162/160-1=1.25% overhead (comma and space?) in the Bitcoin usecase when requesting lots of headers, but that is negligible seeing that we encode raw data as hex strings.

If you document the protocol change I can cherry pick such a commit to #90.

@JeremyRand
Copy link
Contributor Author

Ok, we can do this as part of protocol 1.5.

Sounds good.

IIRC you want this because namecoin has variable sized headers?

Yes, that's correct.

I guess it adds around 162/160-1=1.25% overhead (comma and space?) in the Bitcoin usecase when requesting lots of headers, but that is negligible seeing that we encode raw data as hex strings.

I think it's close to 3 bytes of asymptotic overhead per 160 bytes of hex (2 quotation marks and a comma; I'd expect no space character since presumably the JSON serializer is optimizing for compactness). But yes, close enough, I think the overhead is negligible.

If you document the protocol change I can cherry pick such a commit to #90.

Yes, I can do that.

@JeremyRand
Copy link
Contributor Author

@SomberNight Added a commit to change the protocol docs accordingly. I didn't touch the changelog since that would conflict with your branch; let me know if you want me to write a changelog entry.

@SomberNight
Copy link
Member

I think it's close to 3 bytes of asymptotic overhead per 160 bytes of hex (2 quotation marks and a comma

Ah I forgot about quotes..

I'd expect no space character since presumably the JSON serializer is optimizing for compactness

Hah. Don't assume, check! I did so now and whitespaces are there.
I've tried to regain the loss caused by this PR by stripping the spaces :P
kyuupichan/aiorpcX#38

@SomberNight
Copy link
Member

Please also update the "Example Response" in the doc some lines below.
(no need to keep an example for old protocol; one is enough for the new one)

@JeremyRand
Copy link
Contributor Author

Please also update the "Example Response" in the doc some lines below.
(no need to keep an example for old protocol; one is enough for the new one)

@SomberNight Done.

@SomberNight
Copy link
Member

I have incorporated the code changes into #101.
The doc changes are part of #90.

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

Successfully merging this pull request may close these issues.

3 participants