Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BFT Block Puller: isolate common subcomponents #4292

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

tock-ibm
Copy link
Contributor

@tock-ibm tock-ibm commented Jun 21, 2023

Change-Id: I274d8883add91fac08da5202199d1bac53e99adb

Type of change

  • New feature

Description

  • Isolate subcomponents from the CFT Deliverer that can be used in the BFT Deliverer
  • A skeleton of the BFT Deliverer (no censurship monitor, no tests)

Related issues

#4293

@tock-ibm tock-ibm requested a review from a team as a code owner June 21, 2023 16:25
@tock-ibm tock-ibm force-pushed the bft-bp-refactor-components branch 7 times, most recently from 71d40a4 to 90890d1 Compare June 27, 2023 14:16
@yacovm
Copy link
Contributor

yacovm commented Jun 29, 2023

why is this in internal though? This can be useful for other projects.

@tock-ibm
Copy link
Contributor Author

tock-ibm commented Jul 2, 2023

why is this in internal though? This can be useful for other projects.

Correct. The plan is to put the reusable components in common/deliverclient, as common/deliver already contains the server components. This will be done in one of the future commits. See #4350

@tock-ibm tock-ibm force-pushed the bft-bp-refactor-components branch 3 times, most recently from 04dba24 to bf00f87 Compare July 30, 2023 07:07
}

func backOffDuration(base float64, exponent uint, minDur, maxDur time.Duration) time.Duration {
if base < 1.0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

base is generic but you set it to a constant where you call it.
Why not just pass the constant all the way here and avoid this check?

and why use nano second resolution? Why not just use milliseconds resolution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base may be different in different places. For example in the old CFT code it is 1.2 and In the new BFT code it is 2.0.
This function is flexible enough to cope with different bases.
Nanos-seconds is the underlying resolution of time.Duration (int64 in ns). I see no reason the decrease the resolution.

// TODO stop the monitor
}

func (d *BFTDeliverer) refreshSources() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a question / proposition, not a request.

As you know, when this thing is deployed on WAN, the remote node you pull blocks from, pays for egress traffic.

Would it not make sense to introduce a preference to pull blocks from your own organization?

It would also make sense to do so censorship wise.

Why not for simplicity, start the endpoint when we initialize the structure to be the endpoint of your own org?

Copy link
Contributor Author

@tock-ibm tock-ibm Aug 3, 2023

Choose a reason for hiding this comment

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

This could be an interesting idea to develop in the long run. However, right now, we don't have Org info in this particular data structure, and the peer org may be completely different from all ordering orgs, with no obvious ways of determining who is better to pull from.
I prefer to keep a simple randomized + round-robin policy and evaluate later on whether an improvement is needed.

This idea could be revisited late when we deal with the "connection source" data structure: #4347

d.mutex.Lock()
defer d.mutex.Unlock()
d.blockReceiver.Stop()
// TODO stop the monitor
Copy link
Contributor

Choose a reason for hiding this comment

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

why stop the monitor from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The BFTDeliverer is work in progress. It will be delt with here: #4348

return 0, errors.Errorf("received bad status %v from orderer", t.Status)
case *orderer.DeliverResponse_Block:
blockNum := t.Block.Header.Number
if err := br.blockVerifier.VerifyBlock(gossipcommon.ChannelID(br.channelID), blockNum, t.Block); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we deal a corner case where we ingest a config block and then verify the successive block before we have time to ingest the config block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, we don't. This will be the subject of a future PR where the deliverer hold its own copy of the most current config. See issue: #4346

blockVerifier: d.BlockVerifier,
deliverClient: deliverClient,
cancelSendFunc: cancel,
recvC: make(chan *orderer.DeliverResponse),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a non buffered channel. so we ingest a block one by one.

In gossip there is a buffer, but in the orderer there isn't one, so this is not efficient to pull blocks one by one and commit them one by one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plan is to have a buffer at the orderer as well, a buffer that gets all the blocks taken from this channel. The follower/chain goroutines then pull blocks from that buffer, not directly from the recvC channel shown here.

- Isolate subcomponents from the CFT Deliverer that can be used in the BFT Deliverer
- A skeleton of the BFT Deliverer (no censurship monitor, no tests)

Signed-off-by: Yoav Tock <tock@il.ibm.com>
Change-Id: I274d8883add91fac08da5202199d1bac53e99adb
Signed-off-by: Yoav Tock <tock@il.ibm.com>
Change-Id: I359e8402954b6db3a7a2df7a29d60fb04cfda63c
@yacovm yacovm merged commit 816d347 into hyperledger:main Aug 7, 2023
13 checks passed
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.

2 participants