abci: add preprocess block by tac0turtle · Pull Request #110 · celestiaorg/celestia-core

tac0turtle · 2020-12-11T15:33:41Z

Description

This pr adds preprocess block to abci

Closes: #XXX

liamsi · 2020-12-12T22:14:18Z

liamsi

Did a first pass. Awesome work!

I think we should populate the lazyledger specific fields directly instead of introducing a MetaData field in Block.Data.

abci/types/application.go

proto/tendermint/types/types.proto

types/block.go

state/execution.go

liamsi · 2020-12-14T16:38:31Z

state/execution.go

+
+	messages := types.MessagesFromProto(pbmessages)
+
+	return state.MakeBlock(height, processedTxs, evidence, nil, messages, commit, proposerAddr)


abci/types/application.go

state/execution.go

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

codecov-io · 2020-12-14T17:13:17Z

Codecov Report

Merging #110 (dd9208a) into master (7ae7b13) will increase coverage by 1.70%.
The diff coverage is 4.00%.

@@            Coverage Diff             @@
##           master     tendermint/spec#110      +/-   ##
==========================================
+ Coverage   60.81%   62.51%   +1.70%     
==========================================
  Files         261      145     -116     
  Lines       24216    12092   -12124     
==========================================
- Hits        14726     7559    -7167     
+ Misses       7954     3718    -4236     
+ Partials     1536      815     -721

Impacted Files	Coverage Δ
state/execution.go	`64.80% <0.00%> (-3.56%)`	⬇️
state/state.go	`83.89% <ø> (ø)`
types/block.go	`70.96% <0.00%> (-1.07%)`	⬇️
types/test_util.go	`55.35% <100.00%> (ø)`
cmd/tendermint/commands/show_node_id.go
abci/types/pubkey.go
p2p/errors.go
p2p/trust/config.go
p2p/node_info.go
consensus/types/round_state.go
... and 101 more

…ledger-core into marko/preprocess

liamsi

Great work @marbar3778! Thanks a lot.

LGTM

liamsi · 2020-12-15T13:09:44Z

.github/workflows/tests.yml

-      - name: Set GOBIN
-        run: |
-          echo "::add-path::$(go env GOPATH)/bin"


Thanks for taking care of this too! 🙏

liamsi · 2020-12-15T13:11:27Z

abci/types/application.go

+	DeliverTx(RequestDeliverTx) ResponseDeliverTx             // Deliver a tx for full processing
+	EndBlock(RequestEndBlock) ResponseEndBlock                // Signals the end of a block, returns changes to the validator set
+	Commit() ResponseCommit                                   // Commit the state and return the application Merkle root hash
+	PreprocessTxs(RequestPreprocessTxs) ResponsePreprocessTxs // State machine preprocessing of txs


I'd move this in between InitChain and BeginBlock. But as this is only the interface it does not block merging this PR.

liamsi · 2020-12-15T13:17:36Z

proto/tendermint/abci/types.proto

+message ResponsePreprocessTxs {
+  repeated bytes            txs      = 1;
+  tendermint.types.Messages messages = 2;
+}


In a follow-ip issue, this will also return/include the intermediate state roots. Then, in another PR, this will enable immediate execution (ref #3 and https://github.com/tendermint/spec/issues/208#issuecomment-721158558)

liamsi · 2020-12-15T13:22:00Z

types/block.go

+	MessageEmpty  = Message{}
+	MessagesEmpty = Messages{}


I think these vars won't be used outside of this package? If so, they should be unexported.

liamsi · 2020-12-15T13:25:53Z

blockchain/v0/reactor_test.go

+	block, _ := state.MakeBlock(height, makeTxs(height), nil,
+		nil, types.Messages{}, lastCommit, state.Validators.GetProposer().Address)


Optional nit: I think, here and in other places, where the line becomes too large, we should rather use one line per argument. I think that improves the readability compared to breaking the line somewhere in the middle.
Didn't this use to be convention in the tendermint code-base anyway?

liamsi · 2020-12-15T18:51:54Z

The failing test, seems to be a non-deterministic flaky test unrelated to the changes here: tendermint/tendermint#1040

* version docs * stash commits * fix url * remove master from sidebar * fix build errors * move to consensus connection * add process * add proof to block * add block metadata in beginBlcok * fix testing * regenerate mocks * add in needed proto changes * regenerate mocks * fix makeblock calls * Apply suggestions from code review * fix rpc tests * fix linting and ci errors * fix consensus tests * add preprocess to e2e * add preprocess to counter app * replace meta_data with messages * fix linting * Update state/execution.go Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> * fix comment * fix e2e tests Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

* version docs * stash commits * fix url * remove master from sidebar * fix build errors * move to consensus connection * add process * add proof to block * add block metadata in beginBlcok * fix testing * regenerate mocks * add in needed proto changes * regenerate mocks * fix makeblock calls * Apply suggestions from code review * fix rpc tests * fix linting and ci errors * fix consensus tests * add preprocess to e2e * add preprocess to counter app * replace meta_data with messages * fix linting * Update state/execution.go Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> * fix comment * fix e2e tests Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com> fix test so that it doesn't modify block data after generating the hash fix missed make block api change run make mockery add some docs fix hardcoded test fix linter fix hard coded test

* [backport] Renaming in docker-related files (#123) * [cherry-picked] Renaming in docker-related files (#110) * Performed renaming in docker-related files * Fix e2e launch * Rename TENDERMINT_BUILD_OPTIONS to COMETBFT_BUILD_OPTIONS all over * Revert `cometbft-proof` to `tendermint-proof` * Replaced TENDERMINT_VERSION --> COMETBFT_VERSION all over the codebase * [backport] Rename inside `test/e2e` (#139) (#144) * Renamed all help text for cometbft CLI * Update networks/local/localnode/Dockerfile Co-authored-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Thane Thomson <connect@thanethomson.com>

* Performed renaming in docker-related files * Fix e2e launch * one more

* Rename TENDERMINT_BUILD_OPTIONS to COMETBFT_BUILD_OPTIONS all over * Linter error

tac0turtle and others added 30 commits August 13, 2020 13:09

version docs

4a9247c

stash commits

a46562d

fix url

99cf9c0

remove master from sidebar

c51a1f5

test go version

296d854

remove go version

25de71a

Merge branch 'master' into marko/versioned_docs

0a24577

invert versions

0cdebf6

Merge branch 'master' into marko/versioned_docs

c19e0d3

Merge branch 'master' into marko/versioned_docs

db3140f

Merge branch 'master' into marko/versioned_docs

7756e2d

Merge branch 'master' into marko/versioned_docs

828880d

Merge branch 'master' into marko/versioned_docs

8f37751

add todo

1afe9db

init

b118204

changes to abci

682a16d

fix build errors

35ca92b

move to consensus connection

6a8161d

add process

6ea48d0

add proof to block

0caedd2

undo changes

cb770d6

add block metadata in beginBlcok

4677f69

Merge branch 'master' into marko/preprocess

e8e7179

fix testing

eec2c96

regenerate mocks

7078389

add in needed proto changes

60fa00d

regenerate mocks

c527158

Merge branch 'master' into marko/preprocess

2efa9fb

fix makeblock calls

ff4be69

Merge branch 'master' into marko/preprocess

46f1be9

tac0turtle added 2 commits December 11, 2020 16:35

add preprocess to e2e

4bb6af4

add preprocess to counter app

4e3abfd

liamsi reviewed Dec 13, 2020

View reviewed changes

abci/types/application.go Outdated Show resolved Hide resolved

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

types/block.go Outdated Show resolved Hide resolved

replace meta_data with messages

6842950

liamsi reviewed Dec 14, 2020

View reviewed changes

state/execution.go Outdated Show resolved Hide resolved

liamsi reviewed Dec 14, 2020

View reviewed changes

abci/types/application.go Outdated Show resolved Hide resolved

liamsi reviewed Dec 14, 2020

View reviewed changes

state/execution.go Show resolved Hide resolved

tac0turtle and others added 3 commits December 14, 2020 18:09

fix linting

acd911a

Update state/execution.go

7d179d2

Co-authored-by: Ismail Khoffi <Ismail.Khoffi@gmail.com>

fix comment

add6537

tac0turtle added 2 commits December 15, 2020 10:58

Merge branch 'marko/preprocess' of https://github.com/marbar3778/lazy…

088b4ce

…ledger-core into marko/preprocess

fix e2e tests

dd9208a

tac0turtle requested a review from liamsi December 15, 2020 12:10

liamsi approved these changes Dec 15, 2020

View reviewed changes

liamsi merged commit fbf8ebb into celestiaorg:master Dec 15, 2020

liamsi mentioned this pull request Dec 16, 2020

Implement tendermint/issue#2639 (preprocessing of blocks) #59

Closed

liamsi mentioned this pull request Jan 4, 2021

Use preprocess step to decouple Tx and Messages before proposing a block celestiaorg/cosmos-sdk#5

Closed

liamsi mentioned this pull request Feb 8, 2021

ABCI interface evstack/ev-node#1

Closed

evan-forbes mentioned this pull request Sep 22, 2021

Cherry pick PreprocessTxs #541

Merged

evan-forbes pushed a commit that referenced this pull request Mar 11, 2025

Renaming in docker-related files (#110)

d9cae0c

* Performed renaming in docker-related files * Fix e2e launch * one more

evan-forbes pushed a commit that referenced this pull request Mar 11, 2025

Fix e2e nightlies in CI, broken by #110 (#138)

fc48e6f

* Rename TENDERMINT_BUILD_OPTIONS to COMETBFT_BUILD_OPTIONS all over * Linter error


		messages := types.MessagesFromProto(pbmessages)

		return state.MakeBlock(height, processedTxs, evidence, nil, messages, commit, proposerAddr)

		block, _ := state.MakeBlock(height, makeTxs(height), nil,
		nil, types.Messages{}, lastCommit, state.Validators.GetProposer().Address)

Conversation

tac0turtle commented Dec 11, 2020 • edited by liamsi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

liamsi commented Dec 12, 2020

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liamsi Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

liamsi Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

liamsi Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

liamsi Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

liamsi Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

liamsi Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

liamsi commented Dec 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tac0turtle commented Dec 11, 2020 •

edited by liamsi

Loading

codecov-io commented Dec 14, 2020 •

edited

Loading