feat(pdp): BIG RENAME "proof set"->"data set", "root"->"piece" #569
Conversation
rjan90
added
the
team/fs-wg
| -- Check if migration has already been run (idempotency check) | ||
| DO $$ | ||
| BEGIN | ||
| IF EXISTS (SELECT 1 FROM information_schema.tables | ||
| WHERE table_schema = current_schema() | ||
| AND table_name = 'pdp_data_sets') THEN | ||
| RAISE NOTICE 'Migration already applied, skipping...'; | ||
| RETURN; | ||
| END IF; | ||
| END | ||
| $$; |
There was a problem hiding this comment.
Migrations are already guarded in harmonydb, this is redundant - https://github.com/filecoin-project/curio/blob/main/harmony/harmonydb/harmonydb.go#L271-L297
(To be fair we could improve the harmonydb guard - right now the base table prevents from a migration running twice, but it doesn't prevent from two nodes starting a migration at the same time - this could be solved with a second table, e.g. base_apply - before starting an update a node would look at that table, try to insert an entry with it's address - if that fails it means another node is running the upgrade, or that the upgrade has failed, and in either case the correct behavior is to sit there and wait for the entry to disappear + relevant base entry to appear)
There was a problem hiding this comment.
What happens if this is applied in a running cluster?
I'm all for the rename, but there are multiple live pretty serious clusters with the expectation that all upgrades can be performed live (tho admittedly that set doesn't really overlap with the set of nodes running PDP, for now).
Maybe for now this is fine, but in the future we need to build some mechanism which makes migrations like this possible - e.g. a feature/upgrade flags table, where each node goes and says e.g. "I support PDP name migration", then the cluster operator would roughly:
- Do a rolling upgrade of all nodes in the cluster
- All nodes would advertise support for the pdp upgrade
- The upgrade would trigger (automatically or manually) in some atomic way (e.g. cordoning tasks related to the feature flag)
- Additional mechanism could e.g. apply an implicit config layer which would be applied to nodes NOT supporting a certain feature flag enabled in the cluster (so that old nodes joining the cluster after a feature flag was enabled don't cause a mess)
|
Updated new PDP contract address in here and made it bork on mainnet PDP because that's not deployed yet (@rjan90 maybe we should get a matching mainnet one deployed too?). Re database -- one problem we have with this is that the data itself cannot be migrated because the new contract is not a plain upgrade of the old one with state copied across, so this unfortunately has to be coupled with a cleared database. Which is not going to be ideal for anyone on mainnet, but currently we lack any infrastructure that has a notion of matching proof sets ("data sets") to contracts, it's just one global contract. So we'll have to work on docs & messaging for that, perhaps also some warning or prompting .. somehow. |
|
Retargetted to the |
|
I've manually merged the changes from |
This is a rename to match the PDPVerifier contract that is about to be deployed. The main changes are the terminology of "proof set" becoming "data set" and "root" becoming "piece", but these have large flow-on effects.
I've split this into 3 commits, so it's possible to not go the whole way, although the last commit does contain additional cleanup as I kept on finding things, unfortunately, so if we opt for a part-way rename then I'd have to do a bit more reconciliation work. IMO just doing the full thing makes everything easier and we don't have weird mismatches anywhere in the stack. But I understand that it introduces a little more complication in the mkv2 migration but hopefully that's minimal since the rules are clear.
piece_cid->pieceCid andproofsetRoot->dataSetRoot(S`)) and opted for "SubRoot" and "subRoot", never "subroot". This commit does a fairly nice job of cleaning things up but it is breaking for clients. Ultimately in the SDK and all external interactions we want to entirely remove the old language, we don't want to see error traces or network logs showing "proofset" or "root".Unfortunately, as I mentioned above, this third commit also contains some non-database renames that I found and ended up getting squashed into that commit.
Going to leave this draft until we have a contract deployed and the ability to test it against it. I'd like to do full e2e testing with the SDK rename I'm working on in FilOzone/synapse-sdk#129.