[Consensus] SignerIndices Optimization (2/4) #2101

zhangchiqing · 2022-03-03T18:57:24Z

Build on top of #2047

This PR implements the SignerIndices optimization.

Note:

The EpochCommit event from smart contract uses VoterID, therefore, I have to introduce a new data type for QC: QuorumCertificateWithSignerIDs. See reasoning in comment
The unit test cases are mostly fixed, but integration tests are not, because the CollectionGuarantee also contains SignerIDs. See discussion here.. Will be implemented in separate PR.
make lint passes

zhangchiqing · 2022-03-09T23:48:07Z

module/finalizer/collection/finalizer.go

@@ -150,8 +150,10 @@ func (f *Finalizer) MakeFinal(blockID flow.Identifier) error {
 				Guarantee: flow.CollectionGuarantee{
 					CollectionID:     payload.Collection.ID(),
 					ReferenceBlockID: payload.ReferenceBlockID,
-					SignerIDs:        step.ParentVoterIDs,
-					Signature:        step.ParentVoterSigData,
+					// TODO use signer indices for collection guarantee signer IDs


https://github.com/dapperlabs/flow-go/issues/6181

consensus/hotstuff/signature/packer.go

AlexHentschel · 2022-03-16T06:41:00Z

consensus/hotstuff/signature/packer.go

+// the sigType will be [byte([1,1,0,0,0,0,0,0])],
+// 			bit 1 indicates the signer at the same index in signerIndices signed random beacon sig
+// 			bit 0 indicates the signer at the same index in signerIndices signed staking sig
+func encodeSignerIndicesAndSigType(fullMembers []flow.Identifier, stakingSigners flow.IdentifierList, beaconSigners flow.IdentifierList) ([]byte, []byte, error) {


For me, it would be more intuitive to rename fullMembers to orderedCommittee

Addressed here

durkmurder · 2022-03-17T15:21:15Z

consensus/hotstuff/verification/combined_verifier_v2.go

@@ -112,6 +112,7 @@ func (c *CombinedVerifier) VerifyVote(signer *flow.Identity, sigData []byte, blo
 //  - model.ErrInvalidSignature if a signature is invalid
 //  - error if running into any unexpected exception (i.e. fatal error)
 func (c *CombinedVerifier) VerifyQC(signers flow.IdentityList, sigData []byte, block *model.Block) error {
+	// TODO: remove this check, because we've checked the total stake have passed threshold


I feel like it's a sanity check, I would leave it.

Even if we remove this check, the Unpack will fail anyway if signers is empty.

Also we are not supposed to return model.ErrInvalidFormat if sanity check fails, which is what confused me. When I saw this, I thought there was unhappy case from a user error, but actually not.

Since the later operation will fail even without this check, I would prefer to remove this check here to avoid confusion.

⚠️

I think there are two aspects to this discussion:

Should we have a check here?

If the check fails, what error type should we return.

My thoughts regarding 1:

This is an implementation of the Verifier interface.

The place where we check that the total stake have passed threshold is in the Validator:

flow-go/consensus/hotstuff/validator/validator.go

Lines 70 to 71 in 619a82e

// verify whether the signature bytes are valid for the QC in the context of the protocol state

err = v.verifier.VerifyQC(signers, qc.SigData, block)

So this call from the Validator into the Verifier.VerifyQC happens through an API boundary

Validator [higher level code] --- API boundary ---> Verifier [lower level code]

Now there are some important considerations:

The API specification Verifier.VerifyQC does not mention the prerequisite that that higher level code must check first that the total stake has passed the threshold.

The assumption that "total stake have passed threshold" is made by the Verifier. In other words, we are making assumptions in the lower-level code about ordering of operations in the higher level code. I like to avoid any such assumptions if possible with reasonable effort.

Lastly, I am not convinced about this statement

the Unpack will fail anyway if signers is empty.

If both signers and sigData are empty or nil, are we sure that Unpack will fail? I think it might actually succeed. But the crypto operations (specifically AggregateBLSPublicKeys) will return an unexpected error, thereby crashing the receiving node.

In which order the higher level code executes its operations should ideally be up the higher level code and the lower level code should be agnostic to it. Otherwise, we make the code brittle; an engineer might re-order operations in the higher level code without being aware about implicit assumptions in the lower level code. After such a re-ordering of logic, the code might have uncovered edge cases, which could be exploited by malicious replicas.

Conclusion for 1:

Ideally, the code in CombinedVerifier.VerifyQC should handle any input without making assumptions about higher level code. This also included the case of empty inputs like signers and sigData as the ultimately were generated by an untrusted external actor. I think this is relatively simple to implement (see below).

My thoughts regarding 2:

In general, an empty signers slice indicates an invalid QC.

Nevertheless, @zhangchiqing has a good point:

If the higher-level logic already checked that the "total stake have passed threshold", signers being empty is an internal bug. However, there is no strong reason that the Validator must perform in the checks in this order.

If the Validator checked the signatures first (calling Verifier.VerifyQC first), len(signers) == 0 would indicate a byzantine input.

Conclusion for 2:

I think that the semantics of this failure case depends on the order of checks within Validator. Therefore, we should allow Validator to distinguish the failure case of len(signers) == 0 from other failure cases. Thereby it can make its own decision whether this error is expected (caused by a byzantine input) or a symptom of an internal bug).

Argument also applies to CombinedVerifierV3

In CombinedVerifierV3, we do not check whether signers is empty. In a world where we first check the signatures, this code would actually be vulnerable to a crash attack. This is because AggregateBLSPublicKeys fails with an internal error if there are no signatures to aggregate:

flow-go/consensus/hotstuff/verification/combined_verifier_v3.go

Line 145 in 619a82e

aggregatedKey, err := crypto.AggregateBLSPublicKeys(pubKeys) // caution: requires non-empty slice of keys!

P.S. will be addressing this in my PR with suggested revisions.

Addressed https://github.com/onflow/flow-go/pull/2204/files#diff-0cd49adfc8c2df8b741ff1252f2dc39210c56f5f9a506d21154e259b01b66cdeR123

durkmurder · 2022-03-17T15:43:29Z

engine/collection/compliance/core.go

@@ -83,7 +83,7 @@ func (c *Core) OnBlockProposal(originID flow.Identifier, proposal *messages.Clus
 		Hex("payload_hash", header.PayloadHash[:]).
 		Time("timestamp", header.Timestamp).
 		Hex("proposer", header.ProposerID[:]).
-		Int("num_signers", len(header.ParentVoterIDs)).
+		// Int("num_signers", len(header.ParentVoterIDs)).


is it temporary?

I commented this out because I haven't thought about what to replace with, but also don't want to forget about it.

Now the header only has ParentVoterIndices, I don't want to take extra query to parse the actual signers. Maybe we could simply log the entire ParentVoterIndices field?

Addressed here

durkmurder · 2022-03-17T16:05:34Z

module/finalizer/collection/finalizer_test.go

@@ -342,7 +342,7 @@ func TestFinalizer(t *testing.T) {
 			prov.AssertCalled(t, "SubmitLocal", &messages.SubmitCollectionGuarantee{
 				Guarantee: flow.CollectionGuarantee{
 					CollectionID: block1.Payload.Collection.ID(),
-					SignerIDs:    block1.Header.ParentVoterIDs,
+					SignerIDs:    []flow.Identifier{}, // block1.Header.ParentVoterIndices,


Will it be fixed later?

durkmurder · 2022-03-17T16:14:44Z

consensus/hotstuff/integration/instance_test.go

+
+			signerIndices, err := packer.EncodeSignerIdentifiersToIndices(in.participants.NodeIDs(), voterIDs)
+			if err != nil {
+				panic(fmt.Errorf("could not encode signer indices: %w", err))


I think this panic just makes things more complicated, can we stick to require.NoError?

Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>

AlexHentschel · 2022-03-16T22:10:20Z

consensus/hotstuff/signature/packer.go

+	stakingSignersLookup := stakingSigners.Lookup()
+	beaconSignersLookup := beaconSigners.Lookup()


Lets include some duplication checks here as sanity checks for the inputs:

Suggested change

stakingSignersLookup := stakingSigners.Lookup()

beaconSignersLookup := beaconSigners.Lookup()

stakingSignersLookup := stakingSigners.Lookup()

if len(stakingSignersLookup) != len(stakingSigners) {

return nil, nil, fmt.Errorf("duplicated entries in staking signers %v", stakingSignersLookup)

}

beaconSignersLookup := beaconSigners.Lookup()

if len(beaconSignersLookup) != len(beaconSigners) {

return nil, nil, fmt.Errorf("duplicated entries in beacon signers %v", stakingSignersLookup)

}

AlexHentschel · 2022-03-16T22:21:19Z

module/packer/signer_indices.go

@@ -0,0 +1,65 @@
+package packer


I feel that signer_indices.go largely replicates logic that we already have here: ledger/common/bitutils/utils.go

I think you could just call the functions from bitutils directly in the application code. These functions were written to be widely applicable. Re-packaging the same algorithms into purpose-specific methods (like EncodeSignerIndices) duplicates code and adds an unnecessary level of abstraction to the code.

I would suggest to remove the code in this file in its entirety. In module/packer/signer_ids.go, just use the methods from the bitutil package.

This file could at least re-use the tools from ledger/utils like MakeBitVector and SetBit.

Addressed https://github.com/onflow/flow-go/pull/2204/files#diff-a1983c6bd364d16ae09eb3dcc8abd9970e72cc66df5e8901d1434f188567d5b3R189

AlexHentschel · 2022-03-16T22:49:55Z

module/packer/signer_indices.go

+}
+
+// DecodeSignerIndices decodes the given compacted signer indices to a slice of indices.
+func DecodeSignerIndices(indices []byte, count int) ([]int, error) {


I believe anywhere we use this function, the indices are subsequently translated into node identities right away. For example here:

flow-go/consensus/hotstuff/validator/validator.go

Lines 54 to 62 in 3ab8b59

signerIndices, err := packer.DecodeSignerIndices(qc.SignerIndices, len(allParticipants))

if err != nil {

return newInvalidBlockError(block, fmt.Errorf("qc.SignerIndices is invalid: %w", err))

}

signers, err := filterByIndices(allParticipants, signerIndices)

if err != nil {

return newInvalidBlockError(block, fmt.Errorf("could not find signers by indices: %w", err))

}

But in this case, the validator could just directly DecodeSignerIdentifiersFromIndices

AlexHentschel · 2022-03-19T02:12:36Z

consensus/hotstuff/verification/combined_verifier_v2.go

@@ -112,6 +112,7 @@ func (c *CombinedVerifier) VerifyVote(signer *flow.Identity, sigData []byte, blo
 //  - model.ErrInvalidSignature if a signature is invalid
 //  - error if running into any unexpected exception (i.e. fatal error)
 func (c *CombinedVerifier) VerifyQC(signers flow.IdentityList, sigData []byte, block *model.Block) error {
+	// TODO: remove this check, because we've checked the total stake have passed threshold


⚠️

I think there are two aspects to this discussion:

Should we have a check here?

If the check fails, what error type should we return.

My thoughts regarding 1:

This is an implementation of the Verifier interface.

The place where we check that the total stake have passed threshold is in the Validator:

flow-go/consensus/hotstuff/validator/validator.go

Lines 70 to 71 in 619a82e

// verify whether the signature bytes are valid for the QC in the context of the protocol state

err = v.verifier.VerifyQC(signers, qc.SigData, block)

So this call from the Validator into the Verifier.VerifyQC happens through an API boundary

Validator [higher level code] --- API boundary ---> Verifier [lower level code]

Now there are some important considerations:

The API specification Verifier.VerifyQC does not mention the prerequisite that that higher level code must check first that the total stake has passed the threshold.

The assumption that "total stake have passed threshold" is made by the Verifier. In other words, we are making assumptions in the lower-level code about ordering of operations in the higher level code. I like to avoid any such assumptions if possible with reasonable effort.

Lastly, I am not convinced about this statement

the Unpack will fail anyway if signers is empty.

If both signers and sigData are empty or nil, are we sure that Unpack will fail? I think it might actually succeed. But the crypto operations (specifically AggregateBLSPublicKeys) will return an unexpected error, thereby crashing the receiving node.

In which order the higher level code executes its operations should ideally be up the higher level code and the lower level code should be agnostic to it. Otherwise, we make the code brittle; an engineer might re-order operations in the higher level code without being aware about implicit assumptions in the lower level code. After such a re-ordering of logic, the code might have uncovered edge cases, which could be exploited by malicious replicas.

Conclusion for 1:

Ideally, the code in CombinedVerifier.VerifyQC should handle any input without making assumptions about higher level code. This also included the case of empty inputs like signers and sigData as the ultimately were generated by an untrusted external actor. I think this is relatively simple to implement (see below).

My thoughts regarding 2:

In general, an empty signers slice indicates an invalid QC.

Nevertheless, @zhangchiqing has a good point:

If the higher-level logic already checked that the "total stake have passed threshold", signers being empty is an internal bug. However, there is no strong reason that the Validator must perform in the checks in this order.

If the Validator checked the signatures first (calling Verifier.VerifyQC first), len(signers) == 0 would indicate a byzantine input.

Conclusion for 2:

I think that the semantics of this failure case depends on the order of checks within Validator. Therefore, we should allow Validator to distinguish the failure case of len(signers) == 0 from other failure cases. Thereby it can make its own decision whether this error is expected (caused by a byzantine input) or a symptom of an internal bug).

Argument also applies to CombinedVerifierV3

In CombinedVerifierV3, we do not check whether signers is empty. In a world where we first check the signatures, this code would actually be vulnerable to a crash attack. This is because AggregateBLSPublicKeys fails with an internal error if there are no signatures to aggregate:

flow-go/consensus/hotstuff/verification/combined_verifier_v3.go

Line 145 in 619a82e

aggregatedKey, err := crypto.AggregateBLSPublicKeys(pubKeys) // caution: requires non-empty slice of keys!

tarakby · 2022-03-21T20:18:53Z

consensus/hotstuff/committees/cluster_committee.go

@@ -74,14 +78,12 @@ func (c *Cluster) Identities(blockID flow.Identifier, selector flow.IdentityFilt

 	// use the initial cluster members for root block
 	if isRootBlock {
-		return c.initialClusterMembers.Filter(selector), nil
+		return c.initialClusterMembers, nil


Does initialClusterMembers exclude the ejected nodes?
The list returned from the non-root block snapshot below excludes the ejected nodes.

Queries for the cluster's root block return the initial cluster members, without excluding any nodes ejected in the future. The reason is that the root block is defined at the same time as the initial cluster members are defined -- any ejections after that point shouldn't apply. Any ejections before that point should be reflected in the initial cluster members.

tarakby · 2022-03-21T23:39:01Z

model/flow/quorum_certificate.go

+// QuorumCertificateWithSignerIDs is a QuorumCertificate model with signer IDs instead of indices.
+// it is needed for backward-compatibility with the FlowEpoch smart contract which generates QCs
+// using signer IDs.
+type QuorumCertificateWithSignerIDs struct {


[optional] BootstrappingQuorumCertifcate and QuorumCertificate could be easier names maybe

tarakby · 2022-03-22T00:15:38Z

module/packer/signer_indices.go

@@ -0,0 +1,65 @@
+package packer


This file could at least re-use the tools from ledger/utils like MakeBitVector and SetBit.

tarakby · 2022-03-22T00:17:30Z

module/packer/signer_indices.go

+
+// EncodeSignerIndices encodes indices into compacted bit vector. Each bit represents whether the identity at
+// that index is a signer.
+// Note, the indices in the first argument must be in the strict increasing order,


if we re-use the tools from ledger/utils, I think this condition won't be required anymore.

tarakby · 2022-03-22T00:28:01Z

consensus/hotstuff/signature/packer.go

+//           0,1,1,0,1,1,1,0,1,1
+//    - Lastly, right-pad the resulting bit vector with 0 to full bytes. We have 10 committee members,
+//      so we pad to 2 bytes:
+//           01101110 11000000


Fyi, this is how bytes look like only for big endian cpus.

Suggested change

// 01101110 11000000

// 01101110 11000000 (if big endianness)

tarakby · 2022-03-22T00:28:36Z

consensus/hotstuff/signature/packer.go

+//             ↓ ↓ ↓ ↓ ↓ ↓ ↓
+//             0,1,0,1,1,1,1
+//    - Again, we right-pad with zeros to full bytes, As we only had 7 signers, the sigType slice is 1byte long
+//             01011110


Suggested change

// 01011110

// 01011110 (if big endianness)

jordanschalm · 2022-03-25T21:24:42Z

consensus/hotstuff/signature/packer.go

+//    constitutes an illegal input.
+//  * `stakingSigners` must be a subset of `fullMembers`
+//  * `beaconSigners` must be a subset of `fullMembers`
+// Violation of any of these prerequisites results in an error return.


Very clear documentation 💯

* 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>

* 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>

* [Consensus] SignerIndices Optimization Interface changes (1/4) (#2047) * 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> * fix test cases * update flow dep * fix rpc convert * update go.mod * fix go.mod * fix testnet network * Remove unused travis yml * update mocks * upgrade flow * update mock * update go.sum * fix hotstuff integration tests * fix integration test * first commit * upgrade onflow/flow package to v0.3.0 (#2377) * add checksum * add checksum to signer indices * fix execution tests * fix execution tests * adding comments * fix ingestion tests * fix access tests * fix access tests * fix access tests * fix integration test * add validation * fix consensus integration tests * fix flakey tests * adding checks * fix flakey tests * fix check * fix order * fix sort * use crc32 to compute checksum * use canonical order * fix qc_test * Apply suggestions from code review Co-authored-by: Tarak Ben Youssef <50252200+tarakby@users.noreply.github.com> * update comments * added BackendSCciptsMetrics interface and update noop/collector * call the metrics * switched to counter * use BigEndian.PutUint32 * added buckets for script size * go fmt * added GetTransactionResultRTT metric * record GetTransactionResultRTT metric * record GetTransactionResultRTT metric * check unit tests * fix unit tests * improve signer indices logging * typo * moved start time into loop * go fmt * remove util * avoid race condition in validating blocks * add snapshot command * renamed methods and switched to histograms * add comment on usage * memory limit exception for service account * fixed typos (#2443) minor extension of error handling * fixed imports * address review comments * lint * fix comment * update tests for validPadding * refactor validPadding * chore(docker): fix parallel image builds * separated script size into two metrics * payload_size label * adding comments * chore(tests): fix load test Currently, test fails with the following error: ``` 5:21PM ERR account creation tx failed error="[Error Code: 1101] cadence runtime error Execution failed:\nerror: invalid public key: [248, 71, 184, 64, 62, 118, 6, 35, 180, 178, 53, 126, 38, 116, 202, 182, 91, 243, 157, 205, 89, 77, 199, 235, 192, 117, 38, 122, 5, 166, 65, 116, 192, 67, 8, 190, 19, 100, 174, 246, 253, 218, 181, 121, 11, 79, 74, 202, 139, 156, 247, 215, 109, 152, 90, 162, 48, 116, 13, 180, 101, 48, 1, 40, 106, 13, 47, 226, 2, 3, 130, 3, 232]\n --> 9d665e105234216749b51b3a301153c069b5c1047a1cf3f55e2ea38a5aa2c033:13:23\n |\n13 | let publicKey2 = PublicKey(\n | ^\n" ``` It looks like it tries to pass the encoded public key (along w/ algo, hash, and weight) to a function that expects a raw public key. Fix it by using the underlying PublicKey. * unquarantining 2 flaky tests that have been consistently passing in quarantine (#2455) * comments * fix function renaming * linter (#2457) * Add memory weight for atree encoded slabs * Add missing weights for cadence memory kinds * Update to latest cadence * Fix test * [BFT Testing] Corruptible Conduit Factory authenticating dictated messages. (#2441) * makes generating execution receipt exportable * (no branch): Auto stash before checking out "HEAD" * removes corrupted execution result * makes generating result approval exportable * adds result approval generation to ccf * fixes the tests * adds attestation fixture * adds message content as a parameter to message fixture * fixes bug with publish * wip * adds test for result approval * adds corrupted execution receipt test * adds todos to factory * refactors output types of wintermute orchestrator * fixes lint issue * removes unused variable * Update insecure/wintermute/helpers.go Co-authored-by: Misha <misha.rybalov@dapperlabs.com> * Update insecure/wintermute/helpers.go Co-authored-by: Misha <misha.rybalov@dapperlabs.com> * fixes lint * fixes receipt and approval bug with factory * adds pass through test for receipt * adds approval test * fixes lint * sorting imports * sorting imports Co-authored-by: Misha <misha.rybalov@dapperlabs.com> * go mod tidy * update Cadence * update atree Co-authored-by: Jordan Schalm <jordan@dapperlabs.com> Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co> Co-authored-by: Kay-Zee <kan@axiomzen.co> Co-authored-by: Daniel Holmes <danielholmes@dapper.local> Co-authored-by: Tarak Ben Youssef <50252200+tarakby@users.noreply.github.com> Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com> Co-authored-by: danielholmes839 <danielh839@gmail.com> Co-authored-by: Bastian Müller <bastian@axiomzen.co> Co-authored-by: Daniel Holmes <43529937+danielholmes839@users.noreply.github.com> Co-authored-by: Alexey Ivanov <SaveTheRbtz@GMail.com> Co-authored-by: Misha <misha.rybalov@dapperlabs.com> Co-authored-by: Supun Setunga <supun.setunga@gmail.com> Co-authored-by: Yahya Hassanzadeh <yahya@dapperlabs.com> Co-authored-by: Robert E. Davidson III <45945043+robert-e-davidson3@users.noreply.github.com>

zhangchiqing force-pushed the leo/signer-indices branch 4 times, most recently from d4149ff to 1935196 Compare March 9, 2022 22:50

zhangchiqing added 23 commits March 9, 2022 15:36

implement IdentitiesByIndices

acc75b3

convert signer index

e54c08e

implement encoding and decoding

99c1858

add test cases

54be691

remove signer indices file

11947f1

fix Packer

a5b6ae8

add comments

d612738

remove IdentitiesByIndices

709fc96

update mock

e084823

fix test cases

52e2bc9

add comments

64f8324

remove fields

7b6a1cf

fix ParentVoterIDs

32d162c

fix verifier

6c0586d

fix mock

c7be2ad

implement staking vote progressor

0dc3c71

fix tests

84cb177

move to module/packer

715743e

fix tests

3675126

fix validator test

929bcba

fix validator test

a1aca6a

use ParentVoterIndices

f47f27c

add QuorumCertificateWithSignerIDs

2086754

zhangchiqing force-pushed the leo/signer-indices branch from 1935196 to 2086754 Compare March 9, 2022 23:37

update blockSummary

dd85d5e

zhangchiqing commented Mar 9, 2022

View reviewed changes

zhangchiqing changed the base branch from master to feature/signer-indices March 15, 2022 17:00

zhangchiqing changed the base branch from feature/signer-indices to leo/qc-use-signer-indices March 15, 2022 17:02

AlexHentschel reviewed Mar 16, 2022

View reviewed changes

zhangchiqing requested a review from durkmurder March 16, 2022 16:54

durkmurder reviewed Mar 17, 2022

View reviewed changes

zhangchiqing and others added 2 commits March 17, 2022 15:53

Apply suggestions from code review

b886dea

Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>

Apply suggestions from code review

619a82e

Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>

AlexHentschel reviewed Mar 19, 2022

View reviewed changes

tarakby reviewed Mar 22, 2022

View reviewed changes

zhangchiqing mentioned this pull request Mar 25, 2022

[Consensus] Refactor for guarantee signer indices (4/4) #2204

Merged

zhangchiqing changed the title ~~[Consensus] SignerIndices Optimization~~ [Consensus] SignerIndices Optimization (2/4) Mar 25, 2022

zhangchiqing requested review from durkmurder, AlexHentschel, jordanschalm and tarakby March 25, 2022 16:32

jordanschalm reviewed Mar 28, 2022

View reviewed changes

jordanschalm approved these changes Mar 28, 2022

View reviewed changes

zhangchiqing requested review from ramtinms, janezpodhostnik and m4ksio as code owners March 30, 2022 20:14

zhangchiqing merged commit dc42fe7 into leo/qc-use-signer-indices Mar 30, 2022

zhangchiqing deleted the leo/signer-indices branch March 30, 2022 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Consensus] SignerIndices Optimization (2/4) #2101

[Consensus] SignerIndices Optimization (2/4) #2101

zhangchiqing commented Mar 3, 2022 •

edited

Loading

zhangchiqing Mar 9, 2022

AlexHentschel Mar 16, 2022

zhangchiqing Mar 25, 2022 •

edited

Loading

durkmurder Mar 17, 2022

zhangchiqing Mar 17, 2022

durkmurder Mar 18, 2022

AlexHentschel Mar 19, 2022 •

edited

Loading

AlexHentschel Mar 19, 2022

zhangchiqing Mar 25, 2022

durkmurder Mar 17, 2022

zhangchiqing Mar 17, 2022

zhangchiqing Mar 25, 2022

durkmurder Mar 17, 2022

zhangchiqing Mar 17, 2022

durkmurder Mar 17, 2022 •

edited

Loading

zhangchiqing Mar 17, 2022

AlexHentschel Mar 16, 2022

AlexHentschel Mar 16, 2022

tarakby Mar 22, 2022

zhangchiqing Mar 25, 2022

AlexHentschel Mar 16, 2022

AlexHentschel Mar 19, 2022 •

edited

Loading

tarakby Mar 21, 2022

jordanschalm Mar 22, 2022

tarakby Mar 21, 2022

tarakby Mar 22, 2022

tarakby Mar 22, 2022

tarakby Mar 22, 2022

tarakby Mar 22, 2022

jordanschalm Mar 25, 2022

	// verify whether the signature bytes are valid for the QC in the context of the protocol state
	err = v.verifier.VerifyQC(signers, qc.SigData, block)

		stakingSignersLookup := stakingSigners.Lookup()
		beaconSignersLookup := beaconSigners.Lookup()

	signerIndices, err := packer.DecodeSignerIndices(qc.SignerIndices, len(allParticipants))
	if err != nil {
	return newInvalidBlockError(block, fmt.Errorf("qc.SignerIndices is invalid: %w", err))
	}

	signers, err := filterByIndices(allParticipants, signerIndices)
	if err != nil {
	return newInvalidBlockError(block, fmt.Errorf("could not find signers by indices: %w", err))
	}

	// 01101110 11000000
	// 01101110 11000000 (if big endianness)

[Consensus] SignerIndices Optimization (2/4) #2101

[Consensus] SignerIndices Optimization (2/4) #2101

Conversation

zhangchiqing commented Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

⚠️

My thoughts regarding 1:

Conclusion for 1:

My thoughts regarding 2:

Conclusion for 2:

Argument also applies to CombinedVerifierV3

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durkmurder Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

⚠️

My thoughts regarding 1:

Conclusion for 1:

My thoughts regarding 2:

Conclusion for 2:

Argument also applies to CombinedVerifierV3

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing commented Mar 3, 2022 •

edited

Loading

zhangchiqing Mar 25, 2022 •

edited

Loading

AlexHentschel Mar 19, 2022 •

edited

Loading

Argument also applies to `CombinedVerifierV3`

durkmurder Mar 17, 2022 •

edited

Loading

AlexHentschel Mar 19, 2022 •

edited

Loading

Argument also applies to `CombinedVerifierV3`