abci: implement finalize block#9468
Conversation
|
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. |
|
@cmwaters is this branch safe to be used as a basis of implementing |
sergio-mena
left a comment
There was a problem hiding this comment.
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 ?
| EchoAsync(msg string) *ReqRes | ||
| InfoAsync(types.RequestInfo) *ReqRes | ||
| DeliverTxAsync(types.RequestDeliverTx) *ReqRes | ||
| CheckTxAsync(types.RequestCheckTx) *ReqRes |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| reqRes := NewReqRes(req) | ||
| reqRes.Response = res | ||
| return reqRes | ||
| return app.Application.FinalizeBlock(ctx, req) |
There was a problem hiding this comment.
In v0.37.x Application interface does not return an error here, why this change?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
| Tx: b.Data.Txs[i], | ||
| Result: *(r.DeliverTxs[i]), | ||
| Height: height, | ||
| Index: uint32(idx), |
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion
| 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 { |
Check failure
Code scanning / gosec
Expect directory permissions to be 0750 or less
mempool/v1/mempool.go
Outdated
| }) | ||
| } | ||
| _ = txmp.proxyAppConn.FlushAsync() | ||
| _ = txmp.proxyAppConn.Flush(context.TODO()) |
Check warning
Code scanning / gosec
Returned error is not propagated up the stack.
| // 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 { |
There was a problem hiding this comment.
👏 👏 👏
I think we didn't get to this case in v0.36. Nice.
sergio-mena
left a comment
There was a problem hiding this comment.
Thanks @cmwaters for your patience.
Please make sure you address @williambanfield's comments
williambanfield
left a comment
There was a problem hiding this comment.
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.
| legacyResp := new(tmstate.LegacyABCIResponses) | ||
| rerr := legacyResp.Unmarshal(buf) | ||
| if rerr != nil { | ||
| // DATA HAS BEEN CORRUPTED OR THE SPEC HAS CHANGED |
There was a problem hiding this comment.
Can we write a test for the case of loading the new type when the old type is present in the db?
|
Thanks @cmwaters ! |
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:
Clientinterface (as previously done)abci.Clientover four connections is still in placeAppHashinResponseFinalizeBlockwas renamed toAgreedAppDatato 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.PersistentKVStoreApplicationandKVStoreApplicationwere merged to a single struct with aDBobject passed in the constructor. This means an "in-memory" db can be used or the "go-cleveldb" in the case of the persistent application.EventSinkabstraction 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 theBlockIndexeroperational, I've added a newEventDataNewBlockEventsstruct which specifically contains theEventsfromResponseFinalizeBlock(which concatenates the previous BeginBlock and EndBlock events). The other option would have been to useEventDataNewBlockwhich would provide the entire block (not just the block events needed).ABCIResponsesasLegacyABCIResponsesto maintain backwards compatibility. IfLoadFinalizeBlockResponsesis called, it will first try to unmarshal the bytes asResponseFinalizeBlockand then asLegacyABCIResponses. If it is the legacy data it will then convert it toReponseFinalizeBlockand return that. We will need to follow up on this later as it's possible thatResponseFinalizeBlockdoes not need to be persisted at all but I've left that decision for the future.Flushmethod still exists but it is now redundant.For the Reviewer, the significant changes can be found:
proto/tendermint/abciandproto/tendermint/state(for the proto changes)abcipackagestate/execution.gomempool/v0/clist_mempool.go(removal of async and thus the callbacks)state/store.go. The handling of persisting and loadingResponseFinalizeBlockThe PR can already be viewed but I still need to:
UPGRADING.md