Do not pass value to instance #371

GalRogozinski · 2024-03-24T23:20:14Z

Description

Instance will now fetch consensus data only if it is the round leader.
Timer starts before data is fetched to ensure it is synchronized across committee nodes.

…of time

This reverts commit 63996b3.

GalRogozinski · 2024-03-24T23:24:02Z

#370 was opened while working on this

olegshmuelov · 2024-03-31T14:34:46Z

qbft/instance.go

@@ -121,7 +132,7 @@ func (i *Instance) ProcessMsg(msg *SignedMessage) (decided bool, decidedValue []
 			}
 			return err
 		case RoundChangeMsgType:
-			return i.uponRoundChange(i.StartValue, msg, i.State.RoundChangeContainer, i.config.GetValueCheckF())
+			return i.uponRoundChange(msg, i.State.RoundChangeContainer, i.config.GetValueCheckF())


I noticed we're passing i.StartValue, i.State.RoundChangeContainer, and i.config.GetValueCheckF() as arguments to the function. Given these are accessed via the instance i, could someone help clarify the rationale behind explicitly passing these as parameters instead of accessing them directly within the uponRoundChange method?

Is there a specific design choice or benefit we're aiming for by structuring the call this way? For instance, does this approach enhance testability, readability, or separation of concerns in a way that accessing them directly within the function would not?

ditto
i.uponProposal
i.uponPrepare
i.UponCommit

@GalRogozinski I agree with this, either only pass and not save (if its only used within these function) or save and don't pass.

this is out of scope of PR, but can be done

olegshmuelov · 2024-03-31T14:39:27Z

qbft/instance.go

@@ -121,7 +132,7 @@ func (i *Instance) ProcessMsg(msg *SignedMessage) (decided bool, decidedValue []
 			}
 			return err
 		case RoundChangeMsgType:
-			return i.uponRoundChange(i.StartValue, msg, i.State.RoundChangeContainer, i.config.GetValueCheckF())
+			return i.uponRoundChange(msg, i.State.RoundChangeContainer, i.config.GetValueCheckF())


case CommitMsgType: decided, decidedValue, aggregatedCommit, err = i.UponCommit(msg, i.State.CommitContainer) if decided { i.State.Decided = decided i.State.DecidedValue = decidedValue } return err

not related to the change, but we should check for err before assign

olegshmuelov · 2024-03-31T14:43:18Z

qbft/instance.go

@@ -24,6 +24,7 @@ type Instance struct {
 	// forceStop will force stop the instance if set to true
 	forceStop  bool
 	StartValue []byte
+	CdFetcher  *types.DataFetcher `json:"-"`


Could we rename CdFetcher to CDFetcher for clarity? 'CD' stands for 'Consensus Data', and capitalizing the acronym could improve readability and align with our naming conventions.

I agree. I even prefer the whole ConsensusDataFetcher, or simply DataFetcher.

Lets to this, I prefer DataFetcher or Fetcher. anyway if using acronym always capital letters for all words.

olegshmuelov · 2024-03-31T15:01:49Z

ssv/attester.go

-	}
-
-	if err := r.BaseRunner.decide(r, input); err != nil {
+	if err := r.BaseRunner.decide(r, attestationFetcher); err != nil {


Why are we passing r to r.BaseRunner.decide? Can't it access r internally?

same for
aggregator
proposer
and more

It seems to me that the BaseRunner can't access the runner internally. But, also, the decide function doesn't need the runner as far as I understand. So, I think this could be simplified to r.BaseRunner.decide(fetcher) too

olegshmuelov · 2024-03-31T16:21:21Z

qbft/spectest/tests/startinstance/empty_value.go

@@ -5,6 +5,7 @@ import (
 )

 // EmptyValue tests a starting an instance for an empty value (not passing value check)
+// Instance starts but with an empty value and no proposal gets broadcasted


Is there a way no proposals gets broadcasted?

@olegshmuelov I assume if CDFetcher fails fetching..

olegshmuelov · 2024-03-31T16:21:46Z

qbft/spectest/tests/startinstance/invalid_value.go

@@ -6,6 +6,7 @@ import (
 )

 // InvalidValue tests a starting an instance for an invalid value (not passing value check)
+// Instance starts but with an empty value and no proposal gets broadcasted


olegshmuelov · 2024-03-31T16:22:00Z

qbft/spectest/tests/startinstance/nil_value.go

@@ -5,6 +5,7 @@ import (
 )

 // NilValue tests a starting an instance for a nil value (not passing value check)
+// Instance starts but with an empty value and no proposal gets broadcasted


MatheusFranco99

Good work!! Just left a suggestion

MatheusFranco99 · 2024-04-01T09:38:04Z

qbft/round_change.go

+func chooseValueToPropose(roundChangeMsg *SignedMessage, cdFetcher *types.DataFetcher) ([]byte, error) {
+	var valueToPropose []byte
+	if roundChangeMsg.Message.RoundChangePrepared() {
+		valueToPropose = roundChangeMsg.FullData
+	} else {
+		cd, err := cdFetcher.GetConsensusData()
+		if err != nil {
+			return nil, err
+		}
+		valueToPropose = cd
+	}
+	return valueToPropose, nil
 }


I believe this function can be called more than once during the Round-Change phase. If so, we would also wait for the Beacon Node more than once. Is there a way to save the data, e.g. storing it in the i.StartValue, so it can be returned faster?

The idea of cdFetcher is that implementation can do it in a way where it calls it asynchronously and even caches values. But this is an implementation detail.

@olegshmuelov

nkryuchkov · 2024-04-09T12:08:35Z

qbft/instance.go

+			}
+			if err = valueCheckF(value); err != nil {
+				fmt.Printf("%s\n", err.Error())
+				return


should we also return if CreateProposal returns an error?

nkryuchkov · 2024-04-09T12:12:39Z

types/consensus_data.go

@@ -46,6 +46,12 @@ func (c *Contributions) HashTreeRootWith(hh ssz.HashWalker) error {
 	return nil
 }

+// DataFetcher asynchronusly fetches data from the beacon node upon instantiation


Suggested change

// DataFetcher asynchronusly fetches data from the beacon node upon instantiation

// DataFetcher asynchronously fetches data from the beacon node upon instantiation

nkryuchkov · 2024-04-09T12:14:21Z

types/consensus_data.go

@@ -46,6 +46,12 @@ func (c *Contributions) HashTreeRootWith(hh ssz.HashWalker) error {
 	return nil
 }

+// DataFetcher asynchronusly fetches data from the beacon node upon instantiation
+type DataFetcher struct {


Why not interface? Using concrete types is not flexible and inconvenient for testing and when spec repo is used by other repositories

@nkryuchkov @GalRogozinski
Usually I support using concrete types. As they are more predictable than interfaces. but this time you are using this concrete type as an interface so it might make more sense for it to be an interface ( but then also moved to the runners ).
the rule is -
Return concrete types
Receive interfaces

MatheusFranco99 · 2024-07-10T08:15:23Z

qbft/instance.go

@@ -56,16 +57,26 @@ func (i *Instance) ForceStop() {
 }

 // Start is an interface implementation
-func (i *Instance) Start(value []byte, height Height) {
+func (i *Instance) Start(cdFetcher *types.DataFetcher, height Height, valueCheckF ProposedValueCheckF) {


Can't we use the i.GetConfig().GetValueCheck() instead of using an argument? It seems a duplication to me

I agree, if value check only used here then maybe we don't need it as part of Config.

MatheusFranco99 · 2024-07-10T08:16:37Z

qbft/instance.go

+				fmt.Printf("%s\n", err.Error())
+				return


The error should be returned and not printed, no?

apparently we printed errors in main before...
not sure why...
Maybe we should add errors and test them?
I will open an issue

MatheusFranco99 · 2024-07-10T08:21:33Z

qbft/round_change.go

+	if proposer(state, config, roundChangeMsg.QBFTMessage.Round) != state.CommitteeMember.OperatorID {
+		return nil, errors.New("not proposer")
+	}


The same check is done below. Any reason for duplicating it or we can remove it?

where? I don't see it

MatheusFranco99 · 2024-07-10T08:29:36Z

qbft/round_change.go

+func chooseValueToPropose(roundChangeMsg *Message, cdFetcher *types.DataFetcher, preparedValue []byte) ([]byte, error) {
+	var valueToPropose []byte
+	if roundChangeMsg.RoundChangePrepared() {
+		valueToPropose = preparedValue
+	} else {
+		cd, err := cdFetcher.GetConsensusData()
+		if err != nil {
+			return nil, err
+		}
+		valueToPropose = cd
+	}
+	return valueToPropose, nil


Since the CDFetcher may update its value, we need to make sure that two different proposals for the same round can't be done. One way to do that is to check, in the validation of Round-Change messages, that the msg.Round is strictly above our current round.

I don't understand.. we call this once per round

y0sher · 2024-07-10T11:32:42Z

qbft/controller.go

-	if err := c.GetConfig().GetValueCheckF()(value); err != nil {
-		return errors.Wrap(err, "value invalid")
-	}
+func (c *Controller) StartNewInstance(height Height, cdFetcher *types.DataFetcher) error {


Why not move the fetching logic outside of controller and pass an already fetched value? this of course also means determining that we are leaders outside of the controller. IMO it makes more sense than passing a fetcher. less changes to controller.

thinking again this might not work if we need to fetch data after round change? but probably we should fetch data at the beginning of the slot regardless.

y0sher · 2024-07-10T12:15:52Z

qbft/round_change.go

+// chooseValueToPropose
+// If justifiedRoundChangeMsg has no prepare justification choose state value
+// If justifiedRoundChangeMsg has prepare justification choose prepared value
+func chooseValueToPropose(roundChangeMsg *Message, cdFetcher *types.DataFetcher, preparedValue []byte) ([]byte, error) {


since its only used in one place, this shouldn't be a method IMO, or at least the name should reflect that a long-awaited fetch is part of this function.

y0sher

you think its possible to do the fetching outside of the qbft controller?

iurii-ssv · 2024-10-31T14:11:14Z

@GalRogozinski (if/once you get back to this code) could you clarify/confirm whether this PR (after finished and merged) will support and be 100% compatible with the solution for the issue I described here (which we'll probably want to address at some point) ?

I haven't looked through the code of this PR too much yet, but from cursory overview I think

it couples/bundles the start of QBFT instance and the call to fetch starting data (when leader)
while the more flexible/unassuming approach (for the case when we are QBFT leader especially, but also in general) would be to 1) start QBFT instance with non-blocking Instance.Start call and let it finish all the setup it needs for Instance to become functional in terms of being able to communicate with other instances in the cluster (and participate round-changes for example) 2) and make it such that Instance can just "get injected" with StartValue without knowing when/whether/whom by it will happen (ofc this way there will be a new set of conditions to handle - when StartValue is needed but is not there yet); StartValue name also kinda becomes just Value and Instance will need to process value injection as it sees fits depending on his internal state at the time of the injection (which would be just a simple thread-safe Instance.SetValue call) depending on whether he is still leader, how long he still has till timeout in the current round - for example he can even yield his round voluntarily if timeout happens soon (if we add it as an option, as I describe in proposer-runner: QBFT liveness choked by Beacon node ssv#1825)

lets consider the following scenario that shows when/why "value injection" might be needed:

we have block proposer duty, lets say we found a way to start all QBFT instances for the relevant cluster "roughly at the same time" (which btw current SSV node implementation doesn't do as my comment describes it proposer-runner: QBFT liveness choked by Beacon node ssv#1825 (comment))
to make it concrete, lets say cluster is 4 nodes for this example, all 4 have now started counting time to the first round timeout waiting for leader to fetch his data (which he is in the process of)
leader couldn't fetch his data, and even worse - he is synchronously blocked on a call to Beacon node
now 3 nodes start round 1 change, and stuck leader of round 1 can't further participate in QBFT for this cluster because he is just stuck (his whole duty is blocked/stuck btw waiting for Instance.Start func to return) - all because of QBFT instance took it upon itself to go fetch the data
and Instance can't really recover from this type of scenario (even if DataFetcher function has timeout/retry logic) to start/keep participating in QBFT because technically Instance has never started and is thus useless to QBFT cluster

GalRogozinski · 2024-11-05T09:02:36Z

Hey @iurii-ssv,

Yeah, this PR is suppose to allow making async calls.
We stopped working on it due to Alan fork... which is a shame in my opinion because I think this change should be part of a fork so all nodes have similar behavior

So currently we need to wait for the next fork

GalRogozinski added 22 commits March 20, 2024 03:33

data fetcher WIP

7dfde5b

add beacon block fetcher to instance.go

4c8f16c

fix proposal justification for RC

6f4586b

stop passing startValue where it isn't needed

17b801b

refactor proposer

bdc5e3c

all runners work with fetchers now

78db844

controller spec test bad fix

335b7b1

instance fix?

503748a

move out testing utils function

57908cb

fix input values

ebaab30

change back to encode cd

058a797

start value should be dataSSZ

a2a58e1

solve nil dereference in MsgProcessingTest

8c4fb23

we must start instances now since they don't know their values ahead …

4c62e52

…of time

valueToPropose should be start value

8f185e2

delete needless tests

63996b3

fix error message

609dd25

start value is value

6c75aaa

Revert "delete needless tests"

d7fab20

This reverts commit 63996b3.

fetcher returns bytes

e9a4fe9

fix tests

32e51e3

make generate-jsons

a115e1d

GalRogozinski requested review from y0sher, MatheusFranco99 and moshe-blox March 24, 2024 23:20

lint

520c891

create CdFetcher when running from Json

596ee2a

olegshmuelov reviewed Mar 31, 2024

View reviewed changes

MatheusFranco99 approved these changes Apr 1, 2024

View reviewed changes

olegshmuelov mentioned this pull request Apr 1, 2024

Do not pass value to instance (Align Spec) ssvlabs/ssv#1363

Draft

GalRogozinski added consensus consensus related issues fork labels Apr 4, 2024

nkryuchkov reviewed Apr 9, 2024

View reviewed changes

GalRogozinski changed the base branch from main to alan June 13, 2024 08:33

GalRogozinski added 6 commits July 8, 2024 03:50

Merge branch 'alan' into no-pass-value-instance

dd23f90

Merge branch 'alan' into no-pass-value-instance

1d1a833

fix Committee Runner

c1a4ba7

fix generate ssz

940e3de

delete precon_justifiactions

ce32670

generate jsons

ea2813e

GalRogozinski requested a review from MatheusFranco99 July 9, 2024 06:30

MatheusFranco99 reviewed Jul 10, 2024

View reviewed changes

GalRogozinski mentioned this pull request Jul 10, 2024

we printed errors instead of returning them #460

Open

y0sher reviewed Jul 10, 2024

View reviewed changes

y0sher requested changes Jul 10, 2024

View reviewed changes

GalRogozinski mentioned this pull request Oct 30, 2024

Dynamic QBFT Timeout ssvlabs/SIPs#37

Open

iurii-ssv mentioned this pull request Oct 31, 2024

proposer-runner: QBFT liveness choked by Beacon node ssvlabs/ssv#1825

Open

	// DataFetcher asynchronusly fetches data from the beacon node upon instantiation
	// DataFetcher asynchronously fetches data from the beacon node upon instantiation

Do not pass value to instance #371

Are you sure you want to change the base?

Do not pass value to instance #371

Conversation

GalRogozinski commented Mar 24, 2024 • edited Loading

Description

GalRogozinski commented Mar 24, 2024

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

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

MatheusFranco99 left a 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

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

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

y0sher left a comment

Choose a reason for hiding this comment

iurii-ssv commented Oct 31, 2024 • edited Loading

GalRogozinski commented Nov 5, 2024

GalRogozinski commented Mar 24, 2024 •

edited

Loading

iurii-ssv commented Oct 31, 2024 •

edited

Loading