Skip to content

Conversation

dviejokfs
Copy link

Type of change

  • New feature

Description

Add support for ConsensusTypeBFT in the orderer to be able to use this library to create the channel configuration using golang.

Additional details

None

Related issues

No related issues

@dviejokfs dviejokfs requested a review from a team as a code owner October 11, 2023 15:32
Signed-off-by: David VIEJO <[email protected]>
@denyeart
Copy link
Contributor

Thank you @dviejokfs !

@tock-ibm Any chance you could take a look? Core Fabric maintainers also maintain this fabric-config repository so would be good to get a maintainer familiar with SmartBFT config to take a look.

"github.com/golang/protobuf/proto"
"github.com/hyperledger/fabric-config/configtx/internal/policydsl"
"github.com/hyperledger/fabric-config/configtx/orderer"
"github.com/hyperledger/fabric-protos-go/common"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already imported with alias cb

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

ConsensusTypeEtcdRaft = "etcdraft"

// ConsensusTypeBFT identifies the SmartBFT-based consensus implementation.
ConsensusTypeBFT = "BFT"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no unit tests which use this constant

Comment on lines 880 to 914
if copyMd, ok := proto.Clone(op).(*sb.Options); ok {
return proto.Marshal(copyMd)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the op object cloned before it is marshalled?
marshalling creates a deep copy, this is unnecessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved

Comment on lines 46 to 47
SmartBFT *sb.Options
ConsenterMapping []common.Consenter

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There aren't any unit tests that cover these new objects, please add them. See the coverage of EtcdRaft as an example. In other words, everywhere you find orderer.ConsensusTypeEtcdRaft in tests, add a BFT test case.

EtcdRaft orderer.EtcdRaft
Organizations []Organization

SmartBFT *sb.Options

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SmartBFT -> SmartBFTOptions

Copy link
Author

@dviejokfs dviejokfs Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure? in Fabric 3.0 it's SmartBFT

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I think we need to be consistent

Kafka orderer.Kafka
EtcdRaft orderer.EtcdRaft
Organizations []Organization

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add line comments on these, and update the comment on the type, above, only EtcdRaft and BFT are supported.

}
}

func encodeBFTBlockVerificationPolicy(consenterProtos []common.Consenter, ordererGroup *cb.ConfigGroup) error {
Copy link

@tock-ibm tock-ibm Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is copied from Fabric right?
I think it would have been preferable to import it from Fabric, however, unfortunately, Fabric itself uses fabric-config in the integration tests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, then how do you prefer to resolve it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any news @tock-ibm ?

if consensusMetadata, err = marshalEtcdRaftMetadata(o.EtcdRaft); err != nil {
return fmt.Errorf("marshaling etcdraft metadata for orderer type '%s': %v", orderer.ConsensusTypeEtcdRaft, err)
}
case orderer.ConsensusTypeBFT:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many other places where case orderer.ConsensusTypeEtcdRaft: exists but case orderer.ConsensusTypeBFT: does not. For example: func (o *OrdererGroup) Configuration() (Orderer, error)

Please scan the project for any function that uses case orderer.ConsensusTypeEtcdRaft: and see whether BFT needs to be applied there as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

dviejokfs and others added 6 commits January 18, 2024 18:43
Signed-off-by: David VIEJO <[email protected]>
Signed-off-by: David VIEJO <[email protected]>
Signed-off-by: David VIEJO <[email protected]>
- Bump Go version to 1.22.0 and set toolchain to go1.23.5 in go.mod.
- Update indirect dependencies in go.mod and go.sum, including:
  - Upgrade `golang.org/x/net` to v0.34.0
  - Upgrade `golang.org/x/sys` to v0.29.0
  - Upgrade `golang.org/x/text` to v0.21.0
  - Upgrade `google.golang.org/grpc` to v1.67.3
  - Upgrade `google.golang.org/protobuf` to v1.36.3
  - Add `github.com/hyperledger/fabric-protos-go-apiv2` v0.3.7
- Refactor proto imports across multiple files to use `github.com/hyperledger/fabric-protos-go-apiv2` instead of `github.com/hyperledger/fabric-protos-go`.

Signed-off-by: dviejokfs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants