Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SIMD-0204: Slashable event verification #204
base: main
Are you sure you want to change the base?
SIMD-0204: Slashable event verification #204
Changes from 1 commit
4f69f15
c8b42fc
b980cb1
bd9d399
0d13374
472c2b4
c47359f
f4f8ab7
caf9058
f89a01b
245bd01
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't typically do this when upgrading a program via feature gate, but I like it a lot! We should definitely start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you intend for
S1ashing11111111111111111111111111111111111
to be owned by the upgradeable loader or loader-v4? If it's the former, we should also move the program-data account from the old address to the PDA forS1ashing11111111111111111111111111111111111
, to ensure that the program behaves like any other BPF program. cc @buffalojoecThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not familiar with the loaders so i'll defer to your wisdom :), just let me know which one makes the most sense and I can include it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with the default loader (bpf-loader-upgradeable) for now. We can amend it if loader-v4 becomes the default sooner than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specified the default loader and your comment about moving the program data account, let me know if it looks correct f4f8ab7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call out that it also needs the instructions sysvar for signature verification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using an
Option
, let's just always include this, with the caveat that the data atproof_account
might get modified. And let's add it beforeproof
to make it easier to findThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like it needs one more account -- the reporter to sign. But I wonder if we can allow this to be totally permissionless by having the reporter provide the lamport destination when they create the report. That way we're sure slash reports are always cleaned up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that approach, can definitely see people forgetting to close their reports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just send it back to the initial reporter? Or is it worth carrying around a separate address as the destination
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep them separate -- it gives extra flexibility and clarity at the cost of an extra 32 bytes, which shouldn't be too bad, especially considering the accounts will get cleaned up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separated it here f89a01b