Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

abci: PrepareProposal #6544

Merged
merged 23 commits into from
Jul 27, 2021
Merged

abci: PrepareProposal #6544

merged 23 commits into from
Jul 27, 2021

Conversation

tac0turtle
Copy link
Contributor

@tac0turtle tac0turtle commented Jun 7, 2021

Description

PrepareProposal implementation.

TODO:

  • modify apps
  • fix tests
  • modify e2e

ref #6066

cc @ValarDragon

@tac0turtle tac0turtle added the C:abci Component: Application Blockchain Interface label Jun 7, 2021
@tac0turtle tac0turtle self-assigned this Jun 7, 2021
@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #6544 (26de7e9) into abci++ (c3ae6f5) will decrease coverage by 1.97%.
The diff coverage is 0.00%.

❗ Current head 26de7e9 differs from pull request most recent head a6f5205. Consider uploading reports for the commit a6f5205 to get more accurate results

@@            Coverage Diff             @@
##           abci++    #6544      +/-   ##
==========================================
- Coverage   62.39%   60.41%   -1.98%     
==========================================
  Files         298      299       +1     
  Lines       39991    27943   -12048     
==========================================
- Hits        24951    16881    -8070     
+ Misses      13269     9344    -3925     
+ Partials     1771     1718      -53     
Impacted Files Coverage Δ
abci/client/client.go 32.35% <ø> (-3.82%) ⬇️
abci/client/grpc_client.go 0.00% <0.00%> (ø)
abci/client/local_client.go 0.00% <0.00%> (ø)
abci/client/socket_client.go 37.26% <0.00%> (-3.30%) ⬇️
abci/example/kvstore/kvstore.go 67.07% <0.00%> (-6.97%) ⬇️
abci/example/kvstore/persistent_kvstore.go 39.83% <0.00%> (-9.86%) ⬇️
abci/types/application.go 0.00% <0.00%> (ø)
abci/types/messages.go 7.24% <0.00%> (+1.69%) ⬆️
proxy/app_conn.go 20.51% <0.00%> (-6.16%) ⬇️
state/execution.go 66.05% <0.00%> (+0.73%) ⬆️
... and 390 more

@tac0turtle tac0turtle marked this pull request as ready for review June 7, 2021 13:46
@tac0turtle
Copy link
Contributor Author

Blocked on spec version release.

@tac0turtle tac0turtle added the S:blocked Status: Blocked (link to issue which is needed to unblock) label Jun 7, 2021
@tac0turtle
Copy link
Contributor Author

tac0turtle commented Jun 7, 2021

@ValarDragon in one section of the RFC you mention sending only blockdata (txs) but in a different section you mention also the header.

Could you double-check this is right?

@ValarDragon
Copy link
Contributor

The header should came after vote extensions are built, right now it should just have the tx data

@tessr
Copy link
Contributor

tessr commented Jun 7, 2021

(does this PR have the right name?)

@tac0turtle tac0turtle changed the title abci: Preproces proposal abci: PrepareProposal Jun 7, 2021
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a relative straight forward PR to me.

Since PrepareProposal also acts like a CheckTx, do you think it would be possible to remove or adjust the way we RecheckTx after every block

state/execution.go Outdated Show resolved Hide resolved
state/execution.go Outdated Show resolved Hide resolved
state/execution.go Outdated Show resolved Hide resolved
abci/client/grpc_client.go Outdated Show resolved Hide resolved
abci/client/grpc_client.go Outdated Show resolved Hide resolved
abci/client/grpc_client.go Outdated Show resolved Hide resolved
abci/client/local_client.go Outdated Show resolved Hide resolved
abci/client/socket_client.go Outdated Show resolved Hide resolved
test/e2e/app/app.go Outdated Show resolved Hide resolved
Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dope 👍🏼

@github-actions
Copy link

github-actions bot commented Jul 2, 2021

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.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @marbar3778! Left a bit of small feedback.

Comment on lines +175 to +177
if len(req.BlockData) >= 1 {
req.BlockData[1] = []byte("modified tx")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a quick comment on what we're doing here in this example.

internal/consensus/mempool_test.go Outdated Show resolved Hide resolved
proto/tendermint/abci/types.proto Show resolved Hide resolved
test/e2e/app/app.go Outdated Show resolved Hide resolved
types/tx.go Show resolved Hide resolved
types/tx.go Outdated Show resolved Hide resolved
state/execution.go Outdated Show resolved Hide resolved
state/execution.go Outdated Show resolved Hide resolved
state/execution.go Outdated Show resolved Hide resolved
tac0turtle and others added 6 commits July 15, 2021 14:40
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
@github-actions
Copy link

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 github-actions bot added the stale for use by stalebot label Jul 26, 2021
@tac0turtle tac0turtle removed the stale for use by stalebot label Jul 26, 2021
@tac0turtle tac0turtle merged commit 7d30987 into abci++ Jul 27, 2021
@tac0turtle tac0turtle deleted the preProcesProposal branch July 27, 2021 07:30
sergio-mena pushed a commit that referenced this pull request Jan 31, 2022
sergio-mena pushed a commit that referenced this pull request Feb 2, 2022
sergio-mena pushed a commit that referenced this pull request Feb 2, 2022
This was referenced Jul 22, 2022
sergio-mena added a commit that referenced this pull request Jul 27, 2022
* [Rebased to v0.34.x] abci: PrepareProposal (#6544)

* fixed cherry-pick

* proto changes

* make proto-gen

* UT fixes

* generate Client directive

* mockery

* App fixes

* Disable 'modified tx' hack

* mockery

* Make format

* Fix lint

Co-authored-by: Marko <[email protected]>
samricotta pushed a commit that referenced this pull request Aug 16, 2022
* [Rebased to v0.34.x] abci: PrepareProposal (#6544)

* fixed cherry-pick

* proto changes

* make proto-gen

* UT fixes

* generate Client directive

* mockery

* App fixes

* Disable 'modified tx' hack

* mockery

* Make format

* Fix lint

Co-authored-by: Marko <[email protected]>
cmwaters pushed a commit to cmwaters/tendermint that referenced this pull request Dec 12, 2022
* abci: PrepareProposal (tendermint#6544)

* regenerate mocks, proto, mod/sum, and clean up remaining preprocesstxs

* fix tests and revert to old go.mod

* mockery

* add processproposal proto/boilerplate/logic

* gofmt

* fix test

* move UNKNOWN response behaviour to reject

* fix test and add testing util code

* pass full block data when proposing or processing proposals

* linter

* add the process proposal method to the e2e app

* add missing kvstore abci method

* pass block data and results for bass app

* use correct kvstore process logic for kvstore app

* remove linting comment

Co-authored-by: Ismail Khoffi <[email protected]>

* formatting

Co-authored-by: John Adler <[email protected]>

* use go generate instead of make mockery

* add link

Co-authored-by: Marko <[email protected]>
Co-authored-by: mconcat <[email protected]>
Co-authored-by: Ismail Khoffi <[email protected]>
Co-authored-by: John Adler <[email protected]>
cmwaters pushed a commit to cmwaters/tendermint that referenced this pull request Dec 12, 2022
* abci: PrepareProposal (tendermint#6544)

* regenerate mocks, proto, mod/sum, and clean up remaining preprocesstxs

* fix tests and revert to old go.mod

* mockery

* add processproposal proto/boilerplate/logic

* gofmt

* fix test

* move UNKNOWN response behaviour to reject

* fix test and add testing util code

* pass full block data when proposing or processing proposals

* linter

* add the process proposal method to the e2e app

* add missing kvstore abci method

* pass block data and results for bass app

* use correct kvstore process logic for kvstore app

* add new lazy share writers

* linter

* remove unused arg

* sort messages before exporting

* formatting and bug fix

* fix tests

* allow for picking of square size when computing shares for the data square

* remove accidental code duplication

* fix test from using wrong formatting directive

* linter

* ci: backport lint configuration changes (tendermint#7225)

* lint: cleanup pending lint errors  (tendermint#7237)

* linter

* remove unused arg

* sort messages before exporting

* formatting and bug fix

* fix tests

* allow for picking of square size when computing shares for the data square

* fix test from using wrong formatting directive

* linter

* ci: backport lint configuration changes (tendermint#7225)

* lint: cleanup pending lint errors  (tendermint#7237)

* add new lazy share writers

* fix rebase

* linter

* try ci with go 1.17

* Revert "try ci with go 1.17"

This reverts commit 0f76b4d444465cf8209bd98bb18f21b242207436.

* please work, linter gods

* spelling

Co-authored-by: Hlib Kanunnikov <[email protected]>

* initialize pending share using the const share size for capacity

* force the last reserve bytes to be zero

* remove todo

* add typecheck back to golang linter

* Revert "ci: backport lint configuration changes (tendermint#7225)"

This reverts commit 35178048f0fef2d80a0346f28c7687be75c3db11.

* Revert "lint: cleanup pending lint errors  (tendermint#7237)"

This reverts commit 5d806709a9dbfed4f7dda2facb49d46604584d36.

* add the link to issue back in

* removed unfinished comment

* switch fuzzer back to one minute

* regenerate proto

* better docs

* fix encoding check to include the hash added to Data

* add docs to CotiguousShareWriter

* fix encoding check

* explain why the share reserve byte is zero

* use punctuation

Co-authored-by: Ismail Khoffi <[email protected]>

* use punctuation

Co-authored-by: Ismail Khoffi <[email protected]>

* use punctuation

Co-authored-by: Ismail Khoffi <[email protected]>

* use punctuation

Co-authored-by: Ismail Khoffi <[email protected]>

* use punctuation

Co-authored-by: Ismail Khoffi <[email protected]>

* use punctuation

Co-authored-by: Ismail Khoffi <[email protected]>

* use clearer wording for compute shares docs

* more accurate docs

Co-authored-by: Marko <[email protected]>
Co-authored-by: mconcat <[email protected]>
Co-authored-by: Sam Kleinman <[email protected]>
Co-authored-by: Hlib Kanunnikov <[email protected]>
Co-authored-by: Ismail Khoffi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:abci Component: Application Blockchain Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants