abci: implement finalize block #9468

cmwaters · 2022-09-21T12:09:47Z

Closes: #9427

This PR ports the work around finalize block present in v0.36. Given extensive changes in the abci++ package prior to the finalize block changes, this was manually done and several decisions were made in the process, of note:

The removal of the Async methods in the Client interface (as previously done)
The addition of contexts and errors and usage of pointers instead of concrete structs.
The ported commits depended on changes in the ABCI concurrency model. This was reverted to the previous behaviour (as it is in v0.34/v0.37). As such:
- The proxyApp which multiplexes the abci.Client over four connections is still in place
- The global mutex around the local client is still in place
AppHash in ResponseFinalizeBlock was renamed to AgreedAppData to signify that the returned values do not need to be a hash and any arbitrary size of bytes can be used to demonstrate state machine replication.
The PersistentKVStoreApplication and KVStoreApplication were merged to a single struct with a DB object passed in the constructor. This means an "in-memory" db can be used or the "go-cleveldb" in the case of the persistent application.
The prior commits relied on the EventSink abstraction introduced in v0.35 and v0.36 but not present in v0.34 and v0.37. This work (or some rendition of it) should still be done, but in order to get the BlockIndexer operational, I've added a new EventDataNewBlockEvents struct which specifically contains the Events from ResponseFinalizeBlock (which concatenates the previous BeginBlock and EndBlock events). The other option would have been to use EventDataNewBlock which would provide the entire block (not just the block events needed).
I have kept ABCIResponses as LegacyABCIResponses to maintain backwards compatibility. If LoadFinalizeBlockResponses is called, it will first try to unmarshal the bytes as ResponseFinalizeBlock and then as LegacyABCIResponses. If it is the legacy data it will then convert it to ReponseFinalizeBlock and return that. We will need to follow up on this later as it's possible that ResponseFinalizeBlock does not need to be persisted at all but I've left that decision for the future.
The ported commit includes fixes to the ABCI socket protocol which one can find here (abci: Flush socket requests and responses immediately. #6997). This flushes the buffer after every write. The Flush method still exists but it is now redundant.

For the Reviewer, the significant changes can be found:

in proto/tendermint/abci and proto/tendermint/state (for the proto changes)
in the abci package
in state/execution.go
in mempool/v0/clist_mempool.go (removal of async and thus the callbacks)
in state/store.go. The handling of persisting and loading ResponseFinalizeBlock

The PR can already be viewed but I still need to:

Add notes in UPGRADING.md
Add a changelog entry

github-actions · 2022-10-21T00:22:56Z

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.

faustbrian · 2022-10-21T00:48:23Z

@cmwaters is this branch safe to be used as a basis of implementing FinalizeBlock in ABCI++ applications until it gets merged or are there any bigger changes planned?

sergio-mena

I have started reviewing this. A +6K, -8K PR is certainly going to be very hard to review especially, if the backport was done manually.
I am posting the first comments I have. I will continue with this later today and after my vacation.
One general question I have: if the removal of Async was needed to make the backport of #7798 easier (I can imagine), was the corresponding commit cherry-picked first as described in #9396 ?

sergio-mena · 2022-10-24T13:11:15Z

abci/client/client.go

-	EchoAsync(msg string) *ReqRes
-	InfoAsync(types.RequestInfo) *ReqRes
-	DeliverTxAsync(types.RequestDeliverTx) *ReqRes
-	CheckTxAsync(types.RequestCheckTx) *ReqRes


Removing CheckTxAsync was decision that was made as part of v0.36. We have been revisiting all decisions made in v0.35 and v0.36, I think this one should be no exception.
Futher, I'm not sure I agree with CheckTx being synchrnous, as CheckTx is explained in the spec as an asynchronous method, which makes sense for performance reasons when the App is not in the same process.

If we want CheckTx to be asynchronous, I'd recommend simply running it in a go routine but given that calls are serialized to the application, I'm not sure what it really provides in terms of performance.

After doing a second pass on the beginning for this diff, I realize this goes further than #7607 in removing Async methods. The original #7607 leaves the possibility of leveraging some async methods in the future. This patch, which is supposed to be a backport (or a set of them), removes that possibility.

sergio-mena · 2022-10-24T13:29:39Z

abci/client/local_client.go

-	reqRes := NewReqRes(req)
-	reqRes.Response = res
-	return reqRes
+	return app.Application.FinalizeBlock(ctx, req)


In v0.37.x Application interface does not return an error here, why this change?

v0.36 added contexts and errors to all ABCI calls. I included this as I felt this made sense to port across. I don't think it would be too difficult however to remove the errors if we feel that way

OK, since you decided to port them, let's keep them. But don't you think the initial diff was big enough to try to limit the amount of things we want to bring in here?

proto/tendermint/abci/types.proto

proto/tendermint/state/types.proto

proto/tendermint/abci/types.proto

cmd/tendermint/commands/reindex_event.go

-						Tx:     b.Data.Txs[i],
-						Result: *(r.DeliverTxs[i]),
+						Height: height,
+						Index:  uint32(idx),


test/e2e/app/snapshots.go

@@ -28,7 +31,7 @@
 // NewSnapshotStore creates a new snapshot store.
 func NewSnapshotStore(dir string) (*SnapshotStore, error) {
 	store := &SnapshotStore{dir: dir}
-	if err := os.MkdirAll(dir, 0o755); err != nil {
+	if err := os.MkdirAll(dir, 0755); err != nil {


mempool/v1/mempool.go

@@ -692,7 +693,7 @@
 				return nil
 			})
 		}
-		_ = txmp.proxyAppConn.FlushAsync()
+		_ = txmp.proxyAppConn.Flush(context.TODO())


sergio-mena · 2022-11-22T23:18:22Z

consensus/replay.go

+			// v0.37 with endblock to v0.38 with finalize block and thus
+			// does not have the app hash saved from the previous height
+			// here we take the appHash provided from the Info handshake
+			if len(finalizeBlockResponse.AgreedAppData) == 0 {


👏 👏 👏
I think we didn't get to this case in v0.36. Nice.

sergio-mena

Thanks @cmwaters for your patience.
Please make sure you address @williambanfield's comments

williambanfield

The main change to replace BeginBlock and EndBlock with FinalizeBlock appears correct. There are a few extra changes that make reviewing this a bit difficult. I've added a few non-blocking suggestions.

abci/server/grpc_server.go

state/execution.go

proto/tendermint/abci/types.proto

williambanfield · 2022-11-25T19:26:49Z

state/store.go

+		legacyResp := new(tmstate.LegacyABCIResponses)
+		rerr := legacyResp.Unmarshal(buf)
+		if rerr != nil {
+			// DATA HAS BEEN CORRUPTED OR THE SPEC HAS CHANGED


Can we write a test for the case of loading the new type when the old type is present in the db?

state/store.go

sergio-mena · 2022-11-28T22:29:47Z

Thanks @cmwaters !

cmwaters added 2 commits September 20, 2022 14:14

implement finalize block

344efa2

fix broken tests

61a4e70

cmwaters requested a review from ebuchman as a code owner September 21, 2022 12:09

cmwaters requested a review from a team September 21, 2022 12:09

cmwaters added 15 commits September 22, 2022 10:35

add callback

dc0b892

fix abci_cli tests

62c6e7a

fire new block event

8f47431

fix consensus errors

e35b57d

clean up kvstore example

2dfcbcc

clean up mempool tests

e9ce605

fix mempool tests

178ea47

continue wading through tests

b5fa9b1

a bit more

6841eb1

fix grpc test

9c331e4

lint

3f1554b

fix abci_cli tests

d1616c1

fix up e2e tests

a2978b4

fix further tests

70724ca

fix tests

7f0eed6

cmwaters self-assigned this Oct 10, 2022

github-actions bot added the stale for use by stalebot label Oct 21, 2022

thanethomson removed the stale for use by stalebot label Oct 21, 2022

sergio-mena reviewed Oct 24, 2022

View reviewed changes

faustbrian reviewed Oct 26, 2022

View reviewed changes

proto/tendermint/abci/types.proto Show resolved Hide resolved

faustbrian reviewed Oct 26, 2022

View reviewed changes

proto/tendermint/state/types.proto Outdated Show resolved Hide resolved

faustbrian reviewed Oct 26, 2022

View reviewed changes

proto/tendermint/abci/types.proto Show resolved Hide resolved

Merge branch 'feature/abci++vef' into cal/finalize-block

4c0d925

github-advanced-security bot found potential problems Oct 26, 2022

View reviewed changes

cmwaters added 11 commits November 22, 2022 11:07

add grpc client test

5927691

modify socket client to using flush after every sync call

7740158

update abcicli

be8ea81

fix mempool test

77a02b2

add extra socket client test

d807d51

add TestFinalizeBlockCalled

f6ceff0

remove flush call after recheck tx

418975e

restore discard abci responses

483fa89

clean up usage of begin block, deliver tx and end block

578322a

catch scenario when recovering directly after an upgrade

15065f8

fix flakiness in test

39ff07e

sergio-mena reviewed Nov 22, 2022

View reviewed changes

sergio-mena approved these changes Nov 22, 2022

View reviewed changes

cmwaters added 3 commits November 23, 2022 16:21

fix replay test

6ce3ccc

fix replay_test

dc17927

cleanup test

27c1d6e

williambanfield approved these changes Nov 25, 2022

View reviewed changes

cmwaters added 4 commits November 28, 2022 18:28

add missing finalizeblockdecidedcommit test

d9200e3

add state store test for recovering legacy abci responses

1229a80

remove reference of begin bock

b3c6c4c

remove helper function execBlockOnProxyApp

f0207c2

thanethomson added the C:abci Component: Application Blockchain Interface label Nov 28, 2022

add changelog

5f606e2

cmwaters merged commit c5c2aaf into feature/abci++vef Nov 28, 2022

cmwaters deleted the cal/finalize-block branch November 28, 2022 22:12

mzabaluev mentioned this pull request May 17, 2023

JSON serialization of ExecTxResult breaks naming of fields as they were in ResponseDeliverTx cometbft/cometbft#855

Closed

jchappelow mentioned this pull request Aug 7, 2023

Abci refactor kwilteam/kwil-db#163

Merged

tkxkd0159 mentioned this pull request Nov 7, 2023

Triage on ostracon#156 Finschia/cometbft#31

Closed

sergio-mena mentioned this pull request May 8, 2024

fix: invalid txs_results returned for legacy ABCI responses cometbft/cometbft#3031

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

abci: implement finalize block #9468

abci: implement finalize block #9468

cmwaters commented Sep 21, 2022

github-actions bot commented Oct 21, 2022

faustbrian commented Oct 21, 2022

sergio-mena left a comment

sergio-mena Oct 24, 2022

cmwaters Oct 24, 2022

sergio-mena Nov 3, 2022

sergio-mena Oct 24, 2022

cmwaters Oct 24, 2022

sergio-mena Nov 3, 2022

sergio-mena Nov 22, 2022

sergio-mena left a comment

williambanfield left a comment

williambanfield Nov 25, 2022

sergio-mena commented Nov 28, 2022

abci: implement finalize block #9468

abci: implement finalize block #9468

Conversation

cmwaters commented Sep 21, 2022

github-actions bot commented Oct 21, 2022

faustbrian commented Oct 21, 2022

sergio-mena 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

sergio-mena left a comment

Choose a reason for hiding this comment

williambanfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergio-mena commented Nov 28, 2022