-
Notifications
You must be signed in to change notification settings - Fork 97
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?
Conversation
67179d1
to
061cf5f
Compare
2d2ee08
to
339aa9a
Compare
339aa9a
to
4f69f15
Compare
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.
Really great work! Most of my comments are minor nits for clarification
2. Verify that the program account | ||
`8sT74BE7sanh4iT84EyVUL8b77cVruLHXGjvTyJ4GwCe` has a verified build hash of | ||
`<FILL IN AFTER IMPLEMENTATION>` [\[1\]](#notes) |
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
3. Copy the contents of `8sT74BE7sanh4iT84EyVUL8b77cVruLHXGjvTyJ4GwCe` into | ||
`S1ashing11111111111111111111111111111111111` |
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 for S1ashing11111111111111111111111111111111111
, to ensure that the program behaves like any other BPF program. cc @buffalojoec
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.
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
They can accomplish this via the `CloseProofReport` instruction which requires | ||
2 accounts: |
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.
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
5cf8254
to
0d13374
Compare
Proposal to deploy a program that records and verifies slashable events on chain
This proposal does not modify any stakes or rewards, the program will only verify and record infractions.
At the moment we only verify duplicate block violations, but a future SIMD will add voting violations as well