Make ECChain opaque [][]byte in GBPFT #149
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
What if we just agreed on opaque byte slices and let the client deal with all the serialization and deserialization? This package doesn't actually care about the structure of the data, correct? I.e.: type ECChain [][]byte |
It might care about power table in there |
|
Is there a specific reason for not removing the redundant data from phases other than QUALITY? That seems like a more optimal solution IMHO. |
IMO, it shouldn't. |
Right, the protocol code never inspects the CID, and doesn't generally have access to the corresponding (non-finalised, or non-lookback) table. So @Stebalien 's suggestion is pretty good – I will try it out.
We can still do that. I'm not not doing that, if you know what I mean. But it's a much larger change. We can try that after this, if we think it's worthwhile. |
Thank you, yes I think it is worth it. |
anorth
changed the title
@Stebalien @Kubuxu @masih I've implemented this now. Please see updated PR description and re-review. |
…set ID. Changes the TipsetID type to be [][]byte, removing an additional serialization and making it unusable as a map key.
masih
approved these changes
Apr 9, 2024
| // - a commitment to the resulting power table, | ||
| // - a commitment to additional derived values. | ||
| // However, GossipPBFT doesn't need to know anything about that structure. | ||
| type TipSet = []byte |
There was a problem hiding this comment.
I am curious: why type alias instead of type definition?
There was a problem hiding this comment.
cbor-gen was unable to generate correct (compiling) code for [][]byte. A type definition would have been slightly better, but this isn't a yak I'm prepared to shave.
|
|
||
| func (t TipSetID) Bytes() []byte { | ||
| return []byte(t.value) | ||
| } |
Stebalien
reviewed
Apr 9, 2024
Stebalien
reviewed
Apr 9, 2024
Stebalien
approved these changes
Apr 10, 2024
Stebalien
added a commit
that referenced
this pull request
May 14, 2024
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
Stebalien
added a commit
that referenced
this pull request
May 14, 2024
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
Stebalien
added a commit
that referenced
this pull request
May 14, 2024
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
Stebalien
added a commit
that referenced
this pull request
May 14, 2024
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
Stebalien
added a commit
that referenced
this pull request
May 14, 2024
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
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 17, 2024
* EVM-friendly TipSet and ECChain formats 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 * test marshal payload for signing * slightly optimize hashing * add a worst-case benchmark for marshalling payloads * address feedback * Fake marshaling for testing * pre-allocate when marshaling tipsets * combine signer/verifier and MarshalPayloadForSignature Into a single Signatures interface. * tipset marshal for signing test * Test bad merkle-tree paths
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Removes all internal structure from ECTipset and ECChain, making them entirely opaque. Adds a key type which just stringifies the bytes, ensuring that the entire value is used as a map key. Note that this doesn't yet exactly match the draft FIP (#148).
The internal structure of chain values now needs to be specified elsewhere, but GPBFT will never introspect it.
This also deleted some useful tests for the consistency of chains. When a new type is introduces at the integration layer, those tests should be restored there instead.
Previous PR description below (just introduces the key type) :
This also changes the TipsetID type to be
[][]byte, removing an additional serialization step and, by design, making it unusable as a map key. Note that this doesn't yet exactly match the draft FIP (#148).Review: please critique this design and suggest alternatives. There is an outstanding problem of generating a unique key for
[][]byte(for both keying and signing) that will incorporate the slice boundaries. Ideas welcome.Closes #147.