Draft(WIP): Initial implementation of the propagation reactor #1619
Conversation
rach-id
requested review from
rootulp and
ninabarbakadze
and removed request for
a team
rach-id
requested review from
evan-forbes
and removed request for
rootulp and
ninabarbakadze
| // Channel the channel ID used by the propagation reactor. | ||
| Channel = byte(0x50) | ||
|
|
||
| // DataChannel Duplicate of consensus data channel | ||
| // added as a temporary fix for circular dependencies | ||
| // TODO: fix redefining these values here. | ||
| DataChannel = byte(0x21) | ||
| StateChannel = byte(0x20) |
There was a problem hiding this comment.
documenting a from a sync discussion
the reactors need a new channels, and should only use new channels to remain compatible. We need to communicate between the consensus reactor using the sync routine and an interface that adds handles the valid block message etc
| self p2p.ID | ||
| } | ||
|
|
||
| func NewReactor(consensusState *consensus.State, options ...ReactorOption) *Reactor { | ||
| func NewReactor(options ...ReactorOption) *Reactor { |
There was a problem hiding this comment.
we could pass self as not optional since its always required
| self p2p.ID | ||
| } | ||
|
|
||
| func NewReactor(consensusState *consensus.State, options ...ReactorOption) *Reactor { | ||
| func NewReactor(options ...ReactorOption) *Reactor { |
There was a problem hiding this comment.
are we initializing the state here?
| DataChannel = byte(0x21) | ||
| StateChannel = byte(0x20) |
There was a problem hiding this comment.
we need at least two channels 1) recovery parts, compact blocks / proposals, and haves must be in the same channel 2) its likely advantageous to have another channel for wants
| case Channel: | ||
| switch msg := msg.(type) { | ||
| case *types2.TxMetaData: | ||
| // TODO: implement | ||
| case *types2.CompactBlock: | ||
| // TODO: implement | ||
| case *types2.PartMetaData: | ||
| // TODO: implement | ||
| case *types2.HaveParts: | ||
| // TODO: implement | ||
| case *types2.WantParts: | ||
| // TODO: implement | ||
| case *types2.RecoveryPart: | ||
| case *proptypes.CompactBlock: | ||
| // TODO: implement | ||
| case *proptypes.HaveParts: | ||
| // TODO check if we need to bypass request limits | ||
| blockProp.handleHaves(e.Src.ID(), msg, false) | ||
| case *proptypes.WantParts: | ||
| blockProp.handleWants(e.Src.ID(), msg.Height, msg.Round, msg.Parts) | ||
| case *proptypes.RecoveryPart: | ||
| blockProp.handleRecoveryPart(e.Src.ID(), msg) | ||
| default: | ||
| blockProp.Logger.Error(fmt.Sprintf("Unknown message type %v", reflect.TypeOf(msg))) |
There was a problem hiding this comment.
I think we also need to have the ability to handle proposals
| // HandleProposal adds a proposal to the data routine. This should be called any | ||
| // time a proposal is recevied from a peer or when a proposal is created. If the | ||
| // proposal is new, it will be stored and broadcast to the relevant peers. | ||
| func (blockProp *Reactor) HandleProposal(proposal *types.Proposal, from p2p.ID, haves *bits.BitArray) { |
There was a problem hiding this comment.
I think we could actually include the compact block in the proposal in a non-breaking way if we just add a field that isn't hashed or signed over (so literally just don't touch the signBytes or hash functions, just add the field to the prototype)
this way, peers that are running new reactor will send us proposals with compact blocks, which is nice and we get both in the state at the same time. peers not running the new proposal will just ignore the field in those proposals
Description
Please add a description of the changes that this PR introduces and the files that
are the most critical to review.
If this PR is non-trivial/large/complex, please ensure that you have either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests. The team
can be reached via GitHub Discussions or the Cosmos Network Discord server in
the #cometbft channel. GitHub Discussions is preferred over Discord as it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions
If the work in this PR is not aligned with the team's current priorities, please
be advised that it may take some time before it is merged - especially if it has
not yet been discussed with the team.
See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1
-->
PR checklist
.changelog(we useunclog to manage our changelog)
docs/orspec/) and code comments