-
Notifications
You must be signed in to change notification settings - Fork 6
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
EVM-friendly TipSet and ECChain formats #216
Conversation
6d72d02
to
efdd9e8
Compare
This patch implements the changes described in filecoin-project/FIPs#1004. Unfortunately, go-f3 now needs to be aware of the internal structure of `TipSet` objects as it sends them over the wire in one format, but signs another. This effectively reverts #149. Specific changes: 1. On the wire (and in finality certificates), `TipSet` objects are now directly cbor-marshaled within the payload instead of first being marshaled to byte strings. 2. Instead of directly covering the entire tipset list with the payload signature: 1. `TipSet` objects are "marshaled for signing" per the FIP change proposed above. 2. These marshaled tipset objects are stuffed into a merkle tree. 3. The merkle tree root is signed (along with the payload's other fields), again according the proposed changes. fixes #166
A few TODOs (needs a few more tests) but should otherwise be ready for review. |
Hm. Apparently this is killing performance (probably all the hashing). We may need to make that pluggable (e.g., for testing). |
Welp. Sha256 is 5x faster on my machine. |
If I look at the total time for
|
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.
Partial review, not done yet
gpbft/chain.go
Outdated
|
||
func (ts *TipSet) MarshalForSigning() []byte { | ||
var buf bytes.Buffer | ||
_ = cbg.WriteByteArray(&buf, ts.TipSet) |
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.
Why is this using cbg
? Just buf.Write
?
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 actually not sure where the wires got crossed in the FEVM FIP. IMO, it should have either been CBOR([Cid, Cid, Cid])
or Raw(Cid || Cid || Cid)
. Instead we got CBOR(Cid || Cid || Cid)
.
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.
Honestly, I'm tempted to change it to CBOR([Cid, Cid, Cid])
because then we'd be able to dereference the CIDs. E.g.,$TIPSET_CID/0
would refer to the CID of the first block. But if we did that, we'd probably (eventually) want a small FIP to make FEVM use the new format.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #216 +/- ##
==========================================
- Coverage 73.19% 71.55% -1.64%
==========================================
Files 30 31 +1
Lines 2130 2338 +208
==========================================
+ Hits 1559 1673 +114
- Misses 446 535 +89
- Partials 125 130 +5
|
@@ -46,3 +46,7 @@ func (b *BLSBackend) GenerateKey() (gpbft.PubKey, any) { | |||
b.signersByPubKey[string(pubKeyB)] = blssig.SignerWithKeyOnG1(pubKeyB, priv) | |||
return pubKeyB, priv | |||
} | |||
|
|||
func (b *BLSBackend) MarshalPayloadForSigning(nn gpbft.NetworkName, p *gpbft.Payload) []byte { | |||
return p.MarshalForSigning(nn) |
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 actually marshaling here because, in this case, I assume we don't really care about performance.
@@ -84,3 +85,37 @@ func (s *FakeBackend) VerifyAggregate(payload, aggSig []byte, signers []gpbft.Pu | |||
} | |||
return nil | |||
} | |||
|
|||
func (v *FakeBackend) MarshalPayloadForSigning(nn gpbft.NetworkName, p *gpbft.Payload) []byte { |
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 skips all hashing. We'll eventually hash with sha256 when signing, but only doing that once is much faster.
gpbft/api.go
Outdated
@@ -92,4 +92,6 @@ type Host interface { | |||
Signer | |||
Verifier | |||
DecisionReceiver | |||
|
|||
MarshalPayloadForSigning(*Payload) []byte |
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 added this as a new function on the base Host
because it doesn't really fit on either the Signer
or the Verifier
. I can also create a new SignatureOps
(or something) interface that combines Signer
, Verifier
, and MarshalPayloadForSigning
.
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 merged it into a Signatures
interface. Open to suggestions on better names.
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.
Bikeshedding on name; alternative suggestions:
Signing
,SignerVerifier
- (don't hate me) separate all the sign related stuff into their own package called
signing
and call the interfaceScheme
then ingpbft
it would read `signing.Scheme
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'll leave 3 for a future patch. Between 1, 2, and the current name... I don't have any preference.
@@ -400,7 +405,7 @@ func (i *instance) validateMessage(msg *GMessage) error { | |||
} | |||
|
|||
// Check vote signature. | |||
sigPayload := msg.Vote.MarshalForSigning(i.participant.host.NetworkName()) | |||
sigPayload := i.participant.host.MarshalPayloadForSigning(&msg.Vote) |
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 turned out to be "not terrible" because we already needed the host for the network name.
pt.host.On("NetworkName").Return(pt.networkName).Maybe() | ||
pt.host.On("MarshalPayloadForSigning", mock.AnythingOfType("*gpbft.Payload")). | ||
Return([]byte(gpbft.DOMAIN_SEPARATION_TAG + ":" + pt.networkName)) | ||
|
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.
Is this the right way to do this? Basically:
- We may not call
NetworkName
now, because that's handled inside ofHost.MarshalPayloadForSigning
. - Whenever we call
Sign
, we callHost.MarshalPayloadForSigning
.
Into a single Signatures interface.
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 solid. I suggest waiting for one more peer review on this one, though.
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.
Late but LGTM
This patch implements the changes described in
filecoin-project/FIPs#1004.
Unfortunately, go-f3 now needs to be aware of the internal structure of
TipSet
objects as it sends them over the wire in one format, but signs another. This effectively reverts #149.Specific changes:
TipSet
objects are now directly cbor-marshaled within the payload instead of first being marshaled to byte strings.TipSet
objects are "marshaled for signing" per the FIP change proposed above.fixes #166
fixes #150