V.E PoC #1

Raneet10 · 2023-09-15T04:40:06Z

Description

PoC for using Side-txs with Vote Extensions

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

x/staking/keeper/keeper.go

+	}
+
+	var eventRecord types.EventRecord
+	k.cdc.Unmarshal(bz, &eventRecord)


simapp/abci.go

lasarojc · 2023-09-21T10:06:14Z

proto/cosmos/staking/v1beta1/tx.proto

+
+  // SendEventRecord defines an operation for updating the x/staking module
+  // parameters.
+  rpc SendEventRecord(MsgEventRecord) returns (MsgSendEventRecordResponse);


How are you testing? Broadcasting the same event to all nodes? Otherwise behavior of extend vote may be non-deterministic.

As discussed yesterday, a validator would be leveraging the CLI to broadcast the tx like so :
build/simd tx staking event --from cosmos1czfy9xfjzgphjjufmhfcxx2j7450qkmfuavs65 --keyring-backend=test --keyring-dir=.testnets/chain-YcCES5/node0/simcli --chain-id chain-YcCES5

Is this the only case @Raneet10 (leveraging the CLI I mean)?

lasarojc · 2023-09-21T10:10:12Z

simapp/abci.go


-		_, err := rand.Read(buf)
+		// Fetch from buffer
+		eventRecord, err := h.app.StakingKeeper.GetEventRecordFromBuffer(ctx)


If the node didn't execute processProposal, will vote SKIP.

https://docs.cometbft.com/main/spec/abci/abci++_comet_expected_behavior.html

Could change the behavior to fill the buffer here.

Note also that the me method is called for every round. Is there any situation in which nodes would want to update their votes?

And round numbers are not available here.

If the node didn't execute processProposal, will vote SKIP.

https://docs.cometbft.com/main/spec/abci/abci++_comet_expected_behavior.html

Could change the behavior to fill the buffer here.

That's a good point, noted.

Note also that the me method is called for every round. Is there any situation in which nodes would want to update their votes?

And round numbers are not available here.

Not sure which method you referred to here, but guessing that it's ExtendVote , we'd generally expect honest nodes to deterministically vote irrespective of the rounds but it might depend from module to module (would need to think of vulnerabilities and edge-cases per module here). We can of course have additional checks to ensure we don't get spammed with similar side-txs.

simapp/abci.go

lasarojc · 2023-09-21T10:19:02Z

simapp/abci.go

+		if hasTwoThirdsSigs {
+			fmt.Println("PrepareProposal: INJECTING VEs!")
+
+			for _, v := range req.LocalLastCommit.Votes {


A malicious node could censor side transactions here and nodes executing processProposal would not have a means to detect it.

Noted, we'll modify the approach in the updated PoC.

Hey @lasarojc , if the malicious node were to send empty or wrong VEs, would there be any way that the other nodes can detect it ? I guess they can query their local state and catch but is it possible to do it on the app level or will the Comet handle it ?

For CometBFT, the VE is just an a sequence of bytes. The sequence being empty is a valid case (the validator didn't have anything to add to the vote). Notice that an empty VE must still be signed.
The contents of the VE must make sense to the App and it is the app's job to verify it and reject it if is bogus.

lasarojc · 2023-09-21T10:21:10Z

simapp/abci.go

+// and reject the proposal in case we don't have a majority.
+// A downside is liveness can get negatively affected in case of invalid VEs.
+func (app *SimApp) NewProcessProposalHandler() sdk.ProcessProposalHandler {
+	return func(ctx sdk.Context, req *abci.RequestProcessProposal) (*abci.ResponseProcessProposal, error) {


Does not verify if the side transactions were properly encoded, double proposed or indeed have a majority.

Good point and as mentioned above, we will update in the new PoC.

The RequestProcessProposal.ProposedLastCommit does not seem to have ExtendedVoteInfo to check for majority. Fetching that from txs bytes may not be a good idea because of censorship/bad V.E data. What do you think, am I missing anything?

Have those checks been implemented?

the API does not deliver the extended commit info in process proposal. The recommended approach for now is to extract the set of VE from the special transaction in txs.
In that sense, the proposer will be able to sensor some VE, but must include at least the 2f+1 it used.

simapp/abci.go

marcello33 · 2023-10-13T08:41:07Z

proto/cosmos/staking/v1beta1/tx.proto

+
+  // SendEventRecord defines an operation for updating the x/staking module
+  // parameters.
+  rpc SendEventRecord(MsgEventRecord) returns (MsgSendEventRecordResponse);


Is this the only case @Raneet10 (leveraging the CLI I mean)?

marcello33 · 2023-10-13T08:42:35Z

simapp/abci.go

+6. FinalizeBlock -> Finalize and execute txs.
+7. CommitBlock -> Self explanatory.
+*/
+


@Raneet10 is this the confirmed flow?
@VAIBHAVJINDAL3012 can you add your concerns here wrt "sepcial tx"?

Yes, Encoding the VE and putting it in a block is a special tx. I meant the same.

marcello33 · 2023-10-13T08:45:19Z

simapp/abci.go

+// and reject the proposal in case we don't have a majority.
+// A downside is liveness can get negatively affected in case of invalid VEs.
+func (app *SimApp) NewProcessProposalHandler() sdk.ProcessProposalHandler {
+	return func(ctx sdk.Context, req *abci.RequestProcessProposal) (*abci.ResponseProcessProposal, error) {


Have those checks been implemented?

marcello33 · 2023-10-13T08:48:07Z

simapp/abci.go

+			var eventData *stakingtypes.EventRecord
+			for _, v := range req.Txs {
+				// Have a way to segregate b/w signed VEs and regular txs. Assuming for VEs, byte length would be 8
+				if len(v) >= 8 {


Is this always the case? Can other txs have the same length?

The point here is that we will need a distinct way of having separation between normal txs and V.Es. That's why the comment :
// Have a way to segregate b/w signed VEs and regular txs. Assuming for VEs, byte length would be 8

marcello33 · 2023-10-13T08:48:18Z

simapp/abci.go

+// PreFinalizeBlock -> Extract VEs from previous block and persist in DB.
+func (app *SimApp) NewPreFinalizeBlockHookHandler() sdk.PreFinalizeBlockHook {
+	return func(ctx sdk.Context, req *abci.RequestFinalizeBlock) error {
+		if req.Height > 5 {


The V.E. in the devnet I was testing were enabled from 5.

marcello33 · 2023-10-13T08:50:04Z

x/staking/keeper/keeper.go

+		}
+
+		if len(vote.ExtensionSignature) == 0 {
+			return fmt.Errorf("vote extensions enabled; received empty vote extension signature at height %d", currentHeight)


Does it need to be error? Can we have no vote extensions for some txs (not side-txs)?

Yes we can , but they have to be signed. cc: @lasarojc

github-actions · 2023-10-18T06:39:19Z

baseapp/abci.go

 		Time:               req.Time,
 		ProposerAddress:    req.ProposerAddress,
 		NextValidatorsHash: req.NextValidatorsHash,
+		AppHash:            app.LastCommitID().Hash,
 	}
 	app.setState(execModePrepareProposal, header)



Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).PrepareProposal (baseapp/abci.go:390)

lasarojc · 2023-11-08T17:57:44Z

Yes we can , but they have to be signed. cc: @lasarojc

Once enabled, VE must be included on every round. They may be empty, though.

lasarojc · 2023-11-09T13:53:02Z

x/staking/keeper/keeper.go

@@ -172,3 +179,161 @@ func (k Keeper) GetValidatorUpdates(ctx context.Context) ([]abci.ValidatorUpdate

 	return valUpdates.Updates, nil
 }
+
+// Copied from baseapp/abci_utils.go


This function has been updated by the SDK team to address issue 17518 and the changes need to be reflected here as well.

cc: @VAIBHAVJINDAL3012

lasarojc · 2023-11-09T18:39:32Z

simapp/abci.go

+// A downside is liveness can get negatively affected.
+func (app *SimApp) NewProcessProposalHandler() sdk.ProcessProposalHandler {
+	return func(ctx sdk.Context, req *abci.RequestProcessProposal) (*abci.ResponseProcessProposal, error) {
+		if req.Height > 5 {


As in the the staking/keeper.go helper, use

cp := ctx.ConsensusParams() extsEnabled := cp.Abci != nil && currentHeight >= cp.Abci.VoteExtensionsEnableHeight && cp.Abci.VoteExtensionsEnableHeight != 0

@VAIBHAVJINDAL3012

lasarojc · 2023-11-09T18:58:30Z

simapp/abci.go

+					continue
+				}
+
+				if ve.Result == Vote_YES {


A single YES is enough to persist the VE/execute the SideTx. Should it instead tally the votes to decide on executing or not?

Yes we should , similar to how we do in Heimdall : https://github.com/maticnetwork/heimdall/blob/5490d73aaa33bd83f49897eed70a2092b17ac185/app/side_tx_processor.go#L41
cc: @VAIBHAVJINDAL3012

lasarojc · 2023-11-10T12:55:59Z

simapp/abci.go

+			for _, msg := range msgs {
+				switch msg.(type) {
+				// TODO: Redirect ExtendVote calls to individual module
+				case *stakingtypes.MsgEventRecord:


Note for when implementing: handle or forbid having multiple MsgEventRecord/SideTx in the same block.

github-actions · 2023-12-11T00:43:37Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

simapp/abci.go

+				if ve.Result == Vote_YES {
+					if err := json.Unmarshal(ve.Data, &eventData); err == nil {
+						fmt.Println("PERSISTING VE DATA!", "HEIGHT: ", ctx.BlockHeight())
+						app.StakingKeeper.SetEventRecord(ctx, *eventData)


github-actions · 2024-01-18T00:43:07Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions bot added C:x/gov C:x/staking C:x/tx C:CLI C:orm labels Sep 15, 2023

This comment has been minimized.

Sign in to view

Raneet10 changed the title ~~Raneet10/comet poc~~ V.E PoC Sep 15, 2023

github-advanced-security bot found potential problems Sep 15, 2023

View reviewed changes

x/staking/keeper/keeper.go

}

var eventRecord types.EventRecord

k.cdc.Unmarshal(bz, &eventRecord)

Check warning

Code scanning / gosec

Errors unhandled. Warning

Errors unhandled.

simapp/abci.go Fixed Show fixed Hide fixed

lasarojc reviewed Sep 21, 2023

View reviewed changes

Raneet10 added 4 commits October 5, 2023 09:20

init: cometbft poc

6e6e8d2

add few comments and minor fix

d1a40fa

fix VEs and txs encoding, add/modify few keeper methods for event-record

58260da

init 2-heights VE

4ece8d6

marcello33 reviewed Oct 13, 2023

View reviewed changes

init h+1 side-txs

be36d4b

Raneet10 force-pushed the raneet10/comet-poc branch from c67ec89 to be36d4b Compare October 18, 2023 06:38

github-actions bot added C:x/auth C:x/authz C:x/bank C:x/crisis C:x/distribution C:x/evidence C:x/feegrant C:x/genutil C:x/group C:x/mint C:x/nft C:x/params C:Simulations C:x/slashing labels Oct 18, 2023

github-actions bot added C:x/consensus C:x/circuit C:collections C:Cosmovisor C:Rosetta C:Confix C:Hubl C:Keys Type: Build Type: CI Type: ADR C:Store labels Oct 18, 2023

github-actions bot reviewed Oct 18, 2023

View reviewed changes

lasarojc reviewed Nov 9, 2023

View reviewed changes

lasarojc changed the base branch from raneet10/temp-pr-branch to release/v0.50.x November 10, 2023 12:41

lasarojc reviewed Nov 10, 2023

View reviewed changes

github-actions bot added the Stale label Dec 11, 2023

github-actions bot closed this Dec 16, 2023

Raneet10 reopened this Dec 18, 2023

github-advanced-security bot found potential problems Dec 18, 2023

View reviewed changes

github-actions bot removed the Stale label Dec 19, 2023

github-actions bot added the Stale label Jan 18, 2024

github-actions bot closed this Jan 22, 2024

marcello33 reopened this Feb 7, 2024

github-actions bot removed the Stale label Feb 8, 2024

V.E PoC #1

Are you sure you want to change the base?

V.E PoC #1

Conversation

Raneet10 commented Sep 15, 2023

Description

Author Checklist

Reviewers Checklist

This comment has been minimized.

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

Raneet10 Sep 22, 2023 • 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

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

github-actions bot Oct 18, 2023

Choose a reason for hiding this comment

lasarojc commented Nov 8, 2023

lasarojc Nov 9, 2023 • 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

lasarojc Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 11, 2023

github-actions bot commented Jan 18, 2024

Raneet10 Sep 22, 2023 •

edited

Loading

lasarojc Nov 9, 2023 •

edited

Loading

lasarojc Nov 10, 2023 •

edited

Loading