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.

Sign up for GitHub

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

Jump to bottom

Include polkadot version in artifact path #1828

Merged
merged 5 commits into from
Oct 15, 2023
Merged

Include polkadot version in artifact path #1828

merged 5 commits into from
Oct 15, 2023
+50 −26

Conversation

eagr
Copy link
Contributor

@eagr eagr commented Oct 9, 2023 •
edited
Loading

closes #695

Could potentially be helpful to preserving caches when applicable, as discussed in #685

kusama address: FvpsvV1GQAAbwqX6oyRjemgdKV11QU5bXsMg9xsonD1FLGK

@mrcnski mrcnski mentioned this pull request Oct 9, 2023
Merged
@eagr eagr marked this pull request as draft October 9, 2023 14:28
mrcnski
mrcnski approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

Thanks so much! If you'd like a tip and you have a Polkadot or Kusama address, you can add it to the PR body.

Format: {kusama|polkadot|localtest} address: <SS58 Address>
Doc: https://github.com/paritytech/substrate-tip-bot#pull-request-body
Example: #1720

Let me know if you want ideas for further contributions!

polkadot/node/core/pvf/src/artifacts.rs Outdated Show resolved Hide resolved
polkadot/node/core/pvf/src/artifacts.rs Outdated Show resolved Hide resolved
@mrcnski mrcnski added the T0-node This PR/Issue is related to the topic “node”. label Oct 9, 2023
@eagr
Copy link
Contributor Author

eagr commented Oct 9, 2023

What a pleasant surprise! I'll get an account right away.

Btw, I was trying to add some version check before the execution, but it feels better to put it in the cache purging instead (someday maybe). So any good next issue for me? @mrcnski

@eagr eagr marked this pull request as ready for review October 9, 2023 20:04
@eagr eagr requested a review from mrcnski October 9, 2023 20:06
@mrcnski
Copy link
Contributor

mrcnski commented Oct 10, 2023

What a pleasant surprise! I'll get an account right away.

Nice! I'll tip you on #1774 as it's bigger and more likely for a tip to pass the referendum.

Btw, I was trying to add some version check before the execution, but it feels better to put it in the cache purging instead (someday maybe). So any good next issue for me? @mrcnski

Cool! Since you're already working on this area, do you want to go ahead and implement artifact persistence? We would need the following:

  1. PVF: Avoid clearing the artifacts cache on restart #685
  2. PVF: Compromised artifact file integrity can lead to disputes #677 (comment) (hash after preparation, but only check it on node restart and not before every execution.)

@eagr
Copy link
Contributor Author

eagr commented Oct 11, 2023

Is there anything left unsolved here? I'd like to start working on those issues. It'd be if this could be merged first as they are closely related.

s0me0ne-unkn0wn
s0me0ne-unkn0wn approved these changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn left a comment

Choose a reason for hiding this comment

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

Looks good, I'd appreciate a review from @bkchr as I remember there were discussions about moving around the node version constant... @mrcnski or did you agree already?

@mrcnski
Copy link
Contributor

mrcnski commented Oct 11, 2023

Yeah, PR looks good but I would like to wait for @bkchr approval as well.

bkchr
bkchr approved these changes Oct 15, 2023
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Not the perfect place for the version. However, it is better than using an env variable.

@bkchr bkchr merged commit 9e14470 into paritytech:master Oct 15, 2023
9 of 12 checks passed
ordian added a commit that referenced this pull request Oct 16, 2023
@ordian
Merge branch 'master' into tsv-disabling-backing
c44093d 
* master: (54 commits)
  Publish `xcm-emulator` crate (#1881)
  Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849)
  Arkworks Elliptic Curve utils overhaul (#1870)
  Fix typos (#1878)
  fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176)
  Refactor staking ledger (#1484)
  Paired-key Crypto Scheme (#1705)
  Include polkadot version in artifact path (#1828)
  add link to rfc-0001 in broker README (#1862)
  Discard `Executor` (#1855)
  Macros to use path instead of ident (#1474)
  Remove clippy clone-double-ref lint noise (#1860)
  Refactor alliance benchmarks to v2 (#1868)
  Check executor params coherence (#1774)
  frame: use derive-impl for beefy and mmr pallets (#1867)
  sc-consensus-beefy: improve gossip logic (#1852)
  Adds instance support for composite enums (#1857)
  Fix links to implementers' guide (#1865)
  Disabled validators runtime API (#1257)
  Adding `try_state` hook for `Treasury` pallet (#1820)
  ...
ordian added a commit that referenced this pull request Oct 16, 2023
@ordian
Merge branch 'tsv-disabling-backing' into ao-disabling-statement-dist…
b67c19f 
…ribution

* tsv-disabling-backing: (54 commits)
  Publish `xcm-emulator` crate (#1881)
  Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849)
  Arkworks Elliptic Curve utils overhaul (#1870)
  Fix typos (#1878)
  fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176)
  Refactor staking ledger (#1484)
  Paired-key Crypto Scheme (#1705)
  Include polkadot version in artifact path (#1828)
  add link to rfc-0001 in broker README (#1862)
  Discard `Executor` (#1855)
  Macros to use path instead of ident (#1474)
  Remove clippy clone-double-ref lint noise (#1860)
  Refactor alliance benchmarks to v2 (#1868)
  Check executor params coherence (#1774)
  frame: use derive-impl for beefy and mmr pallets (#1867)
  sc-consensus-beefy: improve gossip logic (#1852)
  Adds instance support for composite enums (#1857)
  Fix links to implementers' guide (#1865)
  Disabled validators runtime API (#1257)
  Adding `try_state` hook for `Treasury` pallet (#1820)
  ...
@kiltbot kiltbot mentioned this pull request Nov 7, 2023
Closed
@ahmadkaouk ahmadkaouk mentioned this pull request Nov 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s0me0ne-unkn0wn s0me0ne-unkn0wn s0me0ne-unkn0wn approved these changes

@bkchr bkchr bkchr approved these changes

@mrcnski mrcnski Awaiting requested review from mrcnski

Assignees
No one assigned
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PVF: include polkadot's version into the artifact path
4 participants
@eagr @mrcnski @bkchr @s0me0ne-unkn0wn