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

Remove archive_genesisHash, since we have chainSpec_V1_genesisHash already? #101

Closed
jsdw opened this issue Sep 18, 2023 · 4 comments
Closed

Comments

@jsdw
Copy link
Collaborator

jsdw commented Sep 18, 2023

I feel like this came up, but searching open/closed issues/PRs I only noticed #61 so maybe I had that in mind.

Anyway, is there any good reason to keep archive_genesisHash? It feels like we should remove it.

@tomaka
Copy link
Contributor

tomaka commented Sep 19, 2023

The point of having multiple identical functions with the same prefix is that the prefixed are all self-contained.
As mentioned in #61, if you're intending to use the archive function, you don't know whether the node you're connected to will also have chainSpec available.

Removing archive_genesisHash "because we have chainSpec_genesisHash" is not by itself a good motivation.

@jsdw
Copy link
Collaborator Author

jsdw commented Sep 19, 2023

Do you ever forsee a time in which a chain would expose archive methods and not chainSpec methods, though?

Removing archive_genesisHash "because we have chainSpec_genesisHash" is not by itself a good motivation.

I don't really agree; I'd see it more as "we shouldn't duplicate methods across prefixes unless there is a good enough reason to".

One argument for keeping it for me is just because archive_hashByHeight I guess allows you to enter 0 as a height to get it anyway, so the logic already needs to exist to make it fetchable already.

Either way, I don't care very much. It just seemed odd when I noticed it.

@tomaka
Copy link
Contributor

tomaka commented Sep 19, 2023

Do you ever forsee a time in which a chain would expose archive methods and not chainSpec methods, though?

A software that is simply plugged onto a RocksDB/ParityDB database (not a proper node, not connected to the peer-to-peer network, just reads from the database) wouldn't be able to serve chainSpec.

I don't really agree; I'd see it more as "we shouldn't duplicate methods across prefixes unless there is a good enough reason to".

Why not?
"Not duplicating" stuff isn't by itself a good reason. Duplicating code is generally frown upon because you might update one piece of code and forget to update the other, or things like that, but this doesn't apply here.

One argument for keeping it for me is just because archive_hashByHeight I guess allows you to enter 0 as a height to get it anyway, so the logic already needs to exist to make it fetchable already.

You mean a good reason to remove archive_genesisHash?
I think the reason why I've put archive_genesisHash in addition to archive_hashByHeight is because it returns a single value instead of an array.
But I'm fine with adding a paragraph to hashByHeight that mentions that in the case of 0 it must always return precisely one item.

@jsdw
Copy link
Collaborator Author

jsdw commented Sep 19, 2023

A software that is simply plugged onto a RocksDB/ParityDB database (not a proper node, not connected to the peer-to-peer network, just reads from the database) wouldn't be able to serve chainSpec.

Aah, ok for me, this is a good enough justification to me to keep archive_genesisHash; thanks for pointing that case out!

I think the reason why I've put archive_genesisHash in addition to archive_hashByHeight is because it returns a single value instead of an array.

I think I'm fine with this too; I only thought about archive_hashByHeight after I opened this issue, and so on the one hand it also provides the same functionality as archive_genesisHash (with the "only one block treturned at 0" caveat), buton the other hand archive_genesisHash can be seen as a nice, understandable alias to that and it'll always be easy to implement anyway given that archive_hashByHeight needs implementing.

So personally I'm all good here; I'm happy with things staying as they are. Thanks for your explanations!

@jsdw jsdw closed this as completed Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants