-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
slashing: add duplicate block proof verification #7418
slashing: add duplicate block proof verification #7418
Conversation
} | ||
|
||
#[test] | ||
fn test_solana_shred_parity() { |
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.
This file is essentially copied from shred::layout
in agave. This test ensures parity.
Not sure if there's a better way that will allow code reuse 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.
Considering we've been working on breaking up the SDK, all of the shred structures could probably use the same treatment, with some sort of solana-shred
crate that can be used onchain.
I don't think the crate would belong in sdk/
in the monorepo, since it's a very niche usecase.
It doesn't need to land with this PR, of course, but what do you think about that approach?
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.
That makes sense, i'm okay with that approach. Are you also suggesting that the monorepo uses solana-shred
? or this is just a mirror for onchain programs.
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.
The monorepo would use it too. I can get a PR together for that split-up if you like
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'm okay with that approach. @behzadnouri wdyt about a solana-shred
crate that copies the data structures and layout
functions for use in solana_ledger
as well as the slashing program onchain?
We can also make this split after this PR lands.
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! The base logic looks solid, and the next steps for signature verification and creating an account are very clear.
Most of the comments are pretty minor, to make this more performant.
} | ||
|
||
#[test] | ||
fn test_solana_shred_parity() { |
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.
Considering we've been working on breaking up the SDK, all of the shred structures could probably use the same treatment, with some sort of solana-shred
crate that can be used onchain.
I don't think the crate would belong in sdk/
in the monorepo, since it's a very niche usecase.
It doesn't need to land with this PR, of course, but what do you think about that approach?
269f41d
to
a3d74ab
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.
This is looking really close! A couple last bits on my side, then we can get this in
Cargo.lock
Outdated
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.
Oh sorry, one last bit, can you revert these changes so that the PR only picks up the new crate that you're adding? There shouldn't be too many other changes past that
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.
Done 74cdd12
8ed365b
to
09235da
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.
Looks really great, thanks for integrating all the feedback!
And sorry, I didn't mean to totally revert Cargo.lock -- can you just run cargo tree
from the root and commit that? Once it passes CI, all good from my side
Pull request has been modified.
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.
Great work!
Initial change for duplicate block slashing as part of solana-foundation/solana-improvement-documents#204
Users should create a proof account with data storage of at least 2512 bytes (2 * PACKET_DATA_SIZE + struct overhead) and populate it with the 2 shreds that comprise the proof in the format of
DuplicateBlockProofData
.The program supports 1 instruction
DuplicateBlockProof
which takes a proof account, an offset to read from, the slot in which a duplicate block was broadcasted, and the pubkey of the offending leader.After verification and deserialization of the proof account, we check if any of the following conditions are met which invalidate the proof:
We then verify if the proof is valid. A proof consisting of shred1 and shred2 is valid if any of the following conditions are met:
At the moment we only log the results of the proof verification. Future work will store these results in a context state account for use by the runtime.
Signature verification is not implemented in this change. A future change will use instruction introspection to verify that both shreds were signed by the
node_pubkey
.