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

[RFC] Genesis ledger export #14213

Merged
merged 14 commits into from
Nov 6, 2023
Merged

Conversation

Sventimir
Copy link
Contributor

@Sventimir Sventimir commented Sep 26, 2023

Explain your changes:

  • Add an RFC on genesis ledger export as part of the hard fork procedure.

Explain how you tested your changes:

  • Read the document.

Closes the issue: MinaFoundation#95

@Sventimir Sventimir added RFC A label for RFCs (request for comments) hard fork MinaFoundation labels Sep 26, 2023
@Sventimir Sventimir self-assigned this Sep 26, 2023
@Sventimir Sventimir force-pushed the sventimir/genesis-ledger-export branch 2 times, most recently from 4f405db to f7a6f6b Compare September 26, 2023 10:00
rfcs/0050-genesis-ledger-export.md Outdated Show resolved Hide resolved
`compatible` and `berkeley` branches' configuration files are
compatible with each other (see: [PR #13768](https://github.com/MinaProtocol/mina/pull/13768)).

The `fork_config` field has been added to GraphQL in [PR #13787](https://github.com/MinaProtocol/mina/pull/13787).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the command gets best-tip data. I think we need height/slot as input as well to get the required protocol state- otherwise you have ~4mins before the best tip changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I realised recently that what I did to compute the previous blockchain length and global slot was wrong. I'm on my way to fix it. I'll open a PR shortly.

@Sventimir
Copy link
Contributor Author

I added a few more words on compatibility issues that arise from exporting epoch data and also from some recent changes to the berkeley branch.

@nholland94
Copy link
Member

@Sventimir In order to deal with the difference in epoch seed representation, we can just have a simple migration tool that is aware of both versions of the epoch seed representation, and convert between those 2. We can import the V1 type from compatible back into berkeley, and then just write a little function that maps between them, and package it up in a binary in the src/apps folder. A simple jq command (or python script) can process the json ledger as part of the hard fork package generation step, converting the epoch seeds using this custom executable.

@Sventimir
Copy link
Contributor Author

@Sventimir In order to deal with the difference in epoch seed representation, we can just have a simple migration tool that is aware of both versions of the epoch seed representation, and convert between those 2. We can import the V1 type from compatible back into berkeley, and then just write a little function that maps between them, and package it up in a binary in the src/apps folder. A simple jq command (or python script) can process the json ledger as part of the hard fork package generation step, converting the epoch seeds using this custom executable.

Looking at the changes to the relevant code (src/lib/mina_base/epoch_seed.ml and src/lib/data_hash_lib/data_hash.ml) I cannot find the root cause of the change of the encoding. It doesn't seem to lie in src/lib/snarky either, which means it must be even deeper, probably in the underlying Rust libraries. Is this correct? Do you know which dependency should I look into?

@Sventimir
Copy link
Contributor Author

Sventimir commented Oct 5, 2023

@Sventimir In order to deal with the difference in epoch seed representation, we can just have a simple migration tool that is aware of both versions of the epoch seed representation, and convert between those 2. We can import the V1 type from compatible back into berkeley, and then just write a little function that maps between them, and package it up in a binary in the src/apps folder. A simple jq command (or python script) can process the json ledger as part of the hard fork package generation step, converting the epoch seeds using this custom executable.

Looking at the changes to the relevant code (src/lib/mina_base/epoch_seed.ml and src/lib/data_hash_lib/data_hash.ml) I cannot find the root cause of the change of the encoding. It doesn't seem to lie in src/lib/snarky either, which means it must be even deeper, probably in the underlying Rust libraries. Is this correct? Do you know which dependency should I look into?

It turns out there's no incompatibility after all. It's just that I used the wrong base58_check function to serialize the seeds. Sadly the compiler is aware that EpochSeed.t and StateHash.t are the same type under the hood (they're not abstracted away properly) and so it couldn't catch this error,

@Sventimir Sventimir force-pushed the sventimir/genesis-ledger-export branch from 85f1515 to ba12db3 Compare October 6, 2023 06:26
@Sventimir
Copy link
Contributor Author

!ci-build-me

@Sventimir
Copy link
Contributor Author

!ci-build-me

@Sventimir
Copy link
Contributor Author

!ci-build-me

@deepthiskumar deepthiskumar self-assigned this Oct 16, 2023
Comment on lines 21 to 23
The genesis ledger export is achieved using a GraphQL field named
`fork_config`. This field, if asked for, contains a new runtime
configuration, automatically updated with:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are putting this in GraphQL, we should have an explicit required property for the implementation that handling this GraphQL request does not incur a long async job > 1 second. Our GraphQL server is not very well optimized, and we have had issues in the past where long running GraphQL tasks can actually break a node. We should be careful to make sure this GraphQL request is safe to make once implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, it does take a couple of seconds to complete. I don't know, how it could be helped though. GraphQL seems to me to be the only way to retrieve the data and it takes time to dump all the accounts in the ledger into JSON. We could try to partition this job into multiple GQL requests and assemble the JSON in a separate program/script, but this seems cumbersome. Another option would be to only enable this query if a certain runtime flag is passed, and instantly return null if it isn't. Not very elegant solution, but what else can we do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's that the account/ledger retrieval functions should be carefully implemented. But wondering if a CLI command would fare better? You'd still have to get the accounts the same way and serialize it to json but no GQL overhead? There already exists cli commands to export ledgers (mina ledger export)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue isn't that the GQL request can't take longer than 1sec, it's that it cannot incur a > 1 sec async cycle. There are logs for when this happens. To test this, just run the GQL query, and then inspect the logs to look for "long async cycle" or "long async job".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no such messages in the logs that I can find while generating the config.

`fork_config`. This field, if asked for, contains a new runtime
configuration, automatically updated with:

* the dump of the current **staged ledger**, which will become the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be the current staged ledger. As per the hard fork specification, we need to get the staged ledger of the final block before the transaction stop slot. This ledger will only be available for 2*k blocks after the transaction stop slot, and is not finalized until the network hits the stop network slot. Ideally, we would be able to send a GraphQL request of the form "give me the fork configuration for the latest block in the canonical chain where the slot is not greater than X" (and then provide the transaction stop slot as the value of X).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the staged ledger is not supposed to change anymore after the transaction stop slot. I think the network stop mechanism is implemented such that no more transactions are accepted, no more SNARK work is purchased and no coinbase rewards are paid anymore. Is this correct, @joaosreis ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, it doesn't matter which ledger do we take, all of them will be identical, won't they?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sventimir yes, that's correct. However, @nholland94 observations are also correct. We want to export the staged ledger of the final block before the transaction stop slot. We stop including any new transactions on succeeding blocks because those would not be included in the HF chain, but we keep producing those blocks so that we can achieve consensus on that final block before the stop transaction slot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay then, I'll add a parameter to the query, allowing the user to specify which block they're interested in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the RFC to reflect this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did:

Asking for this field requires providing a slot or a
state hash of the block that we want to base the exported ledger on.

(line 22-23)

It needs to be extended to return the blockchain state for a given block (height
or state hash) so that we can export the desired ledger after the
blockchain has moved on.

(line 51-54)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I forgot to push that change. Sorry.

rfcs/0050-genesis-ledger-export.md Outdated Show resolved Hide resolved
Comment on lines 67 to 74
The generated genesis ledger is prone to malevolent manual
modifications. Beyond containing the hash of the previous ledger, it's
unprotected from tampering with. However, at the moment there is no
mechanism which could improve the situation. The system considers
genesis ledger the initial state of the blockchain, so there is no
previous state it could refer to. Also, because we dump the **staged
ledger**, it is never snarked. It can only be verified manually by end
users, which is cumbersome at best.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be mitigated by providing the program which migrates the captured genesis ledger from the prior chain into the generated genesis ledger for the new chain. With this tool, users can re-execute the program themselves to verify the results, and read the logic of the program to verify it matches what was published by the generating party.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand. The genesis ledger file generated by the node is fed directly (except perhaps for manual setting the genesis ledger timestamp) into new nodes. No conversion is required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is referring to the case where there are ledger changes in the HF in which case you can't directly use the exported ledger. For example, adding/removing a field or any hash function changes changes the ledger hash of the genesis ledger in the HF. So you'd convert the exported ledger to the format that the HF daemon takes. This conversion can be verified by a program that takes the exported ledger, does the conversion itself and verify that the generated ledger has the same hash as the one used in. Assuming the program is simple enough to read and understand how the conversion is taking place. Of course, users can check their accounts to confirm that there are no balance or nonce changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Adeed in 6b1e886.

@Sventimir Sventimir force-pushed the sventimir/genesis-ledger-export branch from 41bedcd to 0dc8375 Compare October 31, 2023 08:17
@Sventimir
Copy link
Contributor Author

!ci-build-me

@Sventimir
Copy link
Contributor Author

!ci-build-me

@Sventimir Sventimir merged commit f5b0138 into develop Nov 6, 2023
2 checks passed
@Sventimir Sventimir deleted the sventimir/genesis-ledger-export branch November 6, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
berkeley-mainnet hard fork MinaFoundation RFC A label for RFCs (request for comments)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants