Merge Signer-indices to master #2414
Conversation
* update interface * remove IdentitiesByIndices * remove TODO * address review comments * [Consensus] SignerIndices Optimization (2/4) (#2101) * implement IdentitiesByIndices * convert signer index * implement encoding and decoding * add test cases * remove signer indices file * fix Packer * add comments * remove IdentitiesByIndices * update mock * fix test cases * add comments * remove fields * fix ParentVoterIDs * fix verifier * fix mock * implement staking vote progressor * fix tests * move to module/packer * fix tests * fix validator test * fix validator test * use ParentVoterIndices * add QuorumCertificateWithSignerIDs * update blockSummary * move module * add test cases * remove comments * add check to EncodeSignerIndices * Apply suggestions from code review Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> * Apply suggestions from code review Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> * Apply suggestions from code review Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> * [Consensus] Use SignerIndices in CollectionGuarantee (3/4) (#2140) * implement IdentitiesByIndices * implement encoding and decoding * remove IdentitiesByIndices * use ParentVoterIndices * use chainID * add ChainID * fix test case * remove file * sort assignment to canonical order * fix guarantee * fix execution ingestion core * use signer indices * add identifierOrder * use guarantor.FindGuarantors * fix tests * fix tests * fix tests * disable tests * fix tests * fix tests, add validation * fix fvm test * fix access tests * fix tests * fix tests * fix select_filter_test tests * fix access tests * fix access integration * fix access integration tests * revert fvm_test changes * update fvm_test * fix exec tests * fix ingestion engine tests * fix test cases * remove todo * fix error handling * add comment * fix tests * update logging * refactor * fix tests * remove function * fix error wrapping * update comment * update tests * remove unverified signature * revert mutator tests change * update comment * address comments * add tests * add check in NewClusterList to ensure the assignments are sorted in canonical order * rename method * consensus ingestion core log signer indices * remove tests log * refactor canonical order check * replace order.ByNodeIDAsc with order.Canonical * [Consensus] Refactor for guarantee signer indices (4/4) (#2204) * wip * wip * starting to fix tests * adding tests * happy path test * Added toDo for fixing tests for unhappy paths * • fixed packer tests • consolidated logic for checking the padded bits * re-gen mocks * fix validPadding * fix findguarantors * refactor ingestion/core.go * move FindGuarantors to protocol * remove commented code * fix name * fix tests * small refactor * fix import * Apply suggestions from code review Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> * fix error type * fix type * fix error message * update comments * update comment * update tests * fix identity_test * fix tests * fix unittest * fix cluster tests * fix tests * fixtures ingestion engine tests * fix execution_test * fix consensus inclusion tests * fix bootstrap constraint check * fix cycle dep from NewClusterList (#2225) Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
zhangchiqing
requested review from
vishalchangrani,
yhassanzadeh13,
ramtinms,
janezpodhostnik and
m4ksio
as code owners
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit 2e8c9af The command Bench tests were run a total of 7 times on each branch. Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov Report
@@ Coverage Diff @@
## master #2414 +/- ##
==========================================
- Coverage 56.94% 56.64% -0.31%
==========================================
Files 653 656 +3
Lines 60019 60156 +137
==========================================
- Hits 34180 34075 -105
- Misses 22913 23146 +233
- Partials 2926 2935 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
First set of comments. Didn't get through the PR entirely.
Mostly stylistic suggestion and typos. The only noteworthy thing is the error handling of the checksum for the signer indices. We are just returning generic errors, which makes it impossible to identify byzantine inputs as such.
I have added the error handling in this PR (targeting your branch) and fixed a few random typos: #2443
| @@ -49,7 +47,7 @@ func UnpackRandomBeaconSig(sigData []byte) (crypto.Signature, error) { | |||
| packer := SigDataPacker{} | |||
| sig, err := packer.Decode(sigData) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("could not decode sig data %s: %w", err, model.ErrInvalidFormat) | |||
| return nil, NewInvalidFormatErrorf("could not decode sig data: %w", err) | |||
There was a problem hiding this comment.
The API here seems a bit inconsistent:
- in method
UnpackRandomBeaconSigwe return aInvalidFormatErrorif the decoding fails - in method
Decode, we directly return the decoder's error
Suggestions:
- Would be nice to make this a bit more uniform and also return a
InvalidFormatErrorinDecode - I feel
InvalidFormatErroris an expected error during normal operations (indicating byzantine input). Would be great to document this\\ During normal operations, the following error are expected: \\ * InvalidFormatError is `sigData` is not a valid encoding
| // decode into typed data | ||
| data, err := p.Decode(sigData) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not decode sig data %s: %w", err, model.ErrInvalidFormat) | ||
| return nil, model.NewInvalidFormatErrorf("could not decode sig data %s", err) |
There was a problem hiding this comment.
would suggest to move the InvalidFormatError into the Decode function for API consistency
minor extension of error handling
| @@ -124,13 +106,12 @@ type Packer interface { | |||
| // sig is the aggregated signature data. | |||
| // Expected error returns during normal operations: | |||
| // * none; all errors are symptoms of inconsistent input data or corrupted internal state. | |||
| Pack(blockID flow.Identifier, sig *BlockSignatureData) ([]flow.Identifier, []byte, error) | |||
| Pack(blockID flow.Identifier, sig *BlockSignatureData) (signerIndices []byte, sigData []byte, err error) | |||
There was a problem hiding this comment.
what is the format of the returned sigData []byte ? are the 3 signatures concatenated together? It would be clearer to use a struct maybe?
There was a problem hiding this comment.
This sigData is to be used by the block header, which is also []byte.
The Pack method is supposed to convert the structured data, which is the input argument sig, into serialized bytes.
I was thinking about turning it into a type alias, but this sigData is going to be shared by both consensus and collection, the sigData type structure is actually different: collection has 1 less signature. So I ended up using []byte.
model/flow/quorum_certificate.go
Outdated
| // - AggregatedStakingSig crypto.Signature, | ||
| // - AggregatedRandomBeaconSig crypto.Signature |
There was a problem hiding this comment.
To be consistent with the rest of the code, we should use []byte for aggregated signatures (or update the code to use crypto.Signature)
| @@ -0,0 +1,319 @@ | |||
| package signature | |||
There was a problem hiding this comment.
I think the tools fo signer indices dealing with staking and beacon signatures are very specific to Hotstuff and could live under consensus/hotstuff/ instead of module/signature.
There was a problem hiding this comment.
You are right. That's what I was doing initially.
Later we realized the signer indices is not used just by consensus algorithm, but also to encode the approver IDs in the seal, which is not hotstuff related.
If we keep non-beacon related functions in this package to be reused, and move beacon-related functions to consensus/hotstuff, then we would have 2 different places about Encoding/Decoding signer indices, feels a bit having similar logic all over the places.
So in the end, decided to just keep all signer indices related encoding/decoding functions here. It's a trade-off.
module/signature/signer_indices.go
Outdated
| if l == 0 { | ||
| return nil | ||
| } | ||
| // the above check has ensured that lastByte does exist | ||
| lastByte := bitVector[l-1] | ||
| numPaddedBits := 8*l - numUsedBits | ||
| if (lastByte << (8 - numPaddedBits)) != 0 { |
There was a problem hiding this comment.
This seems to be equivalent and faster ( I haven't run the tests):
| if l == 0 { | |
| return nil | |
| } | |
| // the above check has ensured that lastByte does exist | |
| lastByte := bitVector[l-1] | |
| numPaddedBits := 8*l - numUsedBits | |
| if (lastByte << (8 - numPaddedBits)) != 0 { | |
| if numUsedBits & 7 == 0 { // if numUsedBits is multiple of 8, then there are no padding bits to check | |
| return nil | |
| } | |
| // the above check has ensured that lastByte does exist (l==0 is excluded) | |
| lastByte := bitVector[l-1] | |
| if (lastByte << (numUsedBits & 7)) != 0 { // shift by numUsedBits % 8 |
There was a problem hiding this comment.
Good idea. I verified with some tests, it is correct
module/signature/signer_indices.go
Outdated
| // * The number of padded bits are: `numPaddedBits := 8*len(bitVector) - len(canonicalIdentifiers)` | ||
| // * We know that numPaddedBits < 8; as `bitVector` passed check 1. Therefore, all padded bits are | ||
| // located in `bitVector`s _last byte_. | ||
| // * Let `lastByte` be the last byte of `bitVector`. The leading bits, specifically `(8 - numPaddedBits)` |
There was a problem hiding this comment.
Tarak's optimization yields the same result as the previous code, but a logic is structured slightly differently. Hence, I would suggest to also update the documentation that describes the algorithm:
| // * The number of padded bits are: `numPaddedBits := 8*len(bitVector) - len(canonicalIdentifiers)` | |
| // * We know that numPaddedBits < 8; as `bitVector` passed check 1. Therefore, all padded bits are | |
| // located in `bitVector`s _last byte_. | |
| // * Let `lastByte` be the last byte of `bitVector`. The leading bits, specifically `(8 - numPaddedBits)` | |
| // * As `bitVector` passed check 1, all padded bits are located in `bitVector`s _last byte_. | |
| // * Let `lastByte` be the last byte of `bitVector`. The leading bits, specifically `numUsedBits & 7`, |
|
|
||
| signers, err := signature.DecodeSignerIndicesToIdentities(allParticipants, qc.SignerIndices) | ||
| if err != nil { | ||
| if errors.Is(err, signature.ErrIllegallyPaddedBitVector) || errors.Is(err, signature.ErrIncompatibleBitVectorLength) { |
There was a problem hiding this comment.
❗
sorry, forgot to add this in my PR: we also need check for ErrInvalidChecksum here:
| if errors.Is(err, signature.ErrIllegallyPaddedBitVector) || errors.Is(err, signature.ErrIncompatibleBitVectorLength) { | |
| if errors.Is(err, signature.ErrIllegallyPaddedBitVector) || errors.Is(err, signature.ErrIncompatibleBitVectorLength) || errors.Is(err, signature.ErrInvalidChecksum) { | |
engine/consensus/ingestion/core.go
Outdated
| } | ||
| if err != nil { | ||
| return fmt.Errorf("internal error retrieving collector clusters: %w", err) | ||
|
|
There was a problem hiding this comment.
In my mind, this is all one code block, because the error checks all pertain to the ClusterByChainID call above.
I personally find Go already quite verbose and prefer not to add unnecessary newlines 😅
engine/consensus/ingestion/core.go
Outdated
| cluster, _, ok := clusters.ByNodeID(guarantors[0]) | ||
| if !ok { | ||
| return engine.NewInvalidInputErrorf("guarantor (id=%s) does not exist in any cluster", guarantors[0]) | ||
|
|
There was a problem hiding this comment.
same here
engine/consensus/ingestion/core.go
Outdated
| } | ||
| clusterMembers := cluster.Members() | ||
|
|
||
| // TODO: validate checksum |
There was a problem hiding this comment.
Is this TODO still applicable? I think we already validate the checksum:
DecodeSignerIndicesToIdentifiers calls CompareAndExtract which validates the checkum (?)
| // TODO: validate checksum |
| @@ -69,3 +70,26 @@ func IsSporkRootSnapshot(snapshot Snapshot) (bool, error) { | |||
| } | |||
| return true, nil | |||
| } | |||
|
|
|||
| // FindGuarantors decodes the signer indices from the guarantee, and finds the guarantor identifiers from protocol state | |||
| func FindGuarantors(state State, guarantee *flow.CollectionGuarantee) ([]flow.Identifier, error) { | |||
There was a problem hiding this comment.
would be good to document expected error returns here:
| func FindGuarantors(state State, guarantee *flow.CollectionGuarantee) ([]flow.Identifier, error) { | |
| // Expected Error returns during normal operations: | |
| // * signature.ErrIncompatibleBitVectorLength indicates that `signerIndices` has the wrong length | |
| // * signature.ErrIllegallyPaddedBitVector is the vector is padded with bits other than 0 | |
| // * signature.ErrInvalidChecksum if the input is shorter than the expected checksum contained therein | |
| func FindGuarantors(state State, guarantee *flow.CollectionGuarantee) ([]flow.Identifier, error) { |
| @@ -996,8 +996,12 @@ func (e *Engine) matchOrRequestCollections( | |||
| Hex("collection_id", logging.ID(guarantee.ID())). | |||
| Msg("requesting collection") | |||
|
|
|||
| guarantors, err := protocol.FindGuarantors(e.state, guarantee) | |||
There was a problem hiding this comment.
@m4ksio does this function differentiate between byzantine inputs and unexpected internal errors already? If so, we would need proper error treatment here.
model/flow/assignment/sort.go
Outdated
| "github.com/onflow/flow-go/model/flow/order" | ||
| ) | ||
|
|
||
| // FromIdentifierLists creates assignmentlist with canonical ordering from a list of IdentifierList. |
There was a problem hiding this comment.
| // FromIdentifierLists creates assignmentlist with canonical ordering from a list of IdentifierList. | |
| // FromIdentifierLists creates a `flow.AssignmentList` with canonical ordering from | |
| // the given `identifierLists`. |
| @@ -28,3 +24,20 @@ func ByReferenceOrder(nodeIDs []flow.Identifier) func(*flow.Identity, *flow.Iden | |||
| return indices[identity1.NodeID] < indices[identity2.NodeID] | |||
| } | |||
| } | |||
|
|
|||
| func IdentityListCanonical(identities flow.IdentityList) bool { | |||
There was a problem hiding this comment.
a minimal goDoc would be great. 🙏 🙇
module/packer/signers.go
Outdated
| "github.com/onflow/flow-go/model/flow" | ||
| ) | ||
|
|
||
| func FilterByIndices(identities flow.IdentityList, indices []int) (flow.IdentityList, error) { |
There was a problem hiding this comment.
I don't think this function is used at all. Can we remove it?
zhangchiqing
force-pushed
the
leo/feature-signer-indices-to-master
branch
from
b659980 to
eb26b6c
Compare
|
bors merge |
This PR merges signer indices to master.
What does signer indices do?
Each block header contains a list of signer's identifier in
Header.ParentVoteIDsfield to indicate who have signed (voted) for this block.For instance, for a group of 8 nodes:
[A,B,C,D,E,F,G,H], if a block is signed by 6 of them,[A,C,D,E,G,H], thenHeader.ParentVoteIDscould be:Although it's straightforward, the problem is it could take lots of space. On mainnet, ParentVoteIDs field takes about half size of a block head.
How to optimize?
The idea is that instead of indicating who signed by "name" (their identifier), we indicate by "position" (their index).
Since the full group identities is known, and there is a canonical order for the full identities, we can simply indicate by saying the block is signed by the "first" node, and the "third" node, and ...
Therefore, in our example, we could use 8 bits
[1,0,1,1,1,0,1]to indicate a block is signed byA,C,D,E,G,H. Each bit indicates whether a node at that index in the canonical order signed the block. We call these bits - "signer indices". It replaced theHeader.ParentVoteIDsfield, which is32 bytes * 6big, withHeader.ParentVoterIndicesfield, which is only1 bytebig.