abci: Process Proposal by ValarDragon · Pull Request #6480 · tendermint/tendermint

ValarDragon · 2021-05-19T15:58:49Z

This PR adds an implementation of synchronous ProcessProposal into the ABCI++ branch. The main part to review is the consensus/state.go and state/execution.go, and then the rest should be checking if I implemented the ABCI++ scaffolding correctly. (Please do check this, its not really clear to me!)

I'm currently working on tests for state.go, and adding app-returned evidence to the local evidence pool. Currently there is only one added test for block execution.

ref #6066

codecov · 2021-05-20T05:35:53Z

Codecov Report

❗ No coverage uploaded for pull request base (abci++@a9e231b). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head b49e37c differs from pull request most recent head 216b838. Consider uploading reports for the commit 216b838 to get more accurate results

@@            Coverage Diff            @@
##             abci++    #6480   +/-   ##
=========================================
  Coverage          ?   60.81%           
=========================================
  Files             ?      288           
  Lines             ?    27144           
  Branches          ?        0           
=========================================
  Hits              ?    16508           
  Misses            ?     8934           
  Partials          ?     1702

alexanderbez · 2021-05-20T12:09:00Z

abci/client/grpc_client.go

 }

+// NOTE: call is synchronous, use ctx to break early if needed
+func (cli *grpcClient) processProposalAsync(ctx context.Context, params types.RequestProcessProposal) (*ReqRes, error) {


Why do we need this? Can't we just move it into ProcessProposalSync?

Yeah we can remove this. I originally added it since I thought it was part of the required ABCI method scaffolding, but I don't think it is

consensus/byzantine_test.go

consensus/invalid_test.go

alexanderbez · 2021-05-20T12:15:17Z

proto/tendermint/abci/types.proto

@@ -117,6 +118,12 @@ message RequestApplySnapshotChunk {
  string sender = 3;
 }



formatting seems off in this file?

state/execution.go

alexanderbez · 2021-05-20T12:17:40Z

state/execution.go

+	// TODO: Need to implement deserialization of evidence bytes
+	// for _, ev := range resp.Evidence {
+	// 	blockExec.evpool.AddEvidence()
+	// }


The godoc states we process evidence by adding it to the evidence pool. Do we actually do that?

The evidence pool interface only takes in things of type types.Evidence

tendermint/state/services.go

Lines 42 to 47 in e334555

type EvidencePool interface {

PendingEvidence(maxBytes int64) (ev []types.Evidence, size int64)

AddEvidence(types.Evidence) error

Update(State, types.EvidenceList)

CheckEvidence(types.EvidenceList) error

}

Don't we want abci.ResponseProcessProposal to return []types.Evidence instead of [][]byte

More broadly, how is Tendermint expected to process application evidence? When it receives it from the application here does it just trust that it is valid. When it receives application evidence from another node does it run something like CheckEvidence() to make the application validate it.

alexanderbez · 2021-05-20T12:18:45Z

consensus/state.go

 		return
 	}

+	if mustVoteNil {


can you explain when this happens and why?

At the moment, all the validation for determining whether or not we should vote nil happens in the doprevote method. However we want to process the proposal/header earlier on, and not duplicate that. So I added this as a parameter called mustVoteNil. (If process proposal says the block is invalid, vote nil)

Just realized, an alternate approach would be to add a check for if cs.ValidBlock == nil, and then make this gate the block being deemed valid.

☝️ ~~This would also make sense to me~~

ah no wait ValidBlock depends on 2/3+ prevotes which wouldn't work.

I would instead add the check:

stateMachineValidBlock, err := cs.blockExec.ProcessProposal(cs.ProposalBlock) if err != nil { cs.Logger.Error("state machine returned an error when trying to process proposal block", "err", err) }

here as opposed to in addProposalBlockPart(). I also think we should be validating the block before getting the application to process it

consensus/state.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

github-actions · 2021-06-08T00:18:27Z

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 · 2021-06-19T00:08:25Z

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 · 2021-06-30T00:07:05Z

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.

ValarDragon · 2021-07-07T17:55:40Z

consensus/state.go

@@ -1939,7 +1953,8 @@ func (cs *State) addProposalBlockPart(msg *BlockPartMessage, peerID p2p.NodeID)



Should we rename cs.ValidBlock to cs.TwoThirdPrevoteBlock, to indicate that its just about whether there are two/thirds prevotes, and not about Tendermint / App-determined validity of the block.

I think this is a great idea. The naming should at least communicate if this is a block pre-finalization or after.

cmwaters · 2021-07-12T10:57:32Z

I've been digging around consensus for a bit and I want to try get some of the nomenclature clarified:

There are two places where we set ValidBlock:

tendermint/internal/consensus/state.go

Line 1922 in 581dd01

cs.ValidBlock = cs.ProposalBlock
tendermint/internal/consensus/state.go

Line 2092 in 581dd01

cs.ValidBlock = cs.ProposalBlock

This indicates that a ValidBlock is when 2f + 1 have prevoted for the node's ProposalBlock.

LockedBlock only gets set in one place:

tendermint/internal/consensus/state.go

Line 1444 in 581dd01

cs.LockedBlock = cs.ProposalBlock

This is when we've received 2f + 1 votes for the ProposalBlock and just before the node precommits.
So these two things are subtly different.

I do think that calling it Valid could be confusing since it doesn't relate to whether Tendermint or the application find the block to be valid but instead signals that 2/3 have prevoted on it (and thus think it is valid). We could add an extra field POLBlock if we wanted to distinguish these two or we could leave it as is and instead make cs.ProposalBlock nil if either Tendermint or the application find the proposed block to be invalid.

Option 1: Introduce `POLBlock`:

Update cs.RoundState to include POLBlock and POLRound (and remove ValidRound)
Rename all uses of ValidBlock to POLBlock and ValidRound to POLRound
Add the following lines after

tendermint/internal/consensus/state.go

Line 1901 in 581dd01

cs.ProposalBlock = block

// application validity check
appValid, err := cs.blockExec.ProcessProposal(block)
if err != nil {
	cs.Logger.Error("state machine returned an error when trying to process proposal block", "err", err)
}

// tendermint validity check
tmValid := cs.blockExec.ValidateBlock(cs.state, block)
if tmValid != nil {
    cs.Logger.Error("Proposal block is invalid, "err", err)
}

if err != nil && appValid {
    cs.ValidBlock = block
}

and then check if we can update POLBlock and POLRound:

tendermint/internal/consensus/state.go

Lines 1910 to 1923 in 581dd01

    
           // Update Valid* if we can. 
        
           prevotes := cs.Votes.Prevotes(cs.Round) 
        
           blockID, hasTwoThirds := prevotes.TwoThirdsMajority() 
        
           if hasTwoThirds && !blockID.IsZero() && (cs.ValidRound < cs.Round) { 
        
           	if cs.ProposalBlock.HashesTo(blockID.Hash) { 
        
           		cs.Logger.Debug( 
        
           			"updating valid block to new proposal block", 
        
           			"valid_round", cs.Round, 
        
           			"valid_block_hash", cs.ProposalBlock.Hash(), 
        
           		) 
        
           		cs.ValidRound = cs.Round 
        
           		cs.ValidBlock = cs.ProposalBlock 
        
           		cs.ValidBlockParts = cs.ProposalBlockParts

4. In defaultDoPrevote remove the ValidateBlock check and add:

if cs.ValidBlock == nil {
    logger.Debug("prevote step: ValidBlock is nil")
    cs.signAddVote(tmproto.PrevoteType, nil, types.PartSetHeader{})
    return
}

Option 2: Ensure `ProposalBlock` is always valid

Add

// application validity check
appValid, err := cs.blockExec.ProcessProposal(block)
if err != nil {
	cs.Logger.Error("state machine returned an error when trying to process proposal block", "err", err)
}

// tendermint validity check
err := cs.blockExec.ValidateBlock(cs.state, block)
if err != nil {
    cs.Logger.Error("Proposal block is invalid, "err", err)
}

if err != nil && appValid {
    // now we can add this block as the proposal block
    cs.ProposalBlock = block

    // publish event
    // check if we can update valid
}

// do the state transition logic

Add a case in tryFinalizeCommit for when ProposalBlock is nil

Option 2 has a smaller surface area which for me makes it more desirable

Wdyt? @ValarDragon @milosevic

cmwaters · 2021-07-13T12:54:07Z

Another thought I had: Would we ever need to call ProcessProposal whilst processing the blocks in fast sync?

github-actions · 2021-07-25T00:06:52Z

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.

ValarDragon · 2021-07-25T01:51:54Z

Apologies for the delay in response here. I prefer option 2, always ensuring the proposal block is valid

cmwaters · 2021-07-26T08:52:51Z

Okay, that sounds good. Let's add the logic then to addProposalBlockPart()

github-actions · 2021-08-07T00:06:52Z

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.

liamsi · 2021-08-13T20:29:13Z

I see the blocked label applied. Is this blocked only because of time constraints or actually blocked on or due to some other issue? Is there anything we can do to unblock this PR?

github-actions · 2021-08-24T00:08:14Z

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.

ValarDragon · 2021-08-24T03:42:54Z

I'm finally with more access to coding time this week, will get this done by Thursday.

github-actions · 2021-09-04T00:07:14Z

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.

ValarDragon · 2021-09-11T03:10:07Z

Joon's taking this over in a separate PR

ValarDragon added 4 commits May 19, 2021 08:39

Add Serial ProcessProposal boilerplate to Tendermint

0a03c5f

Actual logic for process proposal

ad48c70

Add test in block executor

f713c2d

Missed an implementation in mock, Fix grpc client

997bf88

ValarDragon requested review from alexanderbez, cmwaters, ebuchman, tessr and tychoish as code owners May 19, 2021 15:58

Add mustPrevoteNil

b49e37c

alexanderbez reviewed May 20, 2021

View reviewed changes

Apply suggestions from Bez

216b838

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

github-actions bot added the stale for use by stalebot label Jun 8, 2021

tac0turtle added C:abci Component: Application Blockchain Interface S:blocked Status: Blocked (link to issue which is needed to unblock) and removed stale for use by stalebot labels Jun 8, 2021

github-actions bot added the stale for use by stalebot label Jun 19, 2021

tac0turtle removed the stale for use by stalebot label Jun 19, 2021

github-actions bot added the stale for use by stalebot label Jun 30, 2021

tac0turtle removed the stale for use by stalebot label Jun 30, 2021

ValarDragon commented Jul 7, 2021

View reviewed changes

cmwaters changed the title ~~Process Proposal~~ abci: Process Proposal Jul 13, 2021

liamsi mentioned this pull request Jul 16, 2021

Proposal: Immediate execution to accomodate ISRs celestiaorg/celestia-core#463

Closed

github-actions bot added the stale for use by stalebot label Jul 25, 2021

github-actions bot removed the stale for use by stalebot label Jul 27, 2021

github-actions bot added the stale for use by stalebot label Aug 7, 2021

cmwaters removed the stale for use by stalebot label Aug 9, 2021

github-actions bot added the stale for use by stalebot label Aug 24, 2021

cmwaters removed the stale for use by stalebot label Aug 24, 2021

github-actions bot added the stale for use by stalebot label Sep 4, 2021

github-actions bot closed this Sep 9, 2021

tac0turtle reopened this Sep 10, 2021

tac0turtle removed the stale for use by stalebot label Sep 10, 2021

ValarDragon closed this Sep 11, 2021

This was referenced Jan 31, 2022

ABCI++: Rebase existing PR for ProcessProposal #7735

Closed

Rebased to master the existing ProcessProposal PR #7752

Merged

		@@ -117,6 +118,12 @@ message RequestApplySnapshotChunk {
		string sender = 3;
		}

	type EvidencePool interface {
	PendingEvidence(maxBytes int64) (ev []types.Evidence, size int64)
	AddEvidence(types.Evidence) error
	Update(State, types.EvidenceList)
	CheckEvidence(types.EvidenceList) error
	}

		@@ -1939,7 +1953,8 @@ func (cs State) addProposalBlockPart(msg BlockPartMessage, peerID p2p.NodeID)

Conversation

ValarDragon commented May 19, 2021 • edited by tac0turtle Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmwaters May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jun 8, 2021

Uh oh!

github-actions bot commented Jun 19, 2021

Uh oh!

github-actions bot commented Jun 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmwaters commented Jul 12, 2021

Option 1: Introduce POLBlock:

Option 2: Ensure ProposalBlock is always valid

Uh oh!

cmwaters commented Jul 13, 2021

Uh oh!

github-actions bot commented Jul 25, 2021

Uh oh!

ValarDragon commented Jul 25, 2021

Uh oh!

cmwaters commented Jul 26, 2021

Uh oh!

github-actions bot commented Aug 7, 2021

Uh oh!

liamsi commented Aug 13, 2021

Uh oh!

github-actions bot commented Aug 24, 2021

Uh oh!

ValarDragon commented Aug 24, 2021

Uh oh!

github-actions bot commented Sep 4, 2021

Uh oh!

ValarDragon commented Sep 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ValarDragon commented May 19, 2021 •

edited by tac0turtle

Loading

codecov bot commented May 20, 2021 •

edited

Loading

cmwaters May 28, 2021 •

edited

Loading

Option 1: Introduce `POLBlock`:

Option 2: Ensure `ProposalBlock` is always valid